267263 - Browser does not test to see that server sending proxy auth request is the proxy server (ssl/https).

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Christopher Nebergall]

I haven't seen anywhere in the code where auto-send proxy auth like NTLM and now
SPNEGO are checked to make sure that the proxy server which is configured on the
channel is actually the same server that is sending the 407 to the client.  Did
I miss something?

What would happen if I was connecting to an SSL protected site which uses the
connect method through the proxy and that site sends a 407 w/ NTLM or SPNEGO? 
Would the browser autosend my credentials out site of my network just because a
proxy was configured for that channel?

Comment 1

[Christopher Nebergall]

 if (proxyAuth) {
2100         // only allow a proxy challenge if we have a proxy server configured.
2101         // otherwise, we could inadvertantly expose the user's proxy
2102         // credentials to an origin server.  We could attempt to proceed as
2103         // if we had received a 401 from the server, but why risk flirting
2104         // with trouble?  IE similarly rejects 407s when a proxy server is
2105         // not configured, so there's no reason not to do the same.
2106         if (!mConnectionInfo->UsingHttpProxy()) {
2107             LOG(("rejecting 407 when proxy server not configured!\n"));
2108             return NS_ERROR_UNEXPECTED;
2109         }
2110         challenges = mResponseHead->PeekHeader(nsHttp::Proxy_Authenticate);
2111     }

Comment 2

[Darin Fisher]

I think you are correct.  The test for UsingHttpProxy() should have been
accompanied with a test for !UsingSSL().  This is a serious security flaw. 
Thank you for catching it!

Comment 3

[Darin Fisher]

Actually, testing for UsingSSL() is not sufficient.  We need to allow 407
responses from the SSL proxy server itself, but we need to reject them once we
are successfully connected.  That is going to be tricky to implement.

Comment 4

[Darin Fisher]

Attachment: v1 patch - tested

This patch should do the trick.  NOTE: I haven't built a testcase for this yet,
but I wanted to post this anyways, so my approach could be reviewed.

Comment 5

[Darin Fisher]

Comment on attachment 164259 [details] [diff] [review]
v1 patch - tested

ok, i tested this patch using a squid proxy server configured to allow CONNECT
requests and an apache server hosting a CGI script that output a 407 response.

i used apache's "nph" feature to create the CGI script:

$ cat nph-test.cgi
#!/bin/sh

echo 'HTTP/1.0 407 Proxy Auth'
echo 'Proxy-Authenticate: basic "dummy"'
echo 'Connection: close'
echo ''
echo 'hello'

without my patch loading https://localhost/nph-test.cgi would result in a
prompt for proxy authentication.

with my patch, there was no prompt, and the contents of the 407 response were
displayed in the browser window.

it's arguable that we should maybe not even show content in this case, since it
is likely a spoofing attempt, but i wanted to keep the patch as simple as
possible for now.

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 164259 [details] [diff] [review]
v1 patch - tested

a=[e-mail] for immediate checkin for testing in the next batch of
nightly builds.

Comment 7

[Darin Fisher]

fixed-aviary1.0

waiting for reviews before landing on the trunk and the 1.7 branch.

Comment 8

[Darin Fisher]

I also verified that SSL proxy authentication still works :-)

Comment 9

[Christopher Nebergall]

(In reply to comment #3)
> Actually, testing for UsingSSL() is not sufficient.  We need to allow 407
> responses from the SSL proxy server itself, but we need to reject them once we
> are successfully connected.  That is going to be tricky to implement.

+        if (mConnectionInfo->UsingSSL() && !mTransaction->SSLConnectFailed()) {

I still don't quite understand the need for !mTransaction->SSLConnectFailed(). 
AFAIK SSL Proxy servers don't encrypt or decrypt SSL so if the browser is using
SSL then its response will not be readable by the proxy server and must be going
to the end server.

Bugzilla seems to be having database errors, so if this gets reposted I appologize.

Comment 10

[Christopher Nebergall]

(In reply to comment #3)
> Actually, testing for UsingSSL() is not sufficient.  We need to allow 407
> responses from the SSL proxy server itself, but we need to reject them once we
> are successfully connected.  That is going to be tricky to implement.

+        if (mConnectionInfo->UsingSSL() && !mTransaction->SSLConnectFailed()) {

I still don't quite understand the need for !mTransaction->SSLConnectFailed(). 
AFAIK SSL Proxy servers don't encrypt or decrypt SSL so if the browser is using
SSL then its response will not be readable by the proxy server and must be going
to the end server.

Bugzilla seems to be having database errors, so if this gets reposted I appologize.

[Not specific to this patch] 
The Mozilla code makes the (presumably valid) assumption that all proxy
authenticate headers are added by the proxy server and stripped by the proxy
server if they actually come from an end server.  Is this assumption still true
if a user is using a Socks proxy or is a socks proxy to generic to care about
HTTP headers?

Comment 11

[Darin Fisher]

> I still don't quite understand the need for !mTransaction->SSLConnectFailed(). 
> AFAIK SSL Proxy servers don't encrypt or decrypt SSL so if the browser is 
> using SSL then its response will not be readable by the proxy server and must 
> be going to the end server.

Yes, but before we start speaking SSL over the connection, we first have to
speak plaintext HTTP to issue the CONNECT request.  The response to the CONNECT
request could be a 407 response from the SSL proxy itself.  After a 200 response
from the CONNECT request, we "step up" the connection to SSL.


> Is this assumption still true if a user is using a Socks proxy or is a socks 
> proxy to generic to care about HTTP headers?

We don't support talking to a HTTP proxy over a SOCKS connection at the moment.
 We also don't make any assumptions about a SOCKS proxy modifying the HTTP
transactions.  UsingHttpProxy() is false when a SOCKS proxy is configured.

Comment 12

[Christopher Nebergall]

Comment on attachment 164259 [details] [diff] [review]
v1 patch - tested

Thanks.

r=cneberg

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 164259 [details] [diff] [review]
v1 patch - tested

sr=dveditz

Comment 14

[Darin Fisher]

fixed-on-trunk

Comment 15

[Darin Fisher]

fixed1.7.x

Comment 16

[Christopher Nebergall]

The security Advisory for this bug is now public
http://www.mozilla.org/security/announce/mfsa2005-09.html

Unfortunately, it states that this bug is NTLM/SPNEGO specific. It should work
equally well with any auth type including stealing basic passwords (I didn't
realize this when I opened the bug, it only came out later in further comments).  

Maybe in the future developers/reporters on security bugs should write an
updated security summary when the bug is fixed and put the URI of the comment on
the whiteboard so whoever writes the summaries doesn't have to read the whole
thread to find out the implications or extent of the bug.  

Comment 17

[Daniel Veditz [:dveditz]]

Security Advisories published, clearing confidential flag

Comment 18

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch, backported to 1.4

Includes the patch from bug 205947 comment 37, and shaver's suspenders.

Comment 19

[Darin Fisher]

Comment on attachment 172415 [details] [diff] [review]
Patch, backported to 1.4

r=darin