Closed Bug 1564527 Opened 5 years ago Closed 5 years ago

Let AssertEvalNotUsingSystemPrincipal ride to Nightly

Categories

(Core :: DOM: Security, enhancement, P1)

Product:
Core
Component:
DOM: Security
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla70
Tracking Flags:
Tracking Status
firefox70 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 files)

Bug 1564527 - Enable AssertEvalNotUsingSystemPrincipal on Nightly builds r?ckerschb
(deleted), text/x-phabricator-request
Details
1564527-data-review.txt
(deleted), text/plain
chutten
: data-review+
Details

We've baked this on Debug builds for some time, and we think we have a good handle on things - let's let the assertion ride out to Nightly.

I ran only a single platform because I think we've got pretty good coverage of this already: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e278472003441431adf05c1646010ba46cd3f53

Comment on attachment 9076885 [details]
Bug 1564527 - Enable AssertEvalNotUsingSystemPrincipal on Nightly builds r?ckerschb

This now needs Data Review because we use MOZ_CRASH_UNSAFE_PRINTF

I think it's a pretty non-controversial collection though. It will only report a filename in our package. Examples: https://searchfox.org/mozilla-central/source/modules/libpref/init/all.js#2519

Attachment #9076885 - Flags: data-review?(chutten)
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [domsecurity-active]

Comment on attachment 9076885 [details]
Bug 1564527 - Enable AssertEvalNotUsingSystemPrincipal on Nightly builds r?ckerschb

Data Collection Review process is outlined here: https://wiki.mozilla.org/Firefox/Data_Collection

(( Copy and fill out a form, attach it to bug, set data-review? ))

Attachment #9076885 - Flags: data-review?(chutten)
Attached file 1564527-data-review.txt (deleted) —
Attachment #9077516 - Flags: data-review?(chutten)
Comment on attachment 9077516 [details]
1564527-data-review.txt

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

ni?tjr - Is this collection covered by the [MDN documentation](https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Crash_reporting/Understanding_crash_reports) on crash reports? Is this a PII field?

    Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is associated with Crash Reports which users may opt to send. The automatic sending of crash reports (if opted in to) can be controlled in Firefox's Preferences.

    If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, Christoph Kerschbaumer is responsible.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical.

    Is the data collection request for default-on or default-off?

Default off (Crash Report submission is opt-in). Collection will reach all channels.

    Does the instrumentation include the addition of any new identifiers?

No.

    Is the data collection covered by the existing Firefox privacy notice?

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

No. This collection is permanent.

---
Result: datareview+, pending answers about documentation
Flags: needinfo?(tom)
Attachment #9077516 - Flags: data-review?(chutten) → data-review+

(In reply to Chris H-C :chutten from comment #6)

Comment on attachment 9077516 [details]
1564527-data-review.txt

DATA COLLECTION REVIEW RESPONSE:

Is there or will there be documentation that describes the schema for

the ultimate data set available publicly, complete and accurate?

ni?tjr - Is this collection covered by the [MDN
documentation](https://developer.mozilla.org/en-US/docs/Mozilla/Projects/
Crash_reporting/Understanding_crash_reports) on crash reports? Is this a PII
field?

It is not a PII field; as the filename we are reporting is a file in the Firefox installation and I haven't seen any indication that a particular filename could give rise to PII.

We are crashing with this type of crash from MDN:

Abort: A controlled abort, e.g. via NS_RUNTIMEABORT. (Controlled aborts that occur via MOZ_CRASH or MOZ_RELEASE_ASSERT currently don't get an Abort annotation, but they do get a "MOZ_CRASH Reason" field.)

The "MOZ_CRASH Reason" field is a developer-chosen field although I suppose that's not explicit...

Flags: needinfo?(tom)

Thank you for the clarification of the documentation. data-review+ stands, we're good to go!

Keywords: checkin-needed

Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6e4d13741a0f
Enable AssertEvalNotUsingSystemPrincipal on Nightly builds r=ckerschb

Keywords: checkin-needed

Backed out for causing AddressSanitizer perma fails.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=256282543&resultStatus=success%2Cpending%2Crunning%2Ctestfailed%2Cbusted%2Cexception&searchStr=linux%2Cx64%2Casan%2Cmochitests%2Ctest-linux64-asan%2Fopt-mochitest-devtools-chrome-e10s-5%2Cm%28dt5%29&revision=6e4d13741a0fa14a0e4cb8bd8a20185a1d15b7d1

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=256282543&repo=autoland&lineNumber=1470

[task 2019-07-12T22:58:59.204Z] 22:58:59 INFO - TEST-START | devtools/client/inspector/extensions/test/browser_inspector_extension_sidebar.js
[task 2019-07-12T22:59:01.805Z] 22:59:01 INFO - GECKO(1123) | AddressSanitizer:DEADLYSIGNAL
[task 2019-07-12T22:59:01.805Z] 22:59:01 INFO - GECKO(1123) | =================================================================
[task 2019-07-12T22:59:01.805Z] 22:59:01 ERROR - GECKO(1123) | ==1123==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000001 (pc 0x7f49df068c17 bp 0x7ffff5e508b0 sp 0x7ffff5e50580 T0)
[task 2019-07-12T22:59:01.805Z] 22:59:01 INFO - GECKO(1123) | ==1123==The signal is caused by a WRITE memory access.
[task 2019-07-12T22:59:01.805Z] 22:59:01 INFO - GECKO(1123) | ==1123==Hint: address points to the zero page.
[task 2019-07-12T22:59:02.419Z] 22:59:02 INFO - GECKO(1123) | #0 0x7f49df068c16 in Length /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:347:37
[task 2019-07-12T22:59:02.420Z] 22:59:02 INFO - GECKO(1123) | #1 0x7f49df068c16 in AppendElement<const nsTSubstring<char> &, nsTArrayInfallibleAllocator> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:2394
[task 2019-07-12T22:59:02.420Z] 22:59:02 INFO - GECKO(1123) | #2 0x7f49df068c16 in nsContentSecurityManager::AssertEvalNotUsingSystemPrincipal(nsIPrincipal*, JSContext*) /builds/worker/workspace/build/src/dom/security/nsContentSecurityManager.cpp:184
[task 2019-07-12T22:59:02.420Z] 22:59:02 INFO - GECKO(1123) | #3 0x7f49d9b328c9 in nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction(JSContext*, JS::Handle<JS::Value>) /builds/worker/workspace/build/src/caps/nsScriptSecurityManager.cpp:405:3
[task 2019-07-12T22:59:02.461Z] 22:59:02 INFO - GECKO(1123) | #4 0x7f49e4219d30 in CreateDynamicFunction(JSContext*, JS::CallArgs const&, js::GeneratorKind, js::FunctionAsyncKind) /builds/worker/workspace/build/src/js/src/vm/JSFunction.cpp:1885:8
[task 2019-07-12T22:59:02.461Z] 22:59:02 INFO - GECKO(1123) | #5 0x7f49e4218485 in js::Function(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/JSFunction.cpp:1987:10
[task 2019-07-12T22:59:02.482Z] 22:59:02 INFO - GECKO(1123) | #6 0x7f49e3e2f647 in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:448:13
[task 2019-07-12T22:59:02.483Z] 22:59:02 INFO - GECKO(1123) | #7 0x7f49e3e2f647 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:540
[task 2019-07-12T22:59:02.483Z] 22:59:02 INFO - GECKO(1123) | #8 0x7f49e3e10016 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:599:10
[task 2019-07-12T22:59:02.483Z] 22:59:02 INFO - GECKO(1123) | #9 0x7f49e3e10016 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3089
[task 2019-07-12T22:59:02.484Z] 22:59:02 INFO - GECKO(1123) | #10 0x7f49e3df9a2f in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:425:10
[task 2019-07-12T22:59:02.484Z] 22:59:02 INFO - GECKO(1123) | #11 0x7f49e3e35f0f in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:787:13
[task 2019-07-12T22:59:02.484Z] 22:59:02 INFO - GECKO(1123) | #12 0x7f49e3e366df in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:820:10
[task 2019-07-12T22:59:02.504Z] 22:59:02 INFO - GECKO(1123) | #13 0x7f49e40c1daa in ExecuteScript(JSContext*, JS::Handle<JS::StackGCVector<JSObject*, js::TempAllocPolicy> >, JS::Handle<JSScript*>, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/CompilationAndEvaluation.cpp:468:10
[task 2019-07-12T22:59:02.505Z] 22:59:02 INFO - GECKO(1123) | #14 0x7f49e40c2841 in JS::CloneAndExecuteScript(JSContext*, JS::Handle<JS::StackGCVector<JSObject*, js::TempAllocPolicy> >, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/CompilationAndEvaluation.cpp:526:10
[task 2019-07-12T22:59:02.512Z] 22:59:02 INFO - GECKO(1123) | #15 0x7f49d93e33ce in EvalScript /builds/worker/workspace/build/src/js/xpconnect/loader/mozJSSubScriptLoader.cpp
[task 2019-07-12T22:59:02.512Z] 22:59:02 INFO - GECKO(1123) | #16 0x7f49d93e33ce in mozJSSubScriptLoader::DoLoadSubScriptWithOptions(nsTSubstring<char16_t> const&, LoadSubScriptOptions&, JSContext*, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/xpconnect/loader/mozJSSubScriptLoader.cpp:476
[task 2019-07-12T22:59:02.513Z] 22:59:02 INFO - GECKO(1123) | #17 0x7f49d93e14fb in mozJSSubScriptLoader::LoadSubScript(nsTSubstring<char16_t> const&, JS::Handle<JS::Value>, JSContext*, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/xpconnect/loader/mozJSSubScriptLoader.cpp:324:10
[task 2019-07-12T22:59:02.513Z] 22:59:02 INFO - GECKO(1123) | #18 0x7f49d78f78e1 in NS_InvokeByIndex /builds/worker/workspace/build/src/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:106
[task 2019-07-12T22:59:02.529Z] 22:59:02 INFO - GECKO(1123) | #19 0x7f49d94bad6c in Invoke /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1658:10
[task 2019-07-12T22:59:02.529Z] 22:59:02 INFO - GECKO(1123) | #20 0x7f49d94bad6c in Call /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1195
[task 2019-07-12T22:59:02.529Z] 22:59:02 INFO - GECKO(1123) | #21 0x7f49d94bad6c in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1157
[task 2019-07-12T22:59:02.529Z] 22:59:02 INFO - GECKO(1123) | #22 0x7f49d94c0e7d in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:943:10
[task 2019-07-12T22:59:02.529Z] 22:59:02 INFO - GECKO(1123) | #23 0x1d71d73394c3 (<unknown module>)
[task 2019-07-12T22:59:02.530Z] 22:59:02 INFO - GECKO(1123) | AddressSanitizer can not provide additional info.

Backout: https://hg.mozilla.org/integration/autoland/rev/c31645ba75944c128689a6f0c9a84b9729bfa8c6

Flags: needinfo?(tom)

Investigating.

Flags: needinfo?(tom)

Okay I think this can stick now.

Keywords: checkin-needed

On Thu, July 18, 2019, 1:34 AM GMT+3, by archaeopteryx@coole-files.de.
Revisions: D37460 diff 133358
Details: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. applying /tmp/tmpio0X4C modules/libpref/init/all.js Hunk #1 FAILED at 2511. 1 out of 1 hunk FAILED -- saving rejects to file modules/libpref/init/all.js.rej dom/security/nsContentSecurityManager.cpp Hunk #1 FAILED at 172. 1 out of 2 hunks FAILED -- saving rejects to file dom/security/nsContentSecurityManager.cpp.rej abort: patch command failed: exited with status 256

Flags: needinfo?(tom)

Again!

Flags: needinfo?(tom)
Keywords: checkin-needed

Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eb8976f0aa75
Enable AssertEvalNotUsingSystemPrincipal on Nightly builds r=ckerschb

Keywords: checkin-needed

https://hg.mozilla.org/mozilla-central/rev/eb8976f0aa75

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox70: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: