1665056 - The "security.fileuri.strict_origin_policy" pref should be latched at startup and not updated because it causes cross-process principal inconsistencies

Status: NEW

(Core :: Security: CAPS, task)

Description

[Andrew Sutherland [:asuth] (he/him)]

The "security.fileuri.strict_origin_policy" preference impacts how file URIs are represented as principals. When set to true, the principal contains the file path, when set to false, the principal is "file://UNIVERSAL_FILE_URI_ORIGIN". If the preference is desynchronized between the parent and content processes or changes over time, this can result in crashes due to security checks of principal consistency like in bug 1536596 or could result in more subtle crashes or unsafe program behavior due to logic invariants being violated due to an inconsistent perceived origin.

The preference's primary in-tree justification for existing at this time is that reftests use it for historical performance reasons as captured in bug 1511209. However, except for a few one-off tests (see searchfox search for the pref) latching the value at startup should be fine. The existing tests can be updated or, in the event of complex situations, disabled with a bug filed against the given components.

Correcting this means:

• Updating the StaticPrefList.yaml mirror value: https://searchfox.org/mozilla-central/rev/0c97a6410ff018c22e65a0cbe4e5f2ca4581b22e/modules/libpref/init/StaticPrefList.yaml#9118-9122
• Converting the dynamically tracked value to use the StaticPrefList-exposed value and related preference observing update logic.
• Fixing/disabling the tests that depend on dynamic changes.

Comment 1

[Frederik Braun [:freddy]]

Attachment: Bug 1665056 - do not change security.fileuri_strict_origin_policy at runtime r?asuth,ckerschb

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6fefdfdfe325
do not change security.fileuri_strict_origin_policy at runtime r=asuth,ckerschb

Comment 3

[amarc]

Backed out for causing mass failures

• Backout link
• Push with failures
• Failure Log
• Failure line:

Assertion failure: staticPrefValue == preferenceValue (Preference 'layers.d3d11.enable-blacklist' got modified since StaticPrefs::layers_d3d11_enable_blacklist_AtStartup was initialized. Consider using an `always` mirror kind instead), at /builds/worker/workspace/obj-build/dist/include/mozilla/StaticPrefList_layers.h:43

Comment 4

[Frederik Braun [:freddy]]

I can't see how my pref change in a security pref should change the behavior of the gfx pref at all. I am rebasing on central and pushing to try to learn more, but I feel like maybe this shouldn't have been backed out?

Comment 5

[Frederik Braun [:freddy]]

Great... We have tests files in head.js that do something like the following, which is breaking them.

loadScript("dom/quota/test/xpcshell/common/head.js");

function loadScript(path) {
  let uri = Services.io.newFileURI(do_get_file(depth + path));
  Services.scriptloader.loadSubScript(uri.spec);
}

I guess we would need to change how they are loaded

• move the head files somewhere considered "public" for xpcshell tests
• copy them into the current directory? Ugh!!
• don't use loadSubScript?

Comment 6

[Jan Varga [:janv]]

I don't think that's a problem.

Comment 7

[Jan Varga [:janv]]

Maybe there are other (easier) ways to latch the pref these days, but this is how it's done for localStorage:
https://searchfox.org/mozilla-central/rev/495dafe9d89894e4768afe6d978139351574cc84/dom/localstorage/LocalStorageCommon.h#237

Note how the value is sent to the content process as soon as possible:
https://searchfox.org/mozilla-central/rev/495dafe9d89894e4768afe6d978139351574cc84/dom/ipc/ContentParent.cpp#1639

I remember that the mirrored pref was really problematic, especially changing the value before running a test.

Comment 8

[Alex Finder]

(In reply to amarc from comment #3)

Backed out for causing mass failures

• Backout link
• Push with failures
• Failure Log
• Failure line:

Assertion failure: staticPrefValue == preferenceValue (Preference 'layers.d3d11.enable-blacklist' got modified since StaticPrefs::layers_d3d11_enable_blacklist_AtStartup was initialized. Consider using an `always` mirror kind instead), at /builds/worker/workspace/obj-build/dist/include/mozilla/StaticPrefList_layers.h:43

Perfherder has detected a browsertime performance change from push 1822b0ead8248c72cb3461924fb9ba19911e4cd2.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
65%	instagram FirstVisualChange	linux1804-64-shippable-qr	cold fission webrender	231.01 -> 380.47	Before/After
49%	instagram SpeedIndex	linux1804-64-shippable-qr	fission warm webrender	609.28 -> 909.44	Before/After
8%	instagram fcp	linux1804-64-shippable-qr	cold fission webrender	267.55 -> 289.62	Before/After
4%	instagram largestContentfulPaint	linux1804-64-shippable-qr	fission warm webrender	828.57 -> 863.00	Before/After
3%	instagram loadtime	linux1804-64-shippable-qr	fission warm webrender	408.40 -> 421.56	Before/After

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
35%	instagram ContentfulSpeedIndex	linux1804-64-shippable-qr	fission warm webrender	524.82 -> 343.47	Before/After
25%	instagram ContentfulSpeedIndex	linux1804-64-shippable-qr	cold fission webrender	1,095.16 -> 817.85	Before/After
23%	instagram FirstVisualChange	linux1804-64-shippable-qr	fission warm webrender	141.82 -> 108.89	Before/After
14%	instagram LastVisualChange	linux1804-64-shippable-qr	fission warm webrender	1,172.42 -> 1,009.61	Before/After
11%	instagram PerceptualSpeedIndex	linux1804-64-shippable-qr	fission warm webrender	607.91 -> 543.52	Before/After

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 43236

For more information on performance sheriffing please see our FAQ.

Comment 9

[Frederik Braun [:freddy]]

I'm not sure I understand the suggestions from comment 6 and comment 7.