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

RESOLVED FIXED in mozilla1.8beta1

Status

()

Core
Networking: HTTP
--
critical
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: Christopher Nebergall, Assigned: Darin Fisher)

Tracking

({fixed-aviary1.0, fixed1.4.4, fixed1.7.5})

Trunk
mozilla1.8beta1
x86
All
fixed-aviary1.0, fixed1.4.4, fixed1.7.5
Points:
---
Bug Flags:
blocking-aviary1.0 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

13 years ago
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?
(Reporter)

Updated

13 years ago
Flags: blocking-aviary1.0?
(Reporter)

Comment 1

13 years ago
 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     }
(Assignee)

Comment 2

13 years ago
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
(Assignee)

Comment 3

13 years ago
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.
(Assignee)

Comment 4

13 years ago
Created attachment 164259 [details] [diff] [review]
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.

Updated

13 years ago
Flags: blocking-aviary1.0? → blocking-aviary1.0+
(Assignee)

Comment 5

13 years ago
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)
(Assignee)

Updated

13 years ago
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+
(Assignee)

Comment 7

13 years ago
fixed-aviary1.0

waiting for reviews before landing on the trunk and the 1.7 branch.
Keywords: fixed-aviary1.0
(Assignee)

Comment 8

13 years ago
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).
(Reporter)

Comment 9

13 years ago
(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.
(Reporter)

Comment 10

13 years ago
(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?
(Assignee)

Comment 11

13 years ago
> 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.
(Reporter)

Comment 12

13 years ago
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+
(Assignee)

Updated

13 years ago
Attachment #164259 - Flags: approval1.7.x?
(Assignee)

Comment 14

13 years ago
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Attachment #164259 - Flags: approval1.7.x? → approval1.7.x+
(Assignee)

Comment 15

13 years ago
fixed1.7.x
Keywords: fixed1.7.x

Updated

13 years ago
Flags: blocking1.7.5?

Updated

13 years ago
Blocks: 248511
(Reporter)

Comment 16

13 years ago
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
Created attachment 172415 [details] [diff] [review]
Patch, backported to 1.4

Includes the patch from bug 205947 comment 37, and shaver's suspenders.
Attachment #172415 - Flags: review?(darin)
(Assignee)

Comment 19

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

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