1147597 - (CVE-2015-0798) Privileged URLs processed by about:reader

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect)

Description

[Armin Ebert [:arminius]]

The about:reader page can be opened from an unprivileged context, accepting any protocol in the "url" parameter. If parsing of the target document fails, the browser will eventually redirect to the specified URL.

Therefore, this link will redirect to the XUL document without any user interaction:

about:reader?url=chrome://browser/content/browser.xul

If a javascript: link is given, the script executes when the user tries to close the reader view (navigate back to the "original page"):

about:reader?url=javascript:alert(document.location)

The JS payload then evaluates in the about:reader context (which does not have chrome privileges).

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Seems like we should whitelist http[s] here?

Comment 2

[:Margaret Leibovic]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #1)
> Seems like we should whitelist http[s] here?

We only try parsing http/https/file URIs:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm?force=1#231

But yeah, what you're seeing here is that if we fail to parse the URI, we load the original URL that was passed in:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/AboutReader.jsm?force=1#576

about:reader runs with content privileges, but AboutReader.jsm has chrome privileges, so we do have to be more careful.

A scheme check sounds like the right call to me.

(FYI, this issue also affects mobile, and is something we ship right now.)

Comment 3

[Armin Ebert [:arminius]]

> We only try parsing http/https/file URIs:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/
> ReaderMode.jsm?force=1#231

Isn't allowing the file: scheme already a SOP violation?

Also, relying on a protocol whitelist might still produce some security-relevant bugs. For instance, the malware/phishing warning will be bypassed (on desktop):

about:reader?url=http://www.itisatrap.org/firefox/its-an-attack.html

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Margaret Leibovic from comment #2)
> But yeah, what you're seeing here is that if we fail to parse the URI, we
> load the original URL that was passed in:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/
> AboutReader.jsm?force=1#576

Yeah, we need a scheme check there. There's no valid use case for reader-ing non-http schemes, AFAIK.

Is Firefox 37 affected by this?

Comment 5

[Daniel Veditz [:dveditz]]

(In reply to Armin Razmdjou from comment #3)
> Isn't allowing the file: scheme already a SOP violation?

It's only a "same origin" violation if a web page can get at the contents of a file:// window or frame. We prevent file:// loads mostly so that in case we have a SOP-violation bug attackers can't easily use it to steal sensitive files off your disk.

While we don't want to allow a _web_ page to specify a file: scheme I can see it would be useful for reader mode to support it to read articles that have been downloaded locally. I don't know if that's why we allow it here, though. I'd be more than happy to give up that (uncommon?) case if necessary to stop web pages from being able to trigger file: loads.

(In reply to :Margaret Leibovic from comment #2)
> A scheme check sounds like the right call to me.

A scheme check for loading should usually be the last choice because it scatters security decisions across the tree and they can easily end up inconsistent with each other. Any chance you could use nsIScriptSecurityManager::CheckLoadURIWithPrincipal() (or CheckLoadURIStrWithPrincipal)?

Comment 6

[:Margaret Leibovic]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #4)
> (In reply to :Margaret Leibovic from comment #2)
> > But yeah, what you're seeing here is that if we fail to parse the URI, we
> > load the original URL that was passed in:
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/
> > AboutReader.jsm?force=1#576
> 
> Yeah, we need a scheme check there. There's no valid use case for reader-ing
> non-http schemes, AFAIK.
>
> Is Firefox 37 affected by this?

Yes, we added this in bug 1044781.

It's probably too late to try to uplift this to 36, but we'd definitely want the fix in 37.

Comment 7

[:Margaret Leibovic]

Attachment: Add a security scheme check before loading URLs from about:reader

We'll also need to uplift bug 1147122 if we want the error message to appear, but I don't think that's a strict requirement for fixing this issue in 37 for mobile (bug 1147122 will be uplifted to 38).

Comment 8

[:Margaret Leibovic]

Comment on attachment 8584148 [details] [diff] [review]
Add a security scheme check before loading URLs from about:reader

Review of attachment 8584148 [details] [diff] [review]:
-----------------------------------------------------------------

bnicholson could also review this.

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 8584148 [details] [diff] [review]
Add a security scheme check before loading URLs from about:reader

Review of attachment 8584148 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK to me.

::: toolkit/components/reader/AboutReader.jsm
@@ +769,5 @@
> +      Services.scriptSecurityManager.checkLoadURIWithPrincipal(principal, uri, flags);
> +    } catch (e) {
> +      this._showError();
> +      Cu.reportError("about:reader doesn't have permission to open specified URL");
> +      return false;

These return values aren't used anywhere, and you don't return true below, so should these be void returns?

Comment 10

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #9)

> ::: toolkit/components/reader/AboutReader.jsm
> @@ +769,5 @@
> > +      Services.scriptSecurityManager.checkLoadURIWithPrincipal(principal, uri, flags);
> > +    } catch (e) {
> > +      this._showError();
> > +      Cu.reportError("about:reader doesn't have permission to open specified URL");
> > +      return false;
> 
> These return values aren't used anywhere, and you don't return true below,
> so should these be void returns?

Yeah, good call, that was just my copy/pasta.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8584148 [details] [diff] [review]
Add a security scheme check before loading URLs from about:reader

I think ideally we would error out much earlier (e.g. in the "what URL am I handling?" code), so that we don't even do anything to process non-http[s] URIs at all, rather than isolating the restriction to _loadOriginalUrl.

I think we should be more restrictive than checkLoadURI and just check for schemeIs(http) || schemeIs(https).

If we do that we may need to make matching adjustments to the "when to show reader view button" to ensure we don't offer it when we know it won't work.

Comment 12

[:Margaret Leibovic]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #11)
> Comment on attachment 8584148 [details] [diff] [review]
> Add a security scheme check before loading URLs from about:reader
> 
> I think ideally we would error out much earlier (e.g. in the "what URL am I
> handling?" code), so that we don't even do anything to process non-http[s]
> URIs at all, rather than isolating the restriction to _loadOriginalUrl.

This doesn't totally work, since we would have to take extra measures to hide/disable the "close reader view" button. I also realized that as long as we show an about:reader page for one of these unsupported URLs, the reader toolbar button is also susceptible to this problem (the close reader view logic for that button is similar, but implemented elsewhere).

> I think we should be more restrictive than checkLoadURI and just check for
> schemeIs(http) || schemeIs(https).
>
> If we do that we may need to make matching adjustments to the "when to show
> reader view button" to ensure we don't offer it when we know it won't work.

We already restrict this to http/https/file URIs, but we can remove support for file URIs, since I assume that was just added for local testing, but it's easy enough to run a local web server to test.

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

Unless this is deemed a stop ship bug, it's too late for 37. We have already built and qualified the RC. Can a fix wait for 38?

Comment 14

[:Margaret Leibovic]

Attachment: Alternate patch: just bail if about:reader is loaded with a bogus URL

There is a lot of distributed logic, especially in Fennec, that makes assumptions about things if we're currently on an about:reader page. To avoid needing to add more logic to fix this all over the place, I think we should just get away from the about:reader page if we have a bad URL.

What do you think of this approach?

Comment 15

[:Margaret Leibovic]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #13)
> Unless this is deemed a stop ship bug, it's too late for 37. We have already
> built and qualified the RC. Can a fix wait for 38?

Ah, I didn't think through where we were in the release process, it's fine to wait. But if we end up with a re-spin, I think this could be a possible ride-along.

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

I'm tracking for 37 as a potential ride along for a point release.

Comment 17

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/b995c29a5a2b

What do I need to do to make this non-linkable? I think we should also to that to avoid the phishing problems.

Comment 18

[Daniel Veditz [:dveditz]]

Although this is not directly exploitable I'm going to go ahead and call this sec-high rather than the usual sec-moderate because it's one SOP violation or client UI XSS from from a sec-critical code exploit. For example, this could have been a stand-in for bug 1144991 in Mlynski's two-bug Pwn2Own exploit.

Comment 19

[:Margaret Leibovic]

Comment on attachment 8584226 [details] [diff] [review]
Alternate patch: just bail if about:reader is loaded with a bogus URL

Approval Request Comment
[Feature/regressing bug #]: bug 1044781 for mobile (Fx 35), enabling reader mode for desktop (Fx 38)
[User impact if declined]: potential security exploit
[Describe test coverage new/current, TreeHerder]: tested locally, landed on fx-team
[Risks and why]: low-risk, redirects about:reader if passed a bogus URL
[String/UUID change made/needed]: none

Comment 20

[:Margaret Leibovic]

I'm going to leave this open until we land a fix to make this non-linkable, since even with this fix a user could click on an about:reader link that will open a malicious http/https URL.

Comment 21

[:Gijs (he/him)]

(In reply to :Margaret Leibovic from comment #20)
> I'm going to leave this open until we land a fix to make this non-linkable,
> since even with this fix a user could click on an about:reader link that
> will open a malicious http/https URL.

I'm confused, users can click on links that directly open a malicious http/https URL, too, right? Can you provide more detail about what attack vector you're talking about?

Comment 22

[Carsten Book [:Tomcat]]

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

Comment 23

[:Margaret Leibovic]

(In reply to :Gijs Kruitbosch from comment #21)
> (In reply to :Margaret Leibovic from comment #20)
> > I'm going to leave this open until we land a fix to make this non-linkable,
> > since even with this fix a user could click on an about:reader link that
> > will open a malicious http/https URL.
> 
> I'm confused, users can click on links that directly open a malicious
> http/https URL, too, right? Can you provide more detail about what attack
> vector you're talking about?

I didn't test this myself, but as I interpreted the comments above, opening a malicious link through about:reader this way would avoid the safe browsing service, which would be more dangerous than a user clicking that malicious URL directly. Is that correct?

Is may also be worth considering the fact that it's hard to tell what URL is going to be loaded when clicking a malicious about:reader URL, so it may be harder for users to realize that they don't actually trust the link (although that assumes users are actually paying attention to URLs).

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Making about:reader unlinkable is a belt-and-suspenders fix to prevent other possible abuses of the privileged nature of ReaderMode.jsm. There's no valid use case for people linking to about:reader URIs, so it makes sense to just prevent it.

Comment 25

[:Gijs (he/him)]

Gavin/Margaret: thanks for clarifying!

(In reply to :Margaret Leibovic from comment #17)
> https://hg.mozilla.org/integration/fx-team/rev/b995c29a5a2b
> 
> What do I need to do to make this non-linkable? I think we should also to
> that to avoid the phishing problems.

Gavin, sounds like we still need to know this? :-)

Comment 26

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Margaret Leibovic from comment #17)
> What do I need to do to make this non-linkable? I think we should also to
> that to avoid the phishing problems.

Remove URI_SAFE_FOR_UNTRUSTED_CONTENT from https://hg.mozilla.org/mozilla-central/annotate/da2f28836843/browser/components/about/AboutRedirector.cpp#l121.

Except then you'll run into the issue described in https://hg.mozilla.org/mozilla-central/annotate/da2f28836843/browser/components/about/AboutRedirector.cpp#l27.

We should probably fix that, which expands the scope, and probably requires some changes to nsAboutProtocolHandler.cpp's newURI logic and adding another flag to nsIAboutModule.

Comment 27

[:Margaret Leibovic]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #26)
> (In reply to :Margaret Leibovic from comment #17)
> > What do I need to do to make this non-linkable? I think we should also to
> > that to avoid the phishing problems.
> 
> Remove URI_SAFE_FOR_UNTRUSTED_CONTENT from
> https://hg.mozilla.org/mozilla-central/annotate/da2f28836843/browser/
> components/about/AboutRedirector.cpp#l121.
> 
> Except then you'll run into the issue described in
> https://hg.mozilla.org/mozilla-central/annotate/da2f28836843/browser/
> components/about/AboutRedirector.cpp#l27.
> 
> We should probably fix that, which expands the scope, and probably requires
> some changes to nsAboutProtocolHandler.cpp's newURI logic and adding another
> flag to nsIAboutModule.

Yes, I think we would prefer this be linkable than be privileged (bug 778582). It's unfortunate if there's no simple way to do this right now :(

Maybe we should try to have this redirection happen in our unprivleged aboutReader.js instead of in the privileged AboutReader.jsm? I think it would be possible to do that by listening for/firing a special event on the page. Would that ensure the safebrowsing service takes effect?

Comment 28

[:Gijs (he/him)]

(In reply to :Margaret Leibovic from comment #27)
> (In reply to :Gavin Sharp [email: [e-mail]] from comment #26)
> > (In reply to :Margaret Leibovic from comment #17)
> > > What do I need to do to make this non-linkable? I think we should also to
> > > that to avoid the phishing problems.
> > 
> > Remove URI_SAFE_FOR_UNTRUSTED_CONTENT from
> > https://hg.mozilla.org/mozilla-central/annotate/da2f28836843/browser/
> > components/about/AboutRedirector.cpp#l121.
> > 
> > Except then you'll run into the issue described in
> > https://hg.mozilla.org/mozilla-central/annotate/da2f28836843/browser/
> > components/about/AboutRedirector.cpp#l27.
> > 
> > We should probably fix that, which expands the scope, and probably requires
> > some changes to nsAboutProtocolHandler.cpp's newURI logic and adding another
> > flag to nsIAboutModule.
> 
> Yes, I think we would prefer this be linkable than be privileged (bug
> 778582). It's unfortunate if there's no simple way to do this right now :(
> 
> Maybe we should try to have this redirection happen in our unprivleged
> aboutReader.js instead of in the privileged AboutReader.jsm? I think it
> would be possible to do that by listening for/firing a special event on the
> page. Would that ensure the safebrowsing service takes effect?

Considering the comment Gavin linked to already predicts that we'd run into this issue, I think I'd prefer to fix it there, but we should probably do this in a separate bug? Seems like we could essentially add a flag to mean "don't let content link to this" and add it to about:reader and file a bug investigating if any other content-safe pages want it - shouldn't be toooo hard? We normally take interface changes for aurora, and considering this is the first week of beta we might be able to take it there? We might also not need to break binary compat (if we're only adding a flag, I think that shouldn't affect things?).

Comment 29

[Daniel Veditz [:dveditz]]

(In reply to :Margaret Leibovic from comment #27)
> Yes, I think we would prefer this be linkable than be privileged (bug 778582).

Yes, absolutely!

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a59901a8d5ea

Setting the status to fixed for tracking purposes. Feel free to set it back to affected if/when you have something else to uplift down the road.

Comment 31

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gijs Kruitbosch from comment #28)
> Considering the comment Gavin linked to already predicts that we'd run into
> this issue, I think I'd prefer to fix it there, but we should probably do
> this in a separate bug? Seems like we could essentially add a flag to mean
> "don't let content link to this" and add it to about:reader and file a bug
> investigating if any other content-safe pages want it - shouldn't be toooo
> hard? We normally take interface changes for aurora, and considering this is
> the first week of beta we might be able to take it there? We might also not
> need to break binary compat (if we're only adding a flag, I think that
> shouldn't affect things?).

That sounds like the right plan. Adding a flag has no binary compat impact.

Comment 32

[:Margaret Leibovic]

Comment on attachment 8584226 [details] [diff] [review]
Alternate patch: just bail if about:reader is loaded with a bogus URL

Approval Request Comment
[Feature/regressing bug #]: bug 1044781 for mobile (Fx 35)
[User impact if declined]: potential security exploit
[Describe test coverage new/current, TreeHerder]: tested locally, landed on fx-team
[Risks and why]: low-risk, redirects about:reader if passed a bogus URL
[String/UUID change made/needed]: none

Note: this patch doesn't apply to release. I'll make a patch that applies and land it myself.

Comment 33

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8584226 [details] [diff] [review]
Alternate patch: just bail if about:reader is loaded with a bogus URL

We're going to take this as a ride along in 37.0.1. My approval for release should be carried over to the branch patch that Margaret is working on now. Release+

Comment 34

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-release/rev/833c11cfceec

Comment 35

[Al Billings [:abillings - ex-MoCo]]

This doesn't affect any released version of *Desktop* Firefox, right?

Comment 36

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Al Billings [:abillings] from comment #35)
> This doesn't affect any released version of *Desktop* Firefox, right?

That's correct, desktop Firefox won't have reader mode enabled until 37.1.

Comment 37

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #36)
> (In reply to Al Billings [:abillings] from comment #35)
> > This doesn't affect any released version of *Desktop* Firefox, right?
> 
> That's correct, desktop Firefox won't have reader mode enabled until 37.1.

* 38.1 .

Comment 38

[:Margaret Leibovic]

I filed bug 1150862 as a follow-up for making about:reader un-linkable, so we can close this bug.

Comment 39

[Ioana Chiorean]

Verified this fixed with the url from the description and confirmed with Margaret on IRC that what we got it is the intended behavior. 

Firefox for Android 37.0.1
Nexus 7 ( Android 5.0.1)

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e4566e5991e8