1802511 - Principal's Subsumes and SubsumesInternal return different values

Status: RESOLVED FIXED

(Core :: Security: CAPS, task)

Description

[Olli Pettay [:smaug][[e-mail]]]

In case of about:* urls, Subsumes of two principals with same URI succeeds because
https://searchfox.org/mozilla-central/rev/fadd0a14d2a2724ee4733ef73970a2ddd457a43f/caps/BasePrincipal.h#397,401,406
returns early from FastEquals.
But if one for example creates an ExpandedPrincipal and uses subsumes() on it, https://searchfox.org/mozilla-central/rev/fadd0a14d2a2724ee4733ef73970a2ddd457a43f/caps/ExpandedPrincipal.cpp#90,112 is called, and then https://searchfox.org/mozilla-central/rev/fadd0a14d2a2724ee4733ef73970a2ddd457a43f/caps/ContentPrincipal.cpp#258 does same origin comparisons, and an about:foo isn't same origin with about:foo.
(in bug 1799935 case, about:welcome isn't same origin with another about:welcome)

This inconsistency is a bit worrisome, though in practice it might just break stuff, like about:welcome, rather than cause security issues.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

This shows up, in a bit different form, in bug 1799935. The hashtable explicitly uses FastEquals https://searchfox.org/mozilla-central/rev/fadd0a14d2a2724ee4733ef73970a2ddd457a43f/js/xpconnect/src/xpcprivate.h#584,589 but since the UAWidget code relies on expanded principals, SubsumesInternal is used https://searchfox.org/mozilla-central/rev/fadd0a14d2a2724ee4733ef73970a2ddd457a43f/caps/ExpandedPrincipal.cpp#90,112 and given that those don't match, the behavior is unexpected.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Looks like long ago there was a regression related to about:* handling https://bugzilla.mozilla.org/show_bug.cgi?id=602780

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

If I'm reading this correctly, https://bugzilla.mozilla.org/show_bug.cgi?id=1340710 changed the behavior, accidentally. It made subsumes succeed in cases where it was failing before, because it added the fast path.

Comment 4

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

Yep, it looks like the behaviour changed when we changed to using the fast-path around origin. The intention was that origin equality was equivalent to principal equality, but it turns out that we actually compared URIs for principal equality with NS_SecurityCompareURIs.

When getting down to the actual URI comparison in NS_SecurityCompareURI, the code first fetches the hosts of the URIs, and fails if it can't get them. This will instantly fail on about: URIs, as they are hostless, meaning that two identical about: URIs will not compare equal according to NS_SecurityCompareURI.

In contrast, when we build up the origin string used for FastEquals comparison, we explicitly go out of our way to support about: URIs. Other than those, we generally only support it for file URIs and standard URIs with a host.

I think we do depend on all created content principals actually being able to be serialized and then compare equal to themselves nowadays, thanks to IPC and e10s, so we can't exactly go back to the NS_SecurityCompareURI behaviour we used to have by making about: URIs no longer compare FastEquals. My inclination is to stop having Subsumes on content principals be defined in terms of NS_SecurityCompareURI, and instead define it in terms of origin strings like it is for Equals. This should be possible, as the relationship is relatively straightforward these days (I believe we could implement Subsumes() == Equals() for content principals, and it would be correct OTTOMH. Thunderbird might have some edge case though), and it ensures that we don't end up in awkward situations like this.

We could also change how NS_SecurityCompareURIs works, but TBH I'm not actually sure that we want about: URIs to compare equal to one-another with this method outside of the context of principals. It would have much broader potential implications, and I wouldn't want someone e.g. walking about:blank or about:srcdoc history entries to accidentally conclude that they're all same-origin.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b7e42143bbbc9dc3e5c05bd1e93b6485ce1d0ad4&tochange=58753259bfeb3b818eac7870871b0aae1f8de64a is the change range.
Bug 1340710 is there.

Had to tweak my test script a bit to use the older naming in those builds
(function() { let p1 = Cc["@mozilla.org/scriptsecuritymanager;1"].getService(Ci.nsIScriptSecurityManager).createCodebasePrincipal(Services.io.newURI("about:welcome"), {}); let p2 = Cc["@mozilla.org/scriptsecuritymanager;1"].getService(Ci.nsIScriptSecurityManager).createCodebasePrincipal(Services.io.newURI("about:welcome"), {}); return p1.subsumes(p2); })();

With older build that returns false, after Bug 1340710 it is true.

Comment 6

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

Adding bug 1776755 as a see-also where I mention some other jank around the ExpandedPrincipal::Subsumes method in review comments.

Comment 7

[:Gijs (he/him)]

Urgh. This also ties into bug 1228118 and bug 1767001. Perhaps aligning subsumption/equality for about: URLs more closely with the spec would help here? I agree that about:blank and about:srcdoc shouldn't be treated as equal but in theory documents with those URLs shouldn't actually end up with those origins in their principals, but with the origin of the thing that created them (or a null principal). In fact, I wonder how much would break if we just broke creating codebase principals for those specific URLs... (or made it devolve to "create a null principal").

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Btw, the failure in NS_SecurityCompareURIs happens because of https://searchfox.org/mozilla-central/rev/fadd0a14d2a2724ee4733ef73970a2ddd457a43f/netwerk/base/nsNetUtil.cpp#2471-2475, not because of asciiHost

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Aha, we have https://searchfox.org/mozilla-central/rev/d2fa2b9615dd8460b079804d0075180f689d2ba6/caps/BasePrincipal.cpp#1185-1199
That should help with the weird Thunderbird only nsIURIWithSpecialOrigin.
But there is also IS_ORIGIN_IS_FULL_SPEC_DEFINED for TB. That all makes the code ... complicated..

First we try to generate the origin using https://searchfox.org/mozilla-central/rev/d2fa2b9615dd8460b079804d0075180f689d2ba6/caps/ContentPrincipal.cpp#118 but later, if the uri implements nsIURIWithSpecialOrigin, that value isn't used at all, but instead the origin is created using that interface.
https://searchfox.org/mozilla-central/rev/d2fa2b9615dd8460b079804d0075180f689d2ba6/caps/BasePrincipal.cpp#1170,1196-1197
Just a teeny-tiny error prone and weird setup.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

If https://searchfox.org/mozilla-central/rev/028c68d5f32df54bca4cf96376f79e48dfafdf08/caps/ContentPrincipal.cpp#258 calls only FastEquals(aOther), https://searchfox.org/mozilla-central/rev/028c68d5f32df54bca4cf96376f79e48dfafdf08/browser/components/originattributes/test/browser/browser_windowOpenerRestriction.js#66-70 fails. The value is "title not set", not "pass".
It fails when false is passed here https://searchfox.org/mozilla-central/rev/028c68d5f32df54bca4cf96376f79e48dfafdf08/browser/components/originattributes/test/browser/browser_windowOpenerRestriction.js#95

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Ah, ok, SubsumesInternal isn't supposed to care about mOriginSuffix. https://searchfox.org/mozilla-central/rev/028c68d5f32df54bca4cf96376f79e48dfafdf08/caps/BasePrincipal.h#301-302 and
https://searchfox.org/mozilla-central/rev/028c68d5f32df54bca4cf96376f79e48dfafdf08/dom/base/MaybeCrossOriginObject.cpp#73,77-80 relies on that.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

And since ContentPrincipal::GenerateOriginNoSuffixFromURI fails for uris created for about:blank, meaning that those get null principals, about:blank never subsumes itself, even with FastEquals checks.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1802511, optimize ContentPrincipal::SubsumesInternal by checking FastEquals, r=nika

Comment 14

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

optimize ContentPrincipal::SubsumesInternal by checking FastEquals, r=nika
https://hg.mozilla.org/integration/autoland/rev/667f6f0af3987220655e5731014f584110415043
https://hg.mozilla.org/mozilla-central/rev/667f6f0af398