714631 - (CVE-2012-0479) Redirect to an HTTPS resource that returns an Atom/RSS content type with an invalid body causes the site identity block to show the resource's identity for the source's content

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Jeroen van der Gun]

Attachment: Proof of concept

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:9.0.1) Gecko/20100101 Firefox/9.0.1
Build ID: 20111220165912

Steps to reproduce:

I created a test phishing page. At the bottom of it, I put a small piece of JavaScript that redirects to an HTTPS Twitter API page that replies with an HTTP error code and XML content. I then opened the test phishing page in Firefox.

(The exact HTTP error code does not seem to matter; it works at least with 404 and 500. On the other hand, the XML content in the HTTP error body seems to be necessary.)


Actual results:

The certificate of the HTTPS error page is now shown on my phishing page.


Expected results:

The error from Twitter should be displayed and the address in the address bar should indicate the address of the error page.

Comment 1

[Jeroen van der Gun]

Attachment: Screenshots from the proof of concept

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Confirmed. Just click the attachment and wait.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This happens even with Firefox 3.6.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Given location.href='https://example.org/http404error.xml'

where http404error.xml is an XML document served with an HTTP 404 response code.

* Internet Explorer will show its 404 error page.
* Chrome will display the source of the XML document.
* Firefox won't load the XML document, and stays on the current page, but it changes the site identity block to be the one for "https://example.org".

It seems like we start updating the site identity block while we are loading the target of a navigation, but before we are committed to doing that navigation. That seems wrong to me. We should update the site identity block when we actually navigate away from the current page and when we actually navigate *to* the target page. It seems like we update the address in the address bar correctly in this case; perhaps we "just" need to change things so that the site identity block gets updated (only) at the same time the address bar does. (This makes sense, because the site identity block is part of the address bar, as far as users can see.)

Comment 5

[Jeroen van der Gun]

Just a quick note: the bug also occurs if the user performs the redirect manually by entering https://example.org/http404error.xml in the address bar (instead of by JavaScript as in the phishing example).

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Jeroen van der Gun from comment #5)
> Just a quick note: the bug also occurs if the user performs the redirect
> manually by entering https://example.org/http404error.xml in the address bar
> (instead of by JavaScript as in the phishing example).

Yes. For example, try pasting https://twitter.com/statuses/user_timeline.atom?screen_name=TriggerHTTPError into the address bar when on this bug's page. The site identity block will change to match the URL but the browser remains on this page. See the screenshot I am about to attach.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Screenshot showing address bar and site identity block not matching content.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

See the blog post here, regarding similar issues in Google Chrome:
http://blog.acrossecurity.com/2012/01/google-chrome-https-address-bar.html

Comment 10

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

For this load, the front end code gets an onSecurityChange (update security indicators), but not an onLocationChange (update URL bar).

Comment 11

[Boris Zbarsky [:bzbarsky]]

Gavin, is this specific to the https page involved having the "application/atom+xml" type (and empty data), or will any type we don't handle internally do?

Comment 12

[Boris Zbarsky [:bzbarsky]]

I'm guessing the latter, actually, and in particular that we have to hit the block at http://hg.mozilla.org/mozilla-central/annotate/964b118ac852/uriloader/base/nsURILoader.cpp#l528

If that happens, we in fact leave the old page in place; we shouldn't be sending onSecurityChange in that situation.

Comment 13

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

The onSecurityChange in question is fired from:
http://mxr.mozilla.org/mozilla-central/source/security/manager/boot/src/nsSecureBrowserUIImpl.cpp?rev=f18be484cb5a#1225

Comment 14

[Boris Zbarsky [:bzbarsky]]

Hrm.  Why is requestHasTransferedData true?

Comment 15

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

Short answer is "because nsSecureBrowserIUImpl previously received a STATE_TRANSFERRING & STATE_IS_REQUEST notification for that request" (i.e. it was inserted into mTransferringRequests, which means requestHasTransferedData is set to true when STATE_STOP & STATE_IS_REQUEST is fired for that request).

Comment 16

[Boris Zbarsky [:bzbarsky]]

Well, sure.  The question is why that happens, if the channel is canceled from OnStartRequest....

Comment 18

[Jeroen van der Gun]

Attachment: Proof of concept v2

I think you're looking at the wrong piece of code. I did some further research and discovered that if a generic XML mime type is used in the response header, the page loading is correctly aborted. So the code referenced by comment 12 seems to be secure.

The bug appears to be specific to the Atom and RSS mime types. It should be noted that Twitter gives an XML response that is well-formed XML but not a feed, while it does use a feed mime type. My theory would be that the Atom/RSS handler fails in an insecure way, because the feed is invalid. The HTTP response code might have nothing to do with this bug after all.

Comment 19

[Jeroen van der Gun]

Addition to comment 18: setting Firefox to automatically use an external feed reader does not solve the bug.

Comment 20

[Boris Zbarsky [:bzbarsky]]

OK, if this is specific to the Atom and RSS mime types things make more sense.

So what's happening is that we're landing in the URILoader but the feed sniffer has already sniffed the type to application/vnd.mozilla.maybe.feed.  We look for a stream converter from that type to */* and find a JS-implemented converter; I believe this is FeedConverter.js.  We start an async convert, call onStartRequest on the converter  and return from our original OnStartRequest.  The feed converter "converts" the load to a load woth an Atom type.  We try to go back through URILoader dispatch for this, but we still have no way to handle it, so we go to hand it off to a helper app. But before doing that we check for an error page, and if we have one bail out.

Now the key part for this last bit is that we _do_ have an error page in this case, and by the time we bail out this is the post-converter dispatch running, so we have in fact transferred data.

The fact that the data is not actually a feed _may_ have to do with the fact that we don't find a handler for the feed type.  Maybe.

In any case, the upshot is that data has been transferred but NOT rendered anywhere.  But the secure browser UI just triggers off of data transfer...

We may be able to fix the feed code to bail out earlier (from the initial onStartRequest) if it's not actually the data content that's a problem.  But we should try to fix the UI bit as well.

Comment 21

[David Bolter [:davidb] (NeedInfo me for attention)]

Boris, who do you think can take this one?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Well, normally for secure browser UI I'd guess Kai, but maybe bsmith?

Or alternately someone who knows the feed sniffer code?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: That said, fix to FeedConverter if we need something fast here

Gavin, do you think this is worth taking in the meantime?

Comment 24

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

Comment on attachment 612997 [details] [diff] [review]
That said, fix to FeedConverter if we need something fast here

I don't fully understand how this code works. Can you point me to the relevant AsyncConvertData call that invokes the feed converter in this scenario?

Do you want to be checking requestSucceeded before calling getResponseHeader (which potentially throws)?

Comment 25

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

Comment on attachment 612997 [details] [diff] [review]
That said, fix to FeedConverter if we need something fast here

The general idea seems fine, but I dont't want to r+ this without further clarification (comment 24).

Comment 26

[Boris Zbarsky [:bzbarsky]]

Sorry, I didn't get any mail for comment 24...

The relevant AsyncConvertData call is right here: http://hg.mozilla.org/mozilla-central/file/1711e06ca9f7/uriloader/base/nsURILoader.cpp#l628

> Do you want to be checking requestSucceeded before calling getResponseHeader

Yes, I do.  Good catch.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Feed converter should check HTTP channel response status before proceeding.

Comment 28

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

Comment on attachment 614172 [details] [diff] [review]
Feed converter should check HTTP channel response status before proceeding.

It seems like other nsIStreamConverters could manage to trigger this same issue. Is addressing the root cause what you mean by "fix the UI bit as well" in comment 20?

Comment 29

[Jeroen van der Gun]

Attachment: Example HTTPS resource, well-formed XML, invalid Atom, HTTP 200 OK

To check whether the HTTP status code has any influence on this bug...

Comment 30

[Jeroen van der Gun]

This is also interesting:

1. Visit a non-HTTPS website
2. Enter https://bugzilla.mozilla.org/attachment.cgi?id=614200 in the address bar

Contrary to the Twitter API, I get a download dialog. However, after I cancel that dialog, the site identity button is still spoofed.

My hypothesis:
* An invalid feed shows a download dialog and spoofs the site identity button.
* If, additionally, the HTTP status code indicates an error, this download dialog is suppressed.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> Is addressing the root cause what you mean by "fix the UI bit as well" in comment 20?

Yes.  Basically, the secure browser UI assumes that data read from the network means data rendered, which is just a broken assumption.  In my humble opinion.

I guess I should file a followup bug on that....

Comment 32

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5bdfc9140480

Filed bug 745254 on the security UI issue.

Gavin, how do you feel about backporting this patch?  Seems pretty safe to me....

Comment 33

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

Yeah, I don't see any problem with that.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 614172 [details] [diff] [review]
Feed converter should check HTTP channel response status before proceeding.

[Approval Request Comment]
Regression caused by (bug #): None
User impact if declined: Possible to spoof some random attack page as Twitter and
   possibly other sites via the security UI.
Testing completed (on m-c, etc.): Fixes the testcase in this bug
Risk to taking this patch (and alternatives if risky): Low risk: should only
   affect behavior when loading an HTTP error page with a feed MIME type.
String changes made by this patch:  None.

Comment 35

[Jeroen van der Gun]

I presume the patch in comment 34 does not solve the new test case in comment 30? If I'm correct, this patch would still allow spoofing in certain cases (although a strange download dialog would show up).

(Note I'm not authorized to open bug 745254.)

Comment 36

[Boris Zbarsky [:bzbarsky]]

I can't reproduce the issue from comment 30, so I can't tell how the patch here affects it.....

Comment 37

[Jeroen van der Gun]

The steps to reproduce are the same as in comment 5 / comment 6, but you now paste the URL of the Bugzilla attachment to steal Buzilla's identity instead of a Twitter URL to steal Twitter's identity. Hope that makes it more clear.

Comment 38

[Boris Zbarsky [:bzbarsky]]

The steps were plenty clear.  But in today's nightly when I follow them I see the bugzilla URI in the url bar, but the security stuff all still refers to the site that's actually loaded, as expected.

Comment 39

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

I see the the security info show the information for the currently displayed page, except that it makes it look like it uses SSL, when it actually doesn't (e.g. gavinsharp.com, which doesn't support SSL, shows http://cl.ly/1W1T1i441i2O141o1916).

Comment 40

[Boris Zbarsky [:bzbarsky]]

Ah.  That might be bug 745254, yes.  I don't think the patch in this bug would affect it.

Comment 41

[Jeroen van der Gun]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #39)

That's the same as in the Twitter case. The domain name is correct but the rest of the site identity button is wrong. If Bugzilla would use Extended Validation, you would see 'Mozilla Corporation' in the button and 'connected to gavinsharp.com, run by Mozilla Corporation' in the drop-down window. (See also right picture in attachment 585286 [details], it says the domain is localhost.)

Extended Validation is what makes this bug dangerous. Without Extended Validation, you could only spoof a secure connection for your own domain. With Extended Validation, the name of the target company is shown instead of your own domain name.

Comment 42

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/mozilla-central/rev/5bdfc9140480

Comment 43

[Alex Keybl [:akeybl]]

Comment on attachment 614172 [details] [diff] [review]
Feed converter should check HTTP channel response status before proceeding.

[Triage Comment]
As long as this makes it into our final beta (please land today), we'll take this for FF12 and up. Approving for all branches.

Comment 44

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/releases/mozilla-esr10/rev/ddcd990bddd6
http://hg.mozilla.org/releases/mozilla-aurora/rev/4f8477634e90
http://hg.mozilla.org/releases/mozilla-beta/rev/75bd6549d1ce

Comment 45

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

The PoC in comment 6 is fixed in nightly trunk. Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:14.0) Gecko/20120418 Firefox/14.0a1

I see bug 745254 still needs to be fixed.

Comment 46

[u279076]

Verified fixed in Firefox 10.0.6esrpre 2012-06-21 using comment 6.

Comment 47

[Raymond Forbes[:rforbes]]

rforbes-bugspam-for-setting-that-bounty-flag-20130719