1776755 - (CVE-2023-32210) Logic depending on ExpandedPrincipal ordering in `Document::MaybeDowngradePrincipal` is broken

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect, P2)

Description

[Nika Layzell [:nika] (ni? for response)]

In Document::MaybeDowngradePrincipal there is logic which tries to determine properties of an expanded principal coming from a webextension content script by assuming the order of the principals in the allowList (https://searchfox.org/mozilla-central/rev/5b2d2863bd315f232a3f769f76e0eb16cdca7cb0/dom/base/Document.cpp#3016-3027). This is done under the assumption that because when the extension code creates a content script principal it always passed the content page's script before the extension's principal, it will always appear first.

Unfortunately, this is not correct. The order of principals in an expanded principal is actually determined by their origin's alphabetical order, as they are sorted when they are added to the ExpandedPrincipal (https://searchfox.org/mozilla-central/rev/5b2d2863bd315f232a3f769f76e0eb16cdca7cb0/caps/ExpandedPrincipal.cpp#56-63). This logic only works because the scheme http[s]:// orders lower than the scheme moz-extension:// when sorting, so it ends up appearing first. I worry that we may already have issues in this case for documents with a null principal, as moz-nullprincipal orders after moz-extension.

The logic in ExpandedPrincipal::PrincipalToInherit may also need to be updated, as it falls back to using mPrincipals.LastElement() in some cases, which is probably not what we want given that it's based on the alphabetical order of the principals passed in: https://searchfox.org/mozilla-central/rev/5b2d2863bd315f232a3f769f76e0eb16cdca7cb0/caps/ExpandedPrincipal.cpp#196. There's a similar situation with IsThirdPartyURI which falls back to the first principal: https://searchfox.org/mozilla-central/rev/5b2d2863bd315f232a3f769f76e0eb16cdca7cb0/caps/ExpandedPrincipal.cpp#381.

Alternatively, we could also consider removing the sorting from ExpandedPrincipal.

Comment 1

[Kris Maglione [:kmag]]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #0)

Alternatively, we could also consider removing the sorting from ExpandedPrincipal.

I think this would be my preference. The extension framework should be the only significant user of expanded principals at this point, and it always creates them with the principals in a defined order.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:ckerschb, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

FWIW, we agreed to remove the ordering/sorting.

Comment 4

[Frederik Braun [:freddy]]

Random Note while looking at other code. We definitely also have code that does the [0] array subscription thing in e.g., GetOriginHeaderPrincipal()

Comment 5

[Kris Maglione [:kmag]]

(In reply to Frederik Braun [:freddy] from comment #4)

Random Note while looking at other code. We definitely also have code that does the [0] array subscription thing in e.g., GetOriginHeaderPrincipal()

That code, at least, only runs when the list is one element long. Though I think I'd prefer it used PrincipalToInherit instead.

Comment 6

[June Wilde (she/her) [:jewilde]]

Attachment: Bug 1776755 - Skip ordering ExpandedPrincipals to speed up creation; r=nika

Comment 7

[Frederik Braun [:freddy]]

I'm adding https://bugzilla.mozilla.org/show_bug.cgi?id=1792138 as a See Also, mostly as a drive by. I have not read the bug, only skimmed it. But it does talk about principal ordering so I found it interesting and relevant enough to add :)

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed: https://hg.mozilla.org/integration/autoland/rev/2b1d90490648b844093920db3dfee5bac23d3fec

Backed out for causing xpcshell failures in test_allowedDomainsXHR.js:
https://hg.mozilla.org/integration/autoland/rev/a07ececde41ec4e582486c5d42e884888e05569d

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=EdiWLkAQTbuMYQgEVtJRAg.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=2b1d90490648b844093920db3dfee5bac23d3fec
Failure log: https://treeherder.mozilla.org/logviewer?job_id=404910319&repo=autoland

TEST-UNEXPECTED-FAIL | js/xpconnect/tests/unit/test_allowedDomainsXHR.js | run_test - [run_test : 67] "http://localhost:4444/redirect" == "http://localhost:4444/simple"

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed: https://hg.mozilla.org/integration/autoland/rev/3cdcc72baa6e01bbc3988498e62c55c3e2a08829

Backed out for causing failures at test_ext_contentscript_triggeringPrincipal.js: https://hg.mozilla.org/integration/autoland/rev/d9b6910c58edea2b47d0728f4f035fd3d853bdf7

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedJob=405614590&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunning%2Cpending%2Crunnable&revision=3cdcc72baa6e01bbc3988498e62c55c3e2a08829
Failure log: https://treeherder.mozilla.org/logviewer?job_id=405614590&repo=autoland&lineNumber=2680

[task 2023-02-14T17:13:03.842Z] 17:13:03  WARNING -  TEST-UNEXPECTED-FAIL | xpcshell-e10s.ini:toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js | test_contentscript_triggeringPrincipals - [test_contentscript_triggeringPrincipals : 967] Got expected origin for URL http://example.com/iframe.html?origin=contentScript&source=contentScript - "[Expanded Principal [http://example.com, moz-extension://d4516316-153c-446f-beeb-6f671b514b68]]" == "[Expanded Principal [moz-extension://d4516316-153c-446f-beeb-6f671b514b68, http://example.com]]"
[task 2023-02-14T17:13:03.842Z] 17:13:03     INFO -  /builds/worker/workspace/build/tests/xpcshell/tests/toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:checkChannel:967
[task 2023-02-14T17:13:03.843Z] 17:13:03     INFO -  /builds/worker/workspace/build/tests/xpcshell/tests/toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:awaitLoads/</<:986
[task 2023-02-14T17:13:03.843Z] 17:13:03     INFO -  /builds/worker/workspace/build/tests/xpcshell/head.js:_do_main:238
[task 2023-02-14T17:13:03.843Z] 17:13:03     INFO -  /builds/worker/workspace/build/tests/xpcshell/head.js:_execute_test:585

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:jewilde, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 11

[Nika Layzell [:nika] (ni? for response)]

I think the issue here was a buggy test. The code to create the expanded principals for content scripts constructs them in the order [contentPrincipal, extensionPrincipal] (https://searchfox.org/mozilla-central/rev/dcf64fc565e3749119bd57202e2ab06533155d16/toolkit/components/extensions/ExtensionContent.jsm#844,864), whereas the test constructs it in the order [extensionPrincipal, contentPrincipal] (https://searchfox.org/mozilla-central/rev/dcf64fc565e3749119bd57202e2ab06533155d16/toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js#1261). Previously these would've been equal due to the sorting happening under the hood, but nowadays we should be updating the test to match the behaviour from the actual extension code to make sure it's consistent.

Comment 12

[June Wilde (she/her) [:jewilde]]

sorry! i was out on pto. i've updated all the failing tests I could find and I'll attempt another landing tomorrow after the code freeze lifts

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3f8c82367769

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jewilde, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox112 to wontfix.

For more information, please visit auto_nag documentation.

Comment 15

[Tyson Smith [:tsmith]]

Attachment: advisory.txt