49.54 KB, image/png
86.50 KB, image/png
1.04 KB, text/html
1014 bytes, patch
|Details | Diff | Splinter Review|
1.30 KB, patch
|Details | Diff | Splinter Review|
306 bytes, application/atom+xml
Confirmed. Just click the attachment and wait.
This happens even with Firefox 3.6.
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.)
Created attachment 585338 [details] Screenshot showing address bar and site identity block not matching content.
See the blog post here, regarding similar issues in Google Chrome: http://blog.acrossecurity.com/2012/01/google-chrome-https-address-bar.html
For this load, the front end code gets an onSecurityChange (update security indicators), but not an onLocationChange (update URL bar).
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?
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.
The onSecurityChange in question is fired from: http://mxr.mozilla.org/mozilla-central/source/security/manager/boot/src/nsSecureBrowserUIImpl.cpp?rev=f18be484cb5a#1225
Hrm. Why is requestHasTransferedData true?
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).
Well, sure. The question is why that happens, if the channel is canceled from OnStartRequest....
Created attachment 590843 [details] 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.
Addition to comment 18: setting Firefox to automatically use an external feed reader does not solve the bug.
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.
Boris, who do you think can take this one?
Well, normally for secure browser UI I'd guess Kai, but maybe bsmith? Or alternately someone who knows the feed sniffer code?
Created attachment 612997 [details] [diff] [review] That said, fix to FeedConverter if we need something fast here Gavin, do you think this is worth taking in the meantime?
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 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).
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.
Created attachment 614172 [details] [diff] [review] Feed converter should check HTTP channel response status before proceeding.
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?
Created attachment 614200 [details] Example HTTPS resource, well-formed XML, invalid Atom, HTTP 200 OK To check whether the HTTP status code has any influence on this bug...
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.
> 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....
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....
Yeah, I don't see any problem with that.
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.
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.)
I can't reproduce the issue from comment 30, so I can't tell how the patch here affects it.....
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.
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).
Ah. That might be bug 745254, yes. I don't think the patch in this bug would affect it.
(In reply to Gavin Sharp (use firstname.lastname@example.org 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 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.
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
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.
Verified fixed in Firefox 10.0.6esrpre 2012-06-21 using comment 6.