Closed Bug 127671 Opened 23 years ago Closed 23 years ago

Proxy-authorization header incorrectly sent to server during SSL session

Categories

(Core :: Networking: HTTP, defect, P1)

x86
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla1.0

People

(Reporter: peterg, Assigned: badami)

References

()

Details

(Whiteboard: [patch needs r/sr=][adt1])

Attachments

(3 files, 6 obsolete files)

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i586; en-US; rv:0.9.8) Gecko/20020204
BuildID:    2002020415

When I connect to a SSL server via our firewall, the browser sets up a session
correctly using HTTP CONNECT. However, it then sends Proxy-authorization headers
over this connection, including my firewall
username and password.

Reproducible: Always
Steps to Reproduce:
1. Set up a plain-HTTP server, and in front place a reverse proxy server
(we are using Netscape Proxy Server 3.5, on HPUX)
2. Set up Mozilla behind a proxying firewall configured to allow HTTP CONNECT.
3. Connect the firewall to the reverse proxy 
4. Run tcpdump (version 3.6 or higher) with the -X option on the server.
5. Make a connection from the client and watch the tcpdump output.

Actual Results:  The following is seen in the traffic (X marks the spot... in the
real output it does base64-decode into my password)


0x0060   2e65 7664 742e 6769 6620 4854 5450 2f31        .evdt.gif.HTTP/1
0x0070   2e30 0d0a 5072 6f78 792d 6175 7468 6f72        .0..Proxy-author
0x0080   697a 6174 696f 6e3a 2042 6173 6963 2063        ization:.Basic.c
0x0090   4756 305a XXXX XXXX XXXX XXXX XXXX XXXX        GV0ZXXXXXXXXXXXX
0x00a0   5445 780d 0a48 6f73 743a 206f 6265 726f        TEx..Host:.obero
0x00b0   6e0d 0a55 7365 722d 4167 656e 743a 204d        n..User-Agent:.M
0x00c0   6f7a 696c 6c61 2f35 2e30 2028 5831 313b        ozilla/5.0.(X11;
0x00d0   2055 3b20 4c69 6e75 7820 6935 3836 3b20        .U;.Linux.i586;.
0x00e0   656e 2d55 533b 2072 763a 302e 392e 3829        en-US;.rv:0.9.8)


Expected Results:  Not sent the Proxy-authorization header - it is certainly no use
to the proxy as it is sent inside an encrypted session that the proxy
concerned cannot decrypt.
yeah, this can be considered somewhat of a security issue.  thanks for pointing
out this bug!  increasing priority... will fix for mozilla 1.0.
Severity: major → critical
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: mozilla1.0
Priority: -- → P1
Target Milestone: --- → mozilla1.0
-> badami
Assignee: darin → badami
Status: ASSIGNED → NEW
Whiteboard: [patch needs r/sr=]
Attached patch this one actually compiles (obsolete) — Splinter Review
Attachment #72353 - Attachment is obsolete: true
Comment on attachment 72358 [details] [diff] [review]
this one actually compiles


>Index: nsHttpChannel.cpp

>-        if (proxyHost)
>+        // bug 127671 
>+        // Do not send proxy auth credentials if we were connecting to an secure URL.
>+        // The proxy will pass these to the destination server and do nothing with it
>+        // since the connection was secure.
>+        PRBool isHttps = PR_FALSE;
>+        mURI->SchemeIs("https", &isHttps);
>+        if (proxyHost && !isHttps)

use mConnectionInfo->UsingSSL() instead
Attachment #72358 - Flags: needs-work+
Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to adt@netscape.com.  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.
Target Milestone: mozilla1.0 → mozilla1.2
Attached patch using mConnectionInfo (obsolete) — Splinter Review
Attachment #72358 - Attachment is obsolete: true
this should not be overlooked for nsbeta1

back -> moz 1.0
Keywords: nsbeta1
Target Milestone: mozilla1.2 → mozilla1.0
vinay: i just realized a fundamental problem with this approach.  proxy ssl
connect may require a password.  try setting frame.packetgram.com:443 as your
SSL proxy.  you'll notice that it requires authentication.  we need a better
solution here... perhaps we need to strip the Proxy-Authorization header after
we make a successful proxy connection.  see where
nsHttpConnection::ProxyStepUp() gets called.  of course, we have to be very
careful touching connection code because it is multithreaded, and the auth cache
is not threadsafe.
Darin
I propose the following:
We add an extra boolean argument to RequestHead::Flatten which has a default
value of FALSE and is set to TRUE only when called from
nsHttpConnection::SetupSSLProxyConnect.
That way the proxy headers would go only when we ask it to, not otherwise.

but it would also need to be TRUE for non-SSL proxy connections.  i'm not sure i
like the idea of adding another parameter to Flatten, but i can't really come up
with any good alternatives off the top of my head.  hmm...
peterg: can you test this w/ other web browsers?  is mozilla the only browser
that fails to get this right?  thx!
Mozilla is indeed the only one that does this, of Netscape 4.77, Opera 5.0, MSIE
5.5 and mozilla. FWIW MSIE was on NT4.0 (SP6 I think), rest on linux 2.2.19 (RH6.2).
per adt, critical for nsbeta1. hence plus.
Keywords: nsbeta1nsbeta1+
Whiteboard: [patch needs r/sr=] → [patch needs r/sr=][adt1]
alternatively, we can pass the proxy authenticate headers to nsHttpConnection as
an attribute.  Then we can set it up as a request header just before we do proxy
connects.
or, inside nsHttpConnection::SetupSSLProxyConnect() you could strip the
Proxy-Authorization from the transaction and re-flatten the transaction.  this
isn't exactly optimally efficient... but this isn't a performance critical path.

    val = trans->RequestHead()->PeekHeader(nsHttp::Proxy_Authorization);
    if (val) {
        // we don't know for sure if this authorization is intended for the
        // SSL proxy, so we add it just in case.
        request.SetHeader(nsHttp::Proxy_Authorization, val);

+       trans->RequestHead()->SetHeader(nsHttp::Proxy_Authorization, nsnull);
+       trans->Reflatten();
    }

you'd of course have to implement nsHttpTransaction::Reflatten().  what do you
think?  there may be issues w.r.t. modifying the request object at this point...
it'll be important to consider who owns that object and who might be inspecting
it (hopefully no one is concurrently modifying it or else we'd have bigger
problems).  i think it should be safe... but we must be careful.
Attachment #72533 - Attachment is obsolete: true
Darin,msoltz
Can i get a r/sr on this please ?
Attachment #72767 - Attachment is obsolete: true
Comment on attachment 72953 [details] [diff] [review]
same as ealier patch - cleaned up indentations

patch still has indentation problems.

consider changing "usingProxy" to "pruneProxyHeaders" to be consistent w/
nsHttpResponseHead::Flatten(..., pruneTransients).

make sure you fix nsHttpResponseHead::Flatten so it calls
nsHttpHeaderArray::Flatten w/ pruneProxyHeaders set to FALSE when
pruneTransients is FALSE.

also, mstoltz is not a necko peer... you should probably ask someone who works
on networking code to give you an r=... that is unless mstoltz feels
comfortable giving you an r=.
Attachment #72953 - Flags: needs-work+
No, I'm not the best person to review this.
Attachment #72953 - Attachment is obsolete: true
Gagan, Rick, bbaetz
Can one of u folks review this please ?
Status: NEW → ASSIGNED
Comment on attachment 73187 [details] [diff] [review]
indentations fixes, change in variable name usingProxy to pruneProxyHeader

>Index: nsHttpHeaderArray.cpp

>+        if (pruneProxyHeaders || ((entry->header != nsHttp::Proxy_Authorization) && 
>+            (entry->header != nsHttp::Proxy_Connection))) {
>+            buf.Append(entry->header);

>+        }

this conditional seems a bit odd... 

  // prune proxy headers if requested
  if (pruneProxyHeaders && (entry->header == nsHttp::Proxy_Authorization ||
			    entry->header == nsHttp::Proxy_Connection))
      continue;
  buf.Append(entry->header)
  ...

might be cleaner and a bit more efficient when pruneProxyHeaders is false.


>Index: nsHttpHeaderArray.h

>-    void Flatten(nsACString &);
>+    void Flatten(nsACString &, PRBool pruneProxyHeaders);

how about making pruneProxyHeaders default to FALSE as you did
for nsHttpRequestHead::Flatten.

otherwise, this patch looks good to me.  make sure to test it thoroughly in
all three configurations: direct connection, proxy connection, SSL proxy
connection.
Attachment #73187 - Attachment is obsolete: true
Comment on attachment 73512 [details] [diff] [review]
with darins comments

sr=darin
Attachment #73512 - Flags: superreview+
Comment on attachment 73512 [details] [diff] [review]
with darins comments

r=rpotts@netscape.com
Attachment #73512 - Flags: review+
Comment on attachment 73512 [details] [diff] [review]
with darins comments

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #73512 - Flags: approval+
Fixed with checkin

D:\mozilla\netwerk\protocol\http\src>cvs commit
cvs commit: Examining .
Checking in nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <--  nsHttpChann
el.cpp
new revision: 1.96; previous revision: 1.95
done
Checking in nsHttpConnection.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpConnection.cpp,v  <--  nsHttpCo
nnection.cpp
new revision: 1.31; previous revision: 1.30
done
Checking in nsHttpHeaderArray.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpHeaderArray.cpp,v  <--  nsHttpH
eaderArray.cpp
new revision: 1.8; previous revision: 1.7
done
Checking in nsHttpHeaderArray.h;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpHeaderArray.h,v  <--  nsHttpHea
derArray.h
new revision: 1.6; previous revision: 1.5
done
Checking in nsHttpRequestHead.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpRequestHead.cpp,v  <--  nsHttpR
equestHead.cpp
new revision: 1.4; previous revision: 1.3
done
Checking in nsHttpRequestHead.h;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpRequestHead.h,v  <--  nsHttpReq
uestHead.h
new revision: 1.6; previous revision: 1.5
done
Checking in nsHttpResponseHead.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpResponseHead.cpp,v  <--  nsHttp
ResponseHead.cpp
new revision: 1.24; previous revision: 1.23
done
Checking in nsHttpTransaction.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpTransaction.cpp,v  <--  nsHttpT
ransaction.cpp
new revision: 1.44; previous revision: 1.43
done
Checking in nsHttpTransaction.h;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpTransaction.h,v  <--  nsHttpTra
nsaction.h
new revision: 1.20; previous revision: 1.19
done
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Did this cause bug 130826 + dupes? (http proxy auth not working)

mConnectionInfo->UsingHttpProxy() && !mConnectionInfo->UsingSSL()

as the prune condition doesn't look right - that will prune the proxy headers
from any http proxy, unless we're talking to an ssl host.

Don't you need:

mConnectionInfo->UsingHttpProxy() && mConnectionInfo->UsingSSL(); ?

Also, the stuff in nsHttpConnection.cpp uses in PR_TRUE, but 3 lines before that
the proxy-auth header is explicitly added, which seems wrong. tht was added for
bug 83288, which fixed ssl proxy auth to work. It may not be needed anymore,
though, in which case it should be removed.

I'm not sure where the proxy auth headers get added, so some of this may be
wrong. I'll see if I can repro 130826 tomorrow, if you can't.
qa to me.

We regress this area too much when we patch. 

Should I try to write up a brief: "(Proxy | HTTP) Auth smoketest" ? Darin's
coomments cover the main scenarios, but I think we need some test server/loging
magic to detect the sending of suprious, unneeded auth headers.

reporter: just for my own curiosity, does the patch at least fix the problem you
described in your own environment?
QA Contact: tever → benc
benc: yes, please... if you could put together a proxy auth test suite that
would be extremely helpful :-)
benc,

Can't tell whether the bug is fixed since I can't CONNECT through my proxy in
the first place (per bug 130826 as bbaetz mentioned). Except of course
to the extent that it definitely isn't sending out passwords any more :)
I think that this needs an additional patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 130826
That patch fixed things for me. (I was suffering from bug #130826) Am able to 
browse around after authenticating with proxy.
Comment on attachment 74288 [details] [diff] [review]
corrections to previous patch

sr=darin
Attachment #74288 - Flags: superreview+
*** Bug 130826 has been marked as a duplicate of this bug. ***
Still doesn't CONNECT out through our firewall (cf. comment #35). I'm
trying with build ID 2002031508 - is this the right thing to do?
peter: are you testing w/ vinay's latest patch?  it hasn't been checked in yet.
 you'll need a debug build to test it.
Darin,
No, I tried with the last nightly build. Wrong move :(
Next chance I get will be Monday at the earliest.
*** Bug 131377 has been marked as a duplicate of this bug. ***
*** Bug 131416 has been marked as a duplicate of this bug. ***
*** Bug 131450 has been marked as a duplicate of this bug. ***
I am using a personal build on Win2K applying the patches in attachment 742888 [details].

This seems to be working ok at least from the point that I can already
authenticate myself to our squid proxy server. Now I can browse again, that was
one nasty regression =)
For those who can't compile, it's still a nasty regression.

Any chance, the fix get checked-in soon.
Comment on attachment 74288 [details] [diff] [review]
corrections to previous patch

r=rpotts@netscape.com
Attachment #74288 - Flags: review+
*** Bug 130273 has been marked as a duplicate of this bug. ***
All that is needed is an approval from the drivers to get this checked-in. Any
chance there are drivers reading this bug... =)
Comment on attachment 74288 [details] [diff] [review]
corrections to previous patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #74288 - Flags: approval+
Fixed with checkin 
D:\mozilla\netwerk\protocol\http\src>cvs commit
cvs commit: Examining .
? bak
Checking in nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <-- 
nsHttpChannel.cpp
new revision: 1.99; previous revision: 1.98
done
Checking in nsHttpConnection.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpConnection.cpp,v  <-- 
nsHttpConnecti
new revision: 1.32; previous revision: 1.31
done
Status: REOPENED → RESOLVED
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
*** Bug 132015 has been marked as a duplicate of this bug. ***
*** Bug 132566 has been marked as a duplicate of this bug. ***
Still having problems here, I'm afraid, when trying to use SSL through our
firewall.

Testing with build ID 2002032006 - this should include the right patches, yes?

No SSL session is started at all. Instead mozilla appears to move on to
believing the SSL session has been set up, after the first attempt to CONNECT.
Peter:

If I interpret your comments correctly - you believe the patch is correct
because your patch build worked correctly, but NOBODY has seen this work in a
daily build since the checkin?
It worked for me with 2002031921.  But I haven't tried
later ones, and I do recall seeing something from someone who still
had trouble.
This still does not work for me. When I try to connect to a secure site, a proxy
authentication dialog just keeps popping up no matter how many times I provide
my username and password. Nothing further happens. I have noticed that I can
make a secure connection when I set the HTTP version to 1.0 instead of the
default 1.1. But this worked even before the problem was supposedly 'fixed'. I
am currently using nightly build 2002040103. This is really a showstopper for me.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Benjamin,

No, I have never tried it with anything other than a nightly build (which did
not work).
John: does "worked for you" mean that you had an environment like peter's, where
you could verify the proxy-auth header only arrived to the proxy? or do you mean
that you were able to connect via an SSL proxy w/ auth w/o a problem?

Peter: based on what you are saying, I'm going to leave this reopened.

(BTW: using a reverse proxy to do https-http conversion is a great trick. I had
forgotten you can do that (I was about to configure something much more painful
for the Netscape labs).

Gerdinand: I think your problem is already reported in other bugs. 
This might be the wrong but, because, when I read this, it doesn't make
much sense to me.  I reported another bug which got reassigned to this one.
What works for me (and what did not work before) is this.  I connect to
the University of Pennsylvania Library "EZ Proxy" and can log in with my
password (which I will give to one person if someone wants to try it, but
only through email, not here).  An example is:
http://proxy.library.upenn.edu:8100/login?url=http://gateway.ovid.com/ovidweb.cgi?T=JS&MODE=ovid&PAGE=main&D=medf
(for Medline).  Information about the EZ Proxy server is here:
http://www.library.upenn.edu/services/computing/proxy/index.html
Before the bug was fixed, I got taken to some page that vaguely implied
(without saying so) that I wasn't accepting cookies.  (But I was.)

Jon (not John)
Jonathan: This bug is a pretty complicated security problem, and probably not
related to your situation.

In you case, you are sending a request to a web server that gets the target URL
for you. This is effectively a reverse proxy (although somewhat more obvious
than most reverse proxy configurations).

The problem described involves configuring your manual proxy prefs, then going
to a normal URL.
peter, jonathon: if you are still experiencing authentication related problems
with a proxy server, then please file a new bug.  it sounds like this bug is not
directly related to the problems you're seeing.  thx!

marking FIXED.
Status: REOPENED → RESOLVED
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
As I've said, I think this bug is different from the one I reported.
The one I reported was re-assigned to this one.  The one I reported
is fixed.  No problems for me.  I just tried a recent new build too:
2002040606.

Jon(athan)
*** Bug 137971 has been marked as a duplicate of this bug. ***
VERIFIED: Mozilla 1.4b (all/plats)

I finally figured how to verify the HTTP headers didn't have the proxy-auth lines.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: