491818 - FF does not follow browser redirection to Proxy Login page

Status: RESOLVED WONTFIX

(Core :: Networking, defect)

Description

[Dominique Leuenberger]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b4) Gecko/20090426 SUSE/3.5b4-1.1 Firefox/3.5b4
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b4) Gecko/20090426 SUSE/3.5b4-1.1 Firefox/3.5b4

Problem appeared together with the change to FF 3.5 (worked in the same setup flawless on 3.0.10)

- Corporate environment. Proxy settings distributed by PAC file (http://proxy.foo.com/proxy.pac)
the PAC file returns by default two proxy entries and contains exceptions for all internal websites (sites in the domain foo.com).

The Proxy is a Novell Border Manager, which requires authentication.

so on first 'web site hit', the browser is redirected to a BM login page:
- server.foo.com:444/BM-Login/

With FF 3.5, this redirection does not succeed and I get an error 'a proxy was configured which refused connection'.

Manually browsing to the login page, logging in and then browsing the web works (as the proxy no longer redirects to a login page).

Reproducible: Always

Steps to Reproduce:
Might be tricky, as it's a rather specific setup

1. configure proxy settings
2. have a proxy that redirects to a login page
3.
Actual Results:  
the login page can't be reached

Expected Results:  
The login page appears on screen, you login and then browsing is possible

Comment 1

[Wolfgang Rosenauer [:wolfiR]]

I cannot triage easily but if it's valid (no reason for me to doubt that) it's rather critical. So getting on people's radar by asking for blocking.

Comment 2

[Dominique Leuenberger]

Additional info: it makes a difference if the first page is http or https:
on a http:// request, the HTTP/302 is followed, apparently, on a https:// it is not followed to the proxy login page.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

jduell: was this perhaps caused by bug 479880?

Dominique: does it work with Firefox 3.1 beta 3?

Comment 4

[Jason Duell]

> jduell: was this perhaps caused by bug 479880?

It seems very like that it was.  I'm reopening 479880.  Sigh...

Comment 5

[Dominique Leuenberger]

(In reply to comment #3)
> Dominique: does it work with Firefox 3.1 beta 3?

Wolfgang: do you have anywhere a package I could install for this test? (OSS Factory, x86_64). This would be very helpful for me.

Comment 6

[Samuel Sidler (old account; do not CC)]

Sounds like we need to block on this for 1.9.0.11 (or maybe .12?).

Comment 7

[Adam Barth]

We have to be careful when following redirects because of this case:

<script src="https://www.paypalobjects.com/foo.js"></script>

If we follow redirects, then an active network attacker can redirect the
request and supply his or her own script.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> jduell: was this perhaps caused by bug 479880?

That doesn't match comment 0.  That bug is fixed in Firefox 3.0.10, and this bug is claimed to not appear in that version.  Or is comment 0 wrong?

In any case, the right place to address this bug is right here, not by reopening other bugs.

Dominique, do you think you can try a Firefox 3.0.11 candidate build and see whether the problem appears there too?  Also, I assume the proxy is doing the redirect via a 3xx response, right?  Or is it using some other mechanism?

To answer Jason's question in bug 479880, if the redirect is done via HTTP status code, then it's quite possible to follow it in this case.  Just add the relevant status codes to the if condition that tests for 407 in ProcessResponse, then in the relevant cases error out if redirection fails (again, just like we handled 407).

If the server is using something like a <meta> tag, of course, that won't work.

Comment 9

[Boris Zbarsky [:bzbarsky]]

For comment 7, we could just check whether the request is a document request, I think.  Adam, that seems like it would address your concerns, since data from document requests doesn't get imported into someone else's security context.

Comment 10

[Adam Barth]

That might work, but then you have to worry about iframe injection:

<iframe src="https://example.com/menu.html"></iframe>

Maybe it's safe to follow redirects for the main frame?

Comment 11

[Jason Duell]

Dominique,

If you could grab a network trace of the interaction between the browser and your proxy (using a tool like Wireshark), and attach it to this bug, that would be helpful.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> That might work, but then you have to worry about iframe injection:

You mean worry in terms of phishing stuff, right?  We could restrict it to toplevel frames, I suppose.

Comment 13

[Adam Barth]

Yeah.  Consider this page for example:

https://www.google.com/analytics/reporting/login?ctu=https://www.google.com/analytics/settings/%3Fet%3Dreset%26hl%3Den-US

The password field is in an HTTPS iframe.  If you let the attacker redirect the iframe load to https://attacker.com, you'd have no way to know that you were sending your password off to disaster.

Comment 14

[Adam Barth]

Actually, even top-level loads can be a problem if the request is a POST.  Consider this page:

https://www.google.com/accounts/

If you let the attacker inject a 307 redirect to https://attacker.com, the POST data containing the password gets sent to the attacker.  (Note that the load is targeted at the main frame.)

Comment 15

[Dominique Leuenberger]

Attachment: wireshark trace

some description for the traffic you see:

The Proxy is on 10.3.0.4, port 2323
my machine is 10.25.3.51

the UDP requests from / to port 3024 is the BorderManager Client Trust single sign on check. This does not work properly on Linux (no client available on x86_64), so this one fails. (gives a timeout... but that's fine).

the site I try to connect to (intentionally) is https://webmail.leuenberger.net
the http redirect is in packet No. 167, HTTP/1.1 302 Moved Temporarily

Comment 16

[Boris Zbarsky [:bzbarsky]]

As far as comment 14 goes, the only time we send POST data along after a 3xx redirect is if a 307 is used, and then we prompt the user before doing so.  That said, we don't tell the user _where_ the data will be posted, I guess...

Comment 17

[Samuel Sidler (old account; do not CC)]

Dominique: Just to make sure... In comment 0 you said you didn't see this issue using Firefox 3.0.10. Can you confirm that? If this is a regression from bug 479880, it'd likely appear in 3.0.10 but not 3.0.9, unless it was interacting with something else in the Firefox 3.5 betas.

Comment 18

[Jason Duell]

Attachment: Allow 3xx redirects from proxy servers but don't parse reply if redirect fails

OK, I've implemented BZ's suggestion to just let the 3xx response codes through, and then jump to ProcessFailedSSLConnect() if the redirect fails.  

I've verified that the body of the 3xx response is not parsed in either case (i.e no <script> risks).  I tried a batch of different test cases, which I'll also attach.

I don't know about enough about the document request && iframes stuff to say whether there's still some additional risk here.  Bz/Adam?

Comment 19

[Jason Duell]

Attachment: 302 reply without Location: header. Illegal. (run with "nc -l 3128 resp302_no_location": config Firefox to use proxy at port 3128)

Comment 20

[Jason Duell]

Attachment: Same as previous test, but redirects legally to https://www.mozilla.org. Make an exception for .mozilla.org in proxy settings, and you'll be redirected successfully

Comment 21

[Jason Duell]

Attachment: test that redirects to a bogus protocol

So the last test I attached exhibits some possibly weird behavior (that is if anything a separate bug):  I redirect the user with a Location header to a bogus protocol ("Location: bogus_protocol://whatevar").   The redirect fails, so we're still safe from <script>s in the response body.  But the URL displayed in Firefox is "https://www.mozilla.org/bogus_protocol://whatevar" (the original URL requested was https://www.mozilla.org/, so we're treating the bad protocol as a relative link: is that kosher?).

Comment 22

[Adam Barth]

If I'm understanding this patch correctly, it's still wrong.  You seem to be following the redirect for <script> tags.  That means an active network attacker can XSS https://www.paypal.com/ by redirect requests to https://www.paypalobjects.com to https://attacker.com.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Yeah.  What you probably want to do in the 3xx case is to still fall through to ProcessFailedSSLConnect unless all of the following are true:

1)  The channel has the LOAD_DOCUMENT_URI flag.
2)  The channel's nsILoadContext has an associatedWindow and that window is the
    same as the load context topWindow.
3)  The status code is not 307 or the channel has no POST data.

We could tighten up #3 to "the channel has no POST data" for consistency, if desired.

Comment 24

[Jason Duell]

Attachment: Now allows only top-level, non-POST document loads

OK, I think I've incorporated BZ's additional requirements.  I had to REQUIRE docshell to do it (does this harm necko standalone-ness?  Do we care?).

Comment 25

[Jason Duell]

Oh--I forbid all POST redirects, because this is really only going to be used by proxies that want to redirect users to a login page.  I figured it's exceedingly unlikely that such a login page is going to pass-through a POST after login.

Comment 26

[Jason Duell]

Attachment: Manual test HTML page for this bug

This HTML page contains tests for inline <scripts>, <iframes>, and <images>, as well as testing that top-level page redirects work.  Step-by-step instructions included.

Comment 27

[Jason Duell]

Marking as secure.  Undo if I'm wrong, but we've got the same info in here at bug 479880, which was secure.

Comment 28

[Boris Zbarsky [:bzbarsky]]

So..  that really should be using nsILoadContext if we do it this way.

But the point about REQUIRES is a good one.  We don't want to make that change, I don't think.  I clearly spend too much of my time in code that already depends on docshell.

Can we cheat and use "mURI == mDocumentURI" (plus the LOAD_DOCUMENT_URI flag) as the test for a toplevel load?  We sort of do already in SetupReplacementChannel....

Comment 29

[Boris Zbarsky [:bzbarsky]]

Oh, and you can just use mLoadFlags directly I think.  And instead of the

  if (x)
    return PR_TRUE;

  return PR_FALSE;

pattern, just |return x|?

Comment 30

[Jason Duell]

Attachment: Eliminate DocShell dependency

> Can we cheat and use "mURI == mDocumentURI" 

Yes, very nice.

Other suggestions were also good, and incorporated.

Patch tested with HTML test I previous attached.  Should be ready to go.

Comment 31

[Johnny Stenback (:jst)]

Blocking, we should fix this regression for 3.5.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 377620 [details] [diff] [review]
Eliminate DocShell dependency

>+nsHttpChannel::ShouldSSLProxyResponseContinue(PRUint32 httpStatus)
>+        return (mLoadFlags & nsIChannel::LOAD_DOCUMENT_URI
>+             && mURI == mDocumentURI
>+             && mRequestHead.Method() != nsHttp::Post);

Two nits here.  Please put a pair of parens around the '&': while '&' does have higher precedence than '&&', having to remember that or look it up is a huge pain.  Also, please put the '&&' at the end of the previous line, not the beginning of the next one.

> nsHttpChannel::ProcessResponse()

>+    if (mTransaction->SSLConnectFailed())
>+        if (!ShouldSSLProxyResponseContinue(httpStatus))

Some use of && here would be better, I think.  ;)


>+++ b/netwerk/protocol/http/src/nsHttpChannel.h

>+    PRBool   ShouldSSLProxyResponseContinue(PRUint32 httpStatus);

Document, please.

With those nits, looks good.

Comment 33

[Jason Duell]

Attachment: Code reformatted to BZ's irrational whims

I've made all the changes except the documentation request, since I didn't understand it (what's wrong with the comment at the beginning of ShouldSSLProxyResponseContinue()?  We don't describe any functions in the .h file.  So I'm not sure where or what you want me to write).

I made all of the formatting changes, though I don't actually agree with any of them :).  (ANDs are more natural as the beginning of a clause, and would lessen the need to put 'x & y' in parens.)

We want to get this into the Wednesday release;  feel free to modify my mods as you like if it moves things along.   I'm happy to add more docs if you tell me where.

Comment 34

[Boris Zbarsky [:bzbarsky]]

> I've made all the changes except the documentation request

Ok, I guess.  I sort of like the purpose of functions documented in the header (with the implementation documented in the body as needed), but this one's name might be good enough for the purpose...  I'll land that last patch as-is.  Thanks for fixing the other issues!

Comment 35

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/b7f9b3f6799f

Comment 36

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/43f69cd368c4

Jason, thanks for indulging my whims.  ;)

Comment 37

[Samuel Sidler (old account; do not CC)]

Since it's not clear that this regressed on 1.9.0, we're pushing it out past 1.9.0.11. It'd be good to know if we need to take this in 1.9.0.12 though...

Comment 38

[Jason Duell]

> Since it's not clear that this regressed on 1.9.0

Sorry if I didn't make that clear:  this is a regression om 1.9.0.  Proxy HTTPS redirects (which previously didn't fail in Fx 3.0.9 and earlier) fail in 3.0.10. That's the regression, and what the bug was opened on.

The other security risks from proxy replies are present in 3.0.9 and earlier, and fixed in 3.0.10

Both issues are fixed on trunk and 1.9.1.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Renominating per comment 38 (though I can understand if we still want to push this to 1.9.0.12).

Comment 40

[Samuel Sidler (old account; do not CC)]

We're already code frozen for 1.9.0.11 and Dan and I discussed and decided this can wait for 1.9.0.12. I'd like it to bake anyway. We'll get it fixed in Firefox 3.0.12 (and please be sure to request approval on an appropriate patch).

Comment 41

[Boris Zbarsky [:bzbarsky]]

Jason, want to spin up a 1.9.0 patch here?  Or does that last patch apply to 1.9.0 cleanly?

Comment 42

[Jason Duell]

Attachment: Patch for 1.9.0 branch

Just needed one line change in .h file.

Comment 43

[Daniel Veditz [:dveditz]]

Comment on attachment 378526 [details] [diff] [review]
Patch for 1.9.0 branch

Approved for 1.9.0.12, a=dveditz for release-drivers

Comment 44

[Jason Duell]

So it turns out that all other major browsers (IE 8, Safari, Chrome, Opera) no longer honor 3xx replies from HTTPS proxies.  This is in response to the script/iframe vulnerabilities mentioned in the paper cited in bug 479880 (see section III.B: it's basically what we already describe in this bug report).

We're handling the exploits in the paper correctly.  But since we do support top-level redirects, an evil interloper could still redirect a user to a bogus site, and they might be vulnerable if they don't look at the location bar.   I think it was worth supporting top-level redirects when it looked like some existing proxies relied on them for logins, but if all other browsers have now broken that use case, that's moot, and the main "use" for 3xx replies going forward might be the malicious scenario.

I suspect we should back the patches out and change to WONTFIX. 

Locking down bug while we discuss.

Comment 45

[Boris Zbarsky [:bzbarsky]]

> might be vulnerable if they don't look at the location bar

That's always the case, no?

Have we talked about this with the other browser vendors, by any chance?  Maybe it's just that no one reported a bug to them about this case yet...

Comment 46

[Boris Zbarsky [:bzbarsky]]

Checking in nsHttpChannel.h;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.h,v  <--  nsHttpChannel.h
new revision: 1.99; previous revision: 1.98
done
Checking in nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <--  nsHttpChannel.cpp
new revision: 1.338; previous revision: 1.337
done

Comment 47

[Jason Duell]

From the description in the paper, it sounds like IE and Chrome have not supported 30x replies from proxy servers for a while.  But if we think we're handling them safely, we might as well keep them, just in case some proxy authors want to special-case and give us a redirect to a custom error page.  It's better than not giving them any way to deliver their own content.

Is there a well-known good way to talk about this with other browser vendors?  I think I'll leave filing bug reports with apple, etc., to the proxy vendors.

Making bug public again.

Re: a 1.8 patch:  I'm taking a PTO day today.  If I see my 1.8 patch for 479880 land before Tuesday, I'll submit a separate patch for this bug; otherwise I'll roll them together (which bug would I attach a combo patch for?  I assume 479880).

Comment 48

[Daniel Veditz [:dveditz]]

I landed bug 479880 on the 1.8 branch.

Comment 49

[Jason Duell]

Attachment: Patch for 1.8

Comment 50

[Daniel Veditz [:dveditz]]

Comment on attachment 381170 [details] [diff] [review]
Patch for 1.8 

sr=dveditz -- this code is so little changed it doesn't need a full re-review.

Comment 51

[Samuel Sidler (old account; do not CC)]

Comment on attachment 381170 [details] [diff] [review]
Patch for 1.8 

Approved for 1.8.1.22. a=ss

Comment 52

[Daniel Veditz [:dveditz]]

Checked into 1.8(.1) branch.

Checking in nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <--  nsHttpChannel.cpp
new revision: 1.256.2.24; previous revision: 1.256.2.23
done
Checking in nsHttpChannel.h;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.h,v  <--  nsHttpChannel.h
new revision: 1.70.4.7; previous revision: 1.70.4.6
done

Comment 53

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

(In reply to comment #26)
> Created an attachment (id=377596) [details]
> Manual test HTML page for this bug
> 
> This HTML page contains tests for inline <scripts>, <iframes>, and <images>, as
> well as testing that top-level page redirects work.  Step-by-step instructions
> included.

I used this in 1.8.1.22 testing with Seamonkey. Everything is fixed in the latest nightly builds except for step 14 ("Click here. You should be redirected to Mozilla.org."). This redirect fails with both the nightlies and with the shipped 1.1.16. I assume that the expected behavior, post-fix, is that the redirect works?

Comment 54

[Jason Duell]

Yes, the last redirect to https://www.mozilla.org should work.  But, did you follow step #1 (Make an exception for ".mozilla.com" in your proxy settings)?

Oh, actually, there's a typo in my instructions--the exception should be for ".mozilla.org", because that's where the link goes to, not mozilla.com.  Sigh.  Sorry! 

Anyway, change the exception to use mozilla.org, and you should be fine.

Comment 55

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

Jason, yeah, I followed the instructions (this time) and tried that alternate, which did work. I was wondering if something was off.

Ok, this is verified then.

Verified1.8.1.22

Comment 56

[Jason Duell]

Since there were some lingering sec concerns about allows HTTPS redirects with CONNECT, and other browsers don't support them, we've re-blocked them in bug 599295.  So this will be WONTFIX as of Firefox 10 (i.e. whenever bug 599295 lands).

Comment 57

[Nicolas Mailhot]

This problem could be fixed cleanly by handling
HTTP error 511 Network Authentication Required

as defined by the ietf httpbis workgroup (see bug #728658)