Closed Bug 267263 Opened 16 years ago Closed 16 years ago

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

Categories

(Core :: Networking: HTTP, defect)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.8beta1

People

(Reporter: cneberg, Assigned: darin.moz)

References

Details

(Keywords: fixed-aviary1.0, fixed1.4.4, fixed1.7.5)

Attachments

(2 files)

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?
Flags: blocking-aviary1.0?
 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     }
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!
Status: NEW → ASSIGNED
Flags: blocking1.7.x?
Target Milestone: --- → mozilla1.8beta
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.
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.
Flags: blocking-aviary1.0? → blocking-aviary1.0+
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.
Attachment #164259 - Attachment description: v0 patch - untested → v1 patch - tested
Attachment #164259 - Flags: review?(cneberg)
Attachment #164259 - Flags: superreview?(dveditz)
Comment on attachment 164259 [details] [diff] [review]
v1 patch - tested

a=ben@mozilla.org for immediate checkin for testing in the next batch of
nightly builds.
Attachment #164259 - Flags: approval-aviary+
fixed-aviary1.0

waiting for reviews before landing on the trunk and the 1.7 branch.
Keywords: fixed-aviary1.0
I also verified that SSL proxy authentication still works :-)
Summary: Browser does not test to see that server sending proxy auth request is the proxy server. → Browser does not test to see that server sending proxy auth request is the proxy server (ssl/https).
(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.
(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?
> 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 on attachment 164259 [details] [diff] [review]
v1 patch - tested

Thanks.

r=cneberg
Attachment #164259 - Flags: review?(cneberg) → review+
Comment on attachment 164259 [details] [diff] [review]
v1 patch - tested

sr=dveditz
Attachment #164259 - Flags: superreview?(dveditz) → superreview+
Attachment #164259 - Flags: approval1.7.x?
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Attachment #164259 - Flags: approval1.7.x? → approval1.7.x+
fixed1.7.x
Keywords: fixed1.7.x
Flags: blocking1.7.5?
Blocks: 248511
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.  
Security Advisories published, clearing confidential flag
Group: security
Includes the patch from bug 205947 comment 37, and shaver's suspenders.
Comment on attachment 172415 [details] [diff] [review]
Patch, backported to 1.4

r=darin
Attachment #172415 - Flags: review?(darin) → review+
You need to log in before you can comment on or make changes to this bug.