1309310 - (CVE-2017-5391) Unlinkable content-privileged about: pages can link to chrome-privileged about: pages

Status: VERIFIED FIXED

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

Description

[:Gijs (he/him)]

As per summary. This also allows loading, say, about:preferences in an iframe on about:feeds or about:reader or whatever. Compartments save our butt from immediate pwnage, but I'm fairly sure that chrome-privileged things in non-toplevel documents is bad either way.

This is a regression (it wasn't possible in 47), probably from bug 1253673. Not marking for now because we don't know for sure and I don't yet know exactly how bad this is. Just that it's bad.

Comment 1

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #0)
> This is a regression (it wasn't possible in 47), probably from bug 1253673.

Oh, to be clear: it was a regression for about:feeds, which wasn't unlinkable before that bug. I haven't checked how this affects/affected pages that were unlinkable before, like about:reader.

Comment 2

[Jerri Rice (rehash pending)]

(In reply to :Gijs Kruitbosch from comment #1)
> (In reply to :Gijs Kruitbosch from comment #0)
> > This is a regression (it wasn't possible in 47), probably from bug 1253673.
> 
> Oh, to be clear: it was a regression for about:feeds, which wasn't
> unlinkable before that bug. I haven't checked how this affects/affected
> pages that were unlinkable before, like about:reader.

Even when about:feeds wasn't linkable, the meta redirect technique would still load it( I remember testing this in release when there was no other way for me to load content for about:feeds.)

Should that be a different bug if I can confirm that it does work before the regression seen here?

Comment 3

[:Gijs (he/him)]

(In reply to Jerri Rice (rehash pending) from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > (In reply to :Gijs Kruitbosch from comment #0)
> > > This is a regression (it wasn't possible in 47), probably from bug 1253673.
> > 
> > Oh, to be clear: it was a regression for about:feeds, which wasn't
> > unlinkable before that bug. I haven't checked how this affects/affected
> > pages that were unlinkable before, like about:reader.
> 
> Even when about:feeds wasn't linkable, the meta redirect technique would
> still load it( I remember testing this in release when there was no other
> way for me to load content for about:feeds.)
> 
> Should that be a different bug if I can confirm that it does work before the
> regression seen here?

I'm not sure what "the meta redirect technique" means in this context. about:feeds is currently 'unlinkable', so if you try and do e.g. location.href = 'about:feeds' on the web, you get an error. That didn't use to be the case (pre-firefox-48).

Comment 4

[Jerri Rice (rehash pending)]

Attachment: metaRedirection-testcase-new.html

Loading about:feeds in ff 47 which shows that even without this regression the possibility for this being used maliciously is very real.

Comment 5

[:Gijs (he/him)]

(In reply to Jerri Rice (rehash pending) from comment #4)
> Created attachment 8799934 [details]
> metaRedirection-testcase-new.html
> 
> Loading about:feeds in ff 47 which shows that even without this regression
> the possibility for this being used maliciously is very real.

This doesn't make a difference for this bug - this bug is about loading about:preferences or other privileged about: pages from unprivileged pages like about:feeds . It is orthogonal to whether or not you can load about:feeds, but it was likely regressed by the change in bug 1253673.

As I said in comment #3, in 47 you could load about:feeds directly with a link or assigning to location.href . That is no longer possible. Your redirect works in 52 as well as 47, but that doesn't affect about:preferences . The problem is loading about:preferences *from* about:feeds .

Comment 6

[Jerri Rice (rehash pending)]

Yes, I had realized that after testing that final piece, made a comment pointing out my mistake here only to hit a mid-air collision! 

Gijs is on my clock apparently, that's about the 4th one today :)

Comment 7

[Andrew Overholt [:overholt]]

Gijs, are you going to be able to take this or do you want me to try to find someone? And just to be clear: your thoughts are that we should/should be able to restrict what about:feeds, etc. can load with code in CAPS somewhere? (/me isn't super-familiar with this) Thanks.

Comment 8

[:Gijs (he/him)]

(In reply to Andrew Overholt [:overholt] from comment #7)
> Gijs, are you going to be able to take this or do you want me to try to find
> someone?

I feel bad because it's my regression, but I think it would be good if there was someone else with cycles. If not, ping me again and I can try to find time for this.

Note that this might not be entirely trivial to fix...

> And just to be clear: your thoughts are that we should/should be
> able to restrict what about:feeds, etc. can load with code in CAPS
> somewhere? (/me isn't super-familiar with this) Thanks.

Well, I haven't deep-dived on this yet, but there's basically 3 sets of pages here:

(a) about: pages with system principal (which we try always try to prevent the web from opening)
(b) about: pages without system principal that we try to prevent the web from opening (are "unlinkable")
(c) about: pages without system principal that can easily be opened by the web (e.g. about:blank)

(a) and now (b) are not nested URIs, they're just "about:foo". (c) *are* nested, they are about:foo on the outside, moz-safe-about:foo on the inside).

CAPS has flag checks on URIs where it basically checks whether one URI can open the other based on the flags set on the target URL (like "this URI is a UI resource" or "this URI should not be loaded by web content"). However, we purposefully don't do some of those flag checks when the source and target URI have the same scheme. This means chrome:// URLs can always link to other chrome:// URLs, http:// can always link to other http://, etc.

The nesting difference means that pages in category (c) can't open pages in (b) and (a), because their URI schemes are not the same (the outer scheme is the same (about), but one of them is nested and the other isn't, so we conclude the two URIs aren't scheme-equivalent).

The bug here is that I would argue that pages in (b) also shouldn't be able to open pages in (a).

I don't know for sure what would be the simplest way of adapting CAPS so this is no longer possible. The obvious options include manually special-casing about: URIs somehow, or adding a flag that we set on the about: protocol that we check for in the CAPS code in question, so that we don't allow loads based on scheme-equivalence for non-nested about: URIs. That last one sounds like the most plausible, much though I would hate to add yet another URI flag to nsIProtocolHandler. Boris, does that sound sane?


Note that as of right now, AFAIK there is no obvious way to exploit this. If there was a way to run script or create links in about:feeds, that would change.

Comment 9

[:Gijs (he/him)]

Oh, also, this problem is just going to get worse once we de-nest about: URIs (which we should be doing at some point - bug 1228118).

Comment 11

[Boris Zbarsky [:bzbarsky]]

Sorry for the lag here....

> I don't know for sure what would be the simplest way of adapting CAPS so this is no longer possible

Probably creating a new protocol handler flag and skipping the "allow same-protocol links" if it's set is best.  We _could_ just hard-code "about:", but I think the flexibility of the protocol flag is much more extensible...

Comment 12

[Jerri Rice (rehash pending)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #11)
> Sorry for the lag here....
> 
> > I don't know for sure what would be the simplest way of adapting CAPS so this is no longer possible
> 
> Probably creating a new protocol handler flag and skipping the "allow
> same-protocol links" if it's set is best.  We _could_ just hard-code
> "about:", but I think the flexibility of the protocol flag is much more
> extensible...

I *may* try to implement this a little later today.  My net access is limited and if I do get something together I'll dig up try server access and push the patch.  Anyone else who may pick this up please do because I have very limited net access as of late and it's hard for me to get sufficient time to focus on work.

Comment 13

[:Gijs (he/him)]

Attachment: Patch v0.1

So something like this? Tested by adding a link to about:preferences on about:mozilla, clicking that gets me a security error.

I haven't pushed to try yet... not sure if that's worth it or if I should just land and hope things don't break, given that it's a sec bug. Although I realize this is a sec-moderate, I'm worried that we don't know the exact impact of this bug, and the impact of disclosure-by-patch. But maybe I'm overly paranoid?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8809091 [details] [diff] [review]
Patch v0.1

One upshot here is that a not-publicly-linkable but not system privileged about: URI will no longer be able to link to _itself_, right?  This might in fact cause some breakage.  Maybe we should allow the link even if denySameSchemeLinks if currentURI and currentOtherURI are equalsIgnoringRef or something?

Past that, I'm not sure about try vs just landing; that's something worth checking with the sec-approval folks.

Comment 15

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #14)
> Comment on attachment 8809091 [details] [diff] [review]
> Patch v0.1
> 
> One upshot here is that a not-publicly-linkable but not system privileged
> about: URI will no longer be able to link to _itself_, right?  This might in
> fact cause some breakage. 

Itself, yes, I suppose so. I'm not sure any pages exists that actually need to be able to link to themselves... We could also potentially make only the system-privileged be non-same-scheme-linkable by modifying the flags per URI, which would avoid having to put this logic into caps. That said, that would then allow e.g. about:mozilla and about:reader to link to each other (neither is outside-linkable nor privileged). That might be OK given that neither is privileged - how would you feel about that?

> Maybe we should allow the link even if
> denySameSchemeLinks if currentURI and currentOtherURI are equalsIgnoringRef
> or something?

Indeed, that would also make sense to me.

> Past that, I'm not sure about try vs just landing; that's something worth
> checking with the sec-approval folks.

Dan, thoughts on this?

Comment 16

[Boris Zbarsky [:bzbarsky]]

> I'm not sure any pages exists that actually need to be able to link to themselves

Anything that uses self-embedded SVG or CSS element() or whatnot?  (Maybe there isn't, of course.)

> That might be OK given that neither is privileged - how would you feel about that?

It doesn't seem completely unreasonable, but I think the equalsIgnoringRef suggestion allows fewer things and should be enough in practical terms.

Comment 17

[:Gijs (he/him)]

Attachment: Patch v0.2

Like this? This seems to wfm (tested by opening about:mozilla, and in the web console running location.href = 'about:mozilla', which failed on the previous patch, and succeeds with these changes).

Comment 18

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #16)
> > I'm not sure any pages exists that actually need to be able to link to themselves
> 
> Anything that uses self-embedded SVG or CSS element() or whatnot?  (Maybe
> there isn't, of course.)

I don't think there is, but you're right that having that break would be confusing.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8809380 [details] [diff] [review]
Patch v0.2

r=me

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fb5ae665310e
https://hg.mozilla.org/mozilla-central/rev/385cf64debf7

Comment 21

[:Gijs (he/him)]

I ended up just landing this. Ni me to get this uplifted into 51 if there's no reports of terrible breakage in the next week.

Comment 22

[Honza Bambas (:mayhemer)]

Causes bug 1318664.

Comment 23

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #21)
> I ended up just landing this. Ni me to get this uplifted into 51 if there's
> no reports of terrible breakage in the next week.

Not doing this until bug 1318664 is fixed.

Comment 24

[:Gijs (he/him)]

Attachment: Patch for beta

Approval Request Comment
[Feature/Bug causing the regression]: bug 1253673
[User impact if declined]: a failure in one of our defense-in-depth security mechanisms.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: by automated tests, yes, but not manually
[Needs manual test from QE? If yes, steps to reproduce]: 
Maybe. Here's a quick in-product test that I just came up with (which verifies this is indeed fixed on aurora, and still busted on beta):

1. open about:mozilla
2. open the developer tools console (ctrl-shift-k on Windows/Linux)
3. paste in:

i = document.createElement("iframe"); i.src = "about:preferences"; document.body.appendChild(i); 

4. execute that code

Expected: the iframe (rectangle) that gets added at the bottom of the page is empty-ish
Actual: the iframe loads the Firefox preferences page.

[List of other uplifts needed for the feature/fix]: this patch includes the fix for bug 1318664, which was a regression caused by this patch. (Note for people using this: this is a patch file against beta containing 2 separate csets that was generated by using 'hg export' with multiple revs. "hg import" should be able to cope.)
[Is the change risky?]: not very.
[Why is the change risky/not risky?]: there are pretty thorough tests, we found and fixed the 1 regression this caused (and expanded the test coverage to ensure we don't break that code again) and no other regressions have been reported (that I'm aware of) in the 4 weeks that this has now been present on trunk + aurora.
[String changes made/needed]: nope

Comment 25

[Gerry Chang [:gchang]]

Comment on attachment 8818531 [details] [diff] [review]
Patch for beta

Fix a sec-moderate. Beta51+. Should be in 51 beta 8.

Comment 26

[Gerry Chang [:gchang]]

Hi Florin,
Can you help find someone in your team to verify this? Thanks.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/61d9cd6239b2a1eb6ec3fd16f0a4ffe57c6ecf73

Comment 28

[Alexandru Simonca [:asimonca], Release Desktop QA]

This is Verified Fixed using the str from comment #24 in Firefox Beta 51.0b8 and Firefox Aurora 52.0a2 (id: 20161216004017) on a Windows 10 x64 machine.