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

RESOLVED FIXED in mozilla1.8beta1

Status

()

defect
--
critical
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: cneberg, Assigned: darin.moz)

Tracking

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

Trunk
mozilla1.8beta1
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
blocking-aviary1.0 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Reporter

Description

15 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

15 years ago
Flags: blocking-aviary1.0?
Reporter

Comment 1

15 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

15 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

15 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

15 years ago
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

15 years ago
Flags: blocking-aviary1.0? → blocking-aviary1.0+
Assignee

Comment 5

15 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

15 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

15 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

15 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

15 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

15 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

15 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

15 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

15 years ago
Attachment #164259 - Flags: approval1.7.x?
Assignee

Comment 14

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

Comment 15

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

Updated

15 years ago
Flags: blocking1.7.5?

Updated

15 years ago
Blocks: 248511
Reporter

Comment 16

15 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
Includes the patch from bug 205947 comment 37, and shaver's suspenders.
Assignee

Comment 19

15 years ago
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.