User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; de; rv:1.9.2.10) Gecko/20100915 Ubuntu/9.04 (jaunty) Firefox/3.6.10 Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; de; rv:1.9.2.10) Gecko/20100915 Ubuntu/9.04 (jaunty) Firefox/3.6.10 The firefox accepts a 302 Redirect after the CONNECT method. This can be exploited by an attacker for phising attacks. Let's assume that the attacker can modify packets on a connection. A user wants to connect via a proxy (not located in the local lan) to a bank site. FF: CONNECT mybank.com HTTP/1.1 Proxy: HTTP/1.1 200 Connection established Attacker transforms this to: HTTP/1.1 302 Moved\r\n Location: https://evilbank.com/ The user is redirected without any warning to the other web site. There is no warning and thus the security mechanism of SSL does not longer work. The evilbank.com can have a valid certificate for the domain and the user can be eavesdropped by the attacker. If the CONNECT method is modified directly instead of the response, the user sees a warning. Reproducible: Always Steps to Reproduce: 1. Modify the HTTP/1.1 200 Connection Established HTTP/1.1 line to HTTP/1.1 302 Redirect and insert a new location Actual Results: The user is redirected to another location. Expected Results: The connection is closed or at least a warning is displayed. This kind of attack does not work in Chrome. Here the connection is reset and the user sees a warning.

An easy way to confirm this bug (partially) is to run the following command in a linux bash: touch run; while [ -f run ]; do nc -l 30012 <<EOF HTTP/1.1 302 Moved^M Location: https://www.google.com^M ^M EOF done; # to stop the script, delete the file run Afterwards, one has to configure FF to use a proxy (http/https localhost:30012). Query an arbitrary https site (not google ;-) ). Even though the FF shows an infinite redirect warning, it demonstrates the problem. The FF starts to issue CONNECT methods to google.com. However, in this limited proof-of-concept it does not receive any certificate, but even if, FF will not complain.

Interesting. Does the address bar show the site that provided the content after the redirect?

To avoid infinite loop warning, add www.google.com to the "No Proxy For" list on the proxy setting page. The "Pretty-Bad-Proxy" paper discusses a scenario of using 302 redirects that targets script content inclusion (http://research.microsoft.com/apps/pubs/default.aspx?id=79323 , section III.B "Redirecting Script Requests to Malicious HTTPS Websites"). Here is what I observed in my testing: In Firefox 3.6.10, the browser appears to handle this as a normal 302 and address bar shows location of redirect. In Firefox 4.0b6, the browser doesn't appear to process the 302 following the CONNECT request. There is no warning or other indication that the request fails. The address bar is not updated, but the current page is not refreshed either.

You're right that this attack is best applied to loads of <script> tags. For example, redirecting the CONNECT request to paypalobjects.com when loading paypal.com.

abarth: is this a regression? Didn't we fix this bug in bug 491818? A fallout of bug 491818?

I got one bug number wrong in the previous comment. Sorry. It should read: abarth: is this a regression? Didn't we fix this bug in bug 479880? A fallout of bug 491818?

Reading through Bug 491818 again, it looks like we thought about these nuances. Someone should test an actual attack scenario and see if something bad happens. According to that bug, the redirects should be blocked on subresources. Redirecting the main frame is usually ok, but could be dangerous if there's POST data. If we're just redirecting the main frame for GET requests, we're probably ok.

(In reply to comment #7) > Reading through Bug 491818 again, it looks like we thought about these nuances. > Someone should test an actual attack scenario and see if something bad > happens. According to that bug, the redirects should be blocked on > subresources. Redirecting the main frame is usually ok, but could be dangerous > if there's POST data. If we're just redirecting the main frame for GET > requests, we're probably ok. I disagree: The browser also checks with a direct TLS connection that the entered URL matches the URL in the certificate, why should the user not expect the very same behaviour for tunnelled TLS connections? Additionally, I would doubt that a user checks letter by letter the entered URL against the displayed URL after the page has been loaded, the content is as expected and the green status bar is displayed. Thus, I think it's dangerous to allow any unauthenticated redirect if an authenticated session was requested by a user.

If the user doesn't read the URL bar, there's no way for them to be secure. Keep in mind that an attacker can often navigate a top-level window wherever they like.

Seems like there's two issues here: 1) Whether we did the right thing policy-wise by allowing 302 CONNECT redirects. 2) Whether we have a regression whereby we no longer actually perform the redirects in 4.0b7pre (see comment 3). The comments are all about #1, but I'm wondering what the deal is with #2. Anyway, let's figure out the policy and go from there. As I mentioned in bug 491818 comment 44, the other browsers don't support top-level redirects on CONNECT. We allow it because it seemed safe (note: we don't allow CONNECT redirect on POSTs or non-top-level loads), and thought that maybe proxy vendors would like to use it to show better error pages (now that we don't render any non-redirect content from proxies). But if other browsers don't support it, than it's perhaps not likely to be used other than by Evildoers. I have no opinion about the right thing to do here. But if we mark this as INVALID, let's make sure we followup on point #2.

If we're updating the address bar then this isn't an sg:high complete spoof.

Shouldn't this be in Core::Networking?

I have confirmed the behavior of the current nightly (FF10), to be the same as 3.6.10 from comment 3: "In Firefox 3.6.10, the browser appears to handle this as a normal 302 and address bar shows location of redirect." So comment 3 regarding 4.0.b6 might have just been some other bug with the beta. In any event in the rapid release model FF4 through FF7 aren't especially interesting. I would expect a change to go on m-c and then drivers to decide if they wanted to backport to aurora, beta, or 3.6.x I am going to post a change for review - to disallow redirects on CONNECT because they are an insecure exchange that is a prelude to a request for a secure connection. Furthermore 491818 comment 44 did the research to show that other browsers do not allow that, so it has no use as a compatibility mechanism.

try likes it https://tbpl.mozilla.org/?tree=Try&rev=2b33e103a7bf

Comment on attachment 571410 [details] [diff] [review] patch v1 the patch on this is very simple - the test is more involved. (there was no test afaict for the old behavior). Normally this would be xpcshell, but I made the test a mochitest-chrome test because we obviously need a http proxy that implements CONNECT to do the test and the only piece I am aware of that can do that is ssltunnel which is only part of mochitest (again, afaict.). So the test extends ssltunnel a little bit to be able to issue redirects which we then build a chrome test for to ensure that they don't work ;) (If you run the test without the patch to nshttpchannel that disallows the redirs you will see they do indeed work.) I've added honza to the bug because he might want to look at the ssltunnel and related changes.

According the ssltunnel changes, as I understand you want to return 302 and not 200 as response to the CONNECT request, right? That is pretty one-purpose change, not well placed. I would really like to see this as an xpc-shell test. There is a bug to also support ssl (ssltunnel) in xpcshell tests (bug 466524). You should finish that patch rather and turn this to a an xpcshell test. I'd like to keep the ssltunnel in its general form as it is and clean. We could potentially land the test changes later, but it is apparently time to push on making xpcshell ssl-enabled.

(In reply to Honza Bambas (:mayhemer) from comment #17) > According the ssltunnel changes, as I understand you want to return 302 and > not 200 as response to the CONNECT request, right? That is pretty > one-purpose change, not well placed. > > I would really like to see this as an xpc-shell test. There is a bug to > also support ssl (ssltunnel) in xpcshell tests (bug 466524). You should > finish that patch rather and turn this to a an xpcshell test. I'd like to > keep the ssltunnel in its general form as it is and clean. > > We could potentially land the test changes later, but it is apparently time > to push on making xpcshell ssl-enabled. yes making xpcshell better is desirable. But its also of low value compared to other projects including clearing this sec bug which should have a test. So i'd like to land it as is. It's a conscious trade off. I suppose an alternative is to just not land a test but I don't know why we would do that when we have one.

(In reply to Patrick McManus from comment #18) > I suppose an alternative is to just not land a test but I don't know why we > would do that when we have one. Yes, not intended to block on it. But I will file a bug to back the changes from ssltunnel back and rewrite the test ASAP we support ssl in xpcshell. There were other reasons to have it, this was just the first that was "blocked" by it. Does that sound good?

(In reply to Honza Bambas (:mayhemer) from comment #19) > (In reply to Patrick McManus from comment #18) > > I suppose an alternative is to just not land a test but I don't know why we > > would do that when we have one. > > Yes, not intended to block on it. But I will file a bug to back the changes > from ssltunnel back and rewrite the test ASAP we support ssl in xpcshell. > There were other reasons to have it, this was just the first that was > "blocked" by it. > > Does that sound good? yep! thanks.

Comment on attachment 571410 [details] [diff] [review] patch v1 Review of attachment 571410 [details] [diff] [review]: ----------------------------------------------------------------- +r on the patch, and it looks like honza has +r'd the ssltunnel change for the test, for now at least (I'd have been ok w/o an automated test, but it's nice to have one--thanks Patrick). Given that this bug really only affects users who don't look at the URI bar, I'm not sure it merits sg:moderate, in case we care (as far as back-ports, etc go). Then again, it's a very quick and easy code fix.

I personally don't see the need to backport this anywhere, but if anyone on the bug feels differently feel free to nominate it to drivers - I won't be hurt :)

dan, can we open this bug?