1195976 - (CVE-2015-4508) URL spoofing in reader mode

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect)

Description

[Juho Nurminen [:jupenur]]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.89 Safari/537.36

Steps to reproduce:

Open reader mode by navigating to one of the following URLs:

about:reader?url=http://[e-mail]
about:reader?url=http://igi.tl?@mozilla.org
about:reader?url=http://igi.tl%23.mozilla.org
about:reader?url=http://igi.tl?.mozilla.org
about:reader?url=http://lab.igi.tl/redirect-to-mozilla-org.php



Actual results:

For the first four URLs, reader mode displays content from igi.tl, but highlights the URL in the URL bar as if the content was being served from mozilla.org. For the fifth URL, reader mode displays content from blog.mozilla.org as a result of an HTTP 302 redirection, but retains the original igi.tl URL in the URL bar.


Expected results:

When following redirections (assuming it's necessary in the first place), reader mode should update the displayed URL to match the one the content is actually being loaded from. When displaying the URL, highlighting should be done in a way matching the parsing that's done for the actual fetching of data. That is, when "http://[e-mail]" is parsed as "http://igi.tl/#@mozilla.org", the URL should be displayed so that "igi.tl" is highlighted with a darker font instead of "mozilla.org".

Now, as for exploiting the issue... The first four URLs listed above demonstrate a partial URL spoof, in that the real domain will always be visible, but with the wrong part of the URL highlighted. The fifth is a more "full" URL spoof, but it requires that the victim site contains an open redirection. Those are fairly common in the wild, though.

There are a couple of things that limit the impact here, quite considerably even. First off, reader mode strips all dynamic content off the page, so you can't run scripts on the spoofed page. I don't know how secure the sandboxing is, or if it's even intended to be secure, and I didn't spend a great deal of time on investigating that. Without JavaScript you can still inject text, images, and links, which are plenty to trick an unsuspecting user into handing over their online banking credentials.

Secondly, Firefox doesn't allow linking to the reader mode URL directly. You can have links pointing to it, but clicking on them doesn't do anything. There's a way around this though, if you can trick the user to bookmark the link. Dragging the link to the bookmarks toolbar or right-clicking it and selecting "Bookmark This Link" both work. When the bookmark has been created, clicking it will take you to the spoofed page just as you'd expect. This is related to bug 1185918. I'm also guessing that it would be possible to link to the page from an external application, like email client.

I've tested this on Firefox for Android 40.0 and Iceweasel 38.1.0 on Debian and Iceweasel 38.2.0 on Kali. Worth noting is that this works just as well on both mobile and desktop.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Attachment: Screenshot of the behavior

Comment 2

[Juho Nurminen [:jupenur]]

The clipboard API provides a pretty handy way of exploiting this. Combine that with the "Paste & Go" feature, and the victim might not notice anything strange. Try it here: http://lab.igi.tl/5d9019ad9e606ba48ab2/clipboard.html

Comment 3

[:Gijs (he/him)]

Attachment: Fix domain highlighting

Comment 4

[:Gijs (he/him)]

Attachment: Force URL update for redirects in readermode

Brian, Margaret's on PTO, so can you take this review?

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8653455 [details] [diff] [review]
Fix domain highlighting

Are you sure you need to replace '(.+?)' with '([^@\/]+?)'?

Please update browser/base/content/test/general/browser_urlHighlight.js for this.

But ... I'm also wondering if this is the right fix at all. E.g. typing and submitting http://igi.tl?@mozilla.org rewrites the URL to http://d.igi.tl/?@mozilla.org which works as expected. The URL we get from about:reader should probably behave the same way?

Comment 6

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #5)
> Comment on attachment 8653455 [details] [diff] [review]
> Fix domain highlighting
> 
> Are you sure you need to replace '(.+?)' with '([^@\/]+?)'?

IIRC it's not strictly necessary - I wanted to make sure that the matched domain does not contain those characters, because at some point during my attempt to patch that regex, it did. I can try without.

> Please update browser/base/content/test/general/browser_urlHighlight.js for
> this.
> 
> But ... I'm also wondering if this is the right fix at all. E.g. typing and
> submitting http://igi.tl?@mozilla.org rewrites the URL to
> http://d.igi.tl/?@mozilla.org which works as expected.

We don't "rewrite" the URL like that - it gets 302'd. The "d." bit doesn't come out of nowhere. :-)

> The URL we get from
> about:reader should probably behave the same way?

That's the second patch. The issue is that we use an XHR which you can't tell not to follow redirects. history.replaceState is broken for about: pages, so we end up having to just reload the page, which is kind of shitty and on the other hand, we shouldn't really hit this path normally anyway.

I still think the URL highlighting should be correct irrespective of us rewriting the URI, for other cases where the typedURL gets stuck in the URL bar. If we don't care about cases where the URI is invalid and left there, we should be using URI APIs instead of the hand-rolled regexpes, so I presume we do care...

Comment 7

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > But ... I'm also wondering if this is the right fix at all. E.g. typing and
> > submitting http://igi.tl?@mozilla.org rewrites the URL to
> > http://d.igi.tl/?@mozilla.org which works as expected.
> 
> We don't "rewrite" the URL like that - it gets 302'd. The "d." bit doesn't
> come out of nowhere. :-)

Yet we do rewrite / normalize URLs... For http://d.igi.tl?@mozilla.org we behave the same way, because makeURI("http://d.igi.tl?@mozilla.org").spec is "http://d.igi.tl/?@mozilla.org".

> I still think the URL highlighting should be correct irrespective of us
> rewriting the URI, for other cases where the typedURL gets stuck in the URL
> bar. If we don't care about cases where the URI is invalid and left there,

We care to some extent but this is a rather obscure edge case.

Comment 8

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #7)
> (In reply to :Gijs Kruitbosch from comment #6)
> > (In reply to Dão Gottwald [:dao] from comment #5)
> > > But ... I'm also wondering if this is the right fix at all. E.g. typing and
> > > submitting http://igi.tl?@mozilla.org rewrites the URL to 
> > > http://d.igi.tl/?@mozilla.org which works as expected.
> > 
> > We don't "rewrite" the URL like that - it gets 302'd. The "d." bit doesn't
> > come out of nowhere. :-)
> 
> Yet we do rewrite / normalize URLs... For http://d.igi.tl?@mozilla.org we
> behave the same way, because makeURI("http://d.igi.tl?@mozilla.org").spec is
> "http://d.igi.tl/?@mozilla.org".
> 
> > I still think the URL highlighting should be correct irrespective of us
> > rewriting the URI, for other cases where the typedURL gets stuck in the URL
> > bar. If we don't care about cases where the URI is invalid and left there,
> 
> We care to some extent but this is a rather obscure edge case.

I don't follow what you think the distinction is between one case where nsIURI.spec != the input, and another one. Maybe I just don't understand why we're not using nsIURI? Why is that?

If I take the corrected version of your example, stick it in the URL bar, load the page, remove the / before ?, and then focus the search bar, we mis-highlight mozilla.org as the domain. IMO we should fix that either way.

Comment 9

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #8)
> I don't follow what you think the distinction is between one case where
> nsIURI.spec != the input, and another one. Maybe I just don't understand why
> we're not using nsIURI? Why is that?

I'm not sure how exactly you'd use nsIURI. (I think I actually did it in my original Locationbar² implementation and it was a huge mess, so I ended up using the regex instead.)

> If I take the corrected version of your example, stick it in the URL bar,
> load the page, remove the / before ?, and then focus the search bar, we
> mis-highlight mozilla.org as the domain. IMO we should fix that either way.

It's a correctness bug but I'm not sure an end user would ever hit it except for trying to proof that there's a correctness bug, at which point I don't care much. There's some value to keeping the regex simple too.

Comment 10

[Brian Nicholson (:bnicholson)]

Comment on attachment 8653461 [details] [diff] [review]
Force URL update for redirects in readermode

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

Looks good to me.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8653461 [details] [diff] [review]
Force URL update for redirects in readermode

remote:   https://hg.mozilla.org/integration/fx-team/rev/1d0a9ae2715d

Comment 12

[Juho Nurminen [:jupenur]]

What's bug 1199601 and why does this depend on it? I'm not authorized to access it.

Comment 13

[:Gijs (he/him)]

(In reply to Juho Nurminen [:jupenur] from comment #12)
> What's bug 1199601 and why does this depend on it? I'm not authorized to
> access it.

I've spun off the location bar highlighting issue per some of the discussion here. The fix that just landed will address all of the testcases you posted in comment #0. But for better defense in depth it might be necessary to make changes to the highlighting as well.

I think the most problematic issue here is the cross-domain spoofing (the last link in comment #0). That's fixed on our nightly branch now (assuming the patch sticks, it should be in tomorrow's Firefox nightly). In order to make it clear what bugs are fixed where, and to track uplifts, I created a separate bug for the URL bar highlighting.

Comment 14

[Juho Nurminen [:jupenur]]

Thanks. Could you cc me on any new issues you create? I'm interested in the progress as well.

Comment 15

[:Gijs (he/him)]

(In reply to Juho Nurminen [:jupenur] from comment #14)
> Thanks. Could you cc me on any new issues you create? I'm interested in the
> progress as well.

Done.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1d0a9ae2715d

Comment 17

[:Gijs (he/him)]

Comment on attachment 8653461 [details] [diff] [review]
Force URL update for redirects in readermode

Approval Request Comment
[Feature/regressing bug #]: reader mode
[User impact if declined]: spoofing attacks
[Describe test coverage new/current, TreeHerder]: reader mode has some test coverage. There is no specific test for the functionality I'm adding here
[Risks and why]: low: the fix is specific to how we treat reader mode URLs when we download the document separately (which happens when *not* going into reader mode using the button in the URL bar but, for example, when a URL is put in manually).
[String/UUID change made/needed]: no.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Juho, would you be able to verify the fix on the latest Nightly build (8-29 or later)? Thanks.

Comment 19

[Juho Nurminen [:jupenur]]

Verified on both desktop and Android; looks good to me.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8653461 [details] [diff] [review]
Force URL update for redirects in readermode

Fix a low sec issue, taking it.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/753d9e05c8e8

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3093e8542c2c

Comment 23

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

This is not eligible for bounty as a "low" rated security issue.

Comment 24

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Marking as verified for Firefox 43 based on comment 19.

Comment 25

[:Gijs (he/him)]

QA pointed out that on beta, this fix is only half-working. Instead of redirecting inside reader mode, the user gets kicked out of reader mode. This is because bug 1182778's first patch was denied approval to land on beta, and so the error object we throw up kicks the user out of reader mode instead of redirecting them.

That is not a complete disaster by any means, but it's annoying. I'm flagging this up in case the advisory needs to change or we want to uplift the other patch and respin 41 rc. I'm having a hard time estimating impact here in terms of legitimate sites' reader mode persisting across restarts, but it's possible there would be some impact.

I'm sorry for forgetting that this patch needed the other patch in order to work (and not pushing harder+sooner for that patch to make beta). :-(

Comment 26

[Ada [:adalucinet]]

(In reply to :Gijs Kruitbosch from comment #25)
> QA pointed out that on beta, this fix is only half-working. Instead of
> redirecting inside reader mode, the user gets kicked out of reader mode.

Indeed this happens, but with 41.0 RC build 3, as pointed via IRC (comparison between latest Nightly and 41RC - http://i.imgur.com/15Qvxuk.png). The latest 42.0b1 (Build ID: 20150921151815) is verified fixed across platforms [1].

[1] Windows 7 64-bit, Mac OS X 10.10.4 & Ubuntu 12.04 32-bit

Comment 27

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

(In reply to :Gijs Kruitbosch from comment #25)

> That is not a complete disaster by any means, but it's annoying. I'm
> flagging this up in case the advisory needs to change or we want to uplift
> the other patch and respin 41 rc. I'm having a hard time estimating impact
> here in terms of legitimate sites' reader mode persisting across restarts,
> but it's possible there would be some impact.
> 

I'm seeing this two hours after we shipped. So you can talk to release management about adding this to any point release. The security issue is still effectively addressed based on your description though.

Comment 28

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Gijs, could you please nominate the patch for uplift to mozilla-release? We will most likely do a dot release for FF41 and this patch could be potentially uplifted then.

Comment 29

[:Gijs (he/him)]

(In reply to Al Billings [:abillings] from comment #27)
> (In reply to :Gijs Kruitbosch from comment #25)
> 
> > That is not a complete disaster by any means, but it's annoying. I'm
> > flagging this up in case the advisory needs to change or we want to uplift
> > the other patch and respin 41 rc. I'm having a hard time estimating impact
> > here in terms of legitimate sites' reader mode persisting across restarts,
> > but it's possible there would be some impact.
> > 
> 
> I'm seeing this two hours after we shipped. So you can talk to release
> management about adding this to any point release. The security issue is
> still effectively addressed based on your description though.

Correct.

(In reply to Ritu Kothari (:ritu) from comment #28)
> Gijs, could you please nominate the patch for uplift to mozilla-release? We
> will most likely do a dot release for FF41 and this patch could be
> potentially uplifted then.

Will do.