127671 - Proxy-authorization header incorrectly sent to server during SSL session

Status: VERIFIED FIXED

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

Description

[Peter Greenwood]

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.

Comment 1

[Darin Fisher]

yeah, this can be considered somewhat of a security issue.  thanks for pointing
out this bug!  increasing priority... will fix for mozilla 1.0.

Comment 2

[Darin Fisher]

-> badami

Comment 3

[Vinay Badami]

Attachment: if url was https do not send proxy auth headers

Comment 4

[Vinay Badami]

Attachment: this one actually compiles

Comment 5

[Darin Fisher]

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

Comment 6

[scottputterman]

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 [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 7

[Vinay Badami]

Attachment: using mConnectionInfo

Comment 8

[Darin Fisher]

this should not be overlooked for nsbeta1

back -> moz 1.0

Comment 9

[Darin Fisher]

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.

Comment 10

[Vinay Badami]

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.

Comment 11

[Darin Fisher]

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...

Comment 12

[Darin Fisher]

peterg: can you test this w/ other web browsers?  is mozilla the only browser
that fails to get this right?  thx!

Comment 13

[Peter Greenwood]

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).

Comment 14

[Gagan]

per adt, critical for nsbeta1. hence plus.

Comment 15

[Vinay Badami]

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.

Comment 16

[Darin Fisher]

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.

Comment 17

[Vinay Badami]

Attachment: with a smarter flatten method on RequestHead and HeaderArray.

Comment 18

[Vinay Badami]

Attachment: same as ealier patch - cleaned up indentations

Darin,msoltz
Can i get a r/sr on this please ?

Comment 19

[Darin Fisher]

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=.

Comment 20

[Mitchell Stoltz (not reading bugmail)]

No, I'm not the best person to review this.

Comment 21

[Vinay Badami]

Attachment: indentations fixes, change in variable name usingProxy to pruneProxyHeader

Comment 22

[Vinay Badami]

Gagan, Rick, bbaetz
Can one of u folks review this please ?

Comment 23

[Darin Fisher]

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.

Comment 24

[Vinay Badami]

Attachment: with darins comments

Comment 25

[Darin Fisher]

Comment on attachment 73512 [details] [diff] [review]
with darins comments

sr=darin

Comment 26

[rpotts (gone)]

Comment on attachment 73512 [details] [diff] [review]
with darins comments

r=[e-mail]

Comment 27

[Asa Dotzler [:asa]]

Comment on attachment 73512 [details] [diff] [review]
with darins comments

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 28

[Vinay Badami]

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

Comment 29

[Bradley Baetz (:bbaetz)]

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.

Comment 30

[benc]

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?

Comment 31

[Darin Fisher]

benc: yes, please... if you could put together a proxy auth test suite that
would be extremely helpful :-)

Comment 32

[Peter Greenwood]

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 :)

Comment 33

[Vinay Badami]

I think that this needs an additional patch.

Comment 34

[Vinay Badami]

Attachment: corrections to previous patch

Comment 35

[Harshal Pradhan]

That patch fixed things for me. (I was suffering from bug #130826) Am able to 
browse around after authenticating with proxy.

Comment 36

[Darin Fisher]

Comment on attachment 74288 [details] [diff] [review]
corrections to previous patch

sr=darin

Comment 37

[Darin Fisher]

*** Bug 130826 has been marked as a duplicate of this bug. ***

Comment 38

[Peter Greenwood]

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?

Comment 39

[Darin Fisher]

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.

Comment 40

[Peter Greenwood]

Darin,
No, I tried with the last nightly build. Wrong move :(
Next chance I get will be Monday at the earliest.

Comment 41

[Matthias Versen [:Matti]]

*** Bug 131377 has been marked as a duplicate of this bug. ***

Comment 42

[Aleksey Nogin]

*** Bug 131416 has been marked as a duplicate of this bug. ***

Comment 43

[Aleksey Nogin]

*** Bug 131450 has been marked as a duplicate of this bug. ***

Comment 44

[RFLazaro]

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 =)

Comment 45

[Marc Boullet]

For those who can't compile, it's still a nasty regression.

Any chance, the fix get checked-in soon.

Comment 46

[rpotts (gone)]

Comment on attachment 74288 [details] [diff] [review]
corrections to previous patch

r=[e-mail]

Comment 47

[Darin Fisher]

*** Bug 130273 has been marked as a duplicate of this bug. ***

Comment 48

[RFLazaro]

All that is needed is an approval from the drivers to get this checked-in. Any
chance there are drivers reading this bug... =)

Comment 49

[Asa Dotzler [:asa]]

Comment on attachment 74288 [details] [diff] [review]
corrections to previous patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 50

[Vinay Badami]

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

Comment 51

[Darin Fisher]

*** Bug 132015 has been marked as a duplicate of this bug. ***

Comment 52

[Marc Boullet]

*** Bug 132566 has been marked as a duplicate of this bug. ***

Comment 53

[Peter Greenwood]

Attachment: Annotated tcpdump log of attempted ssl connection

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.

Comment 54

[benc]

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?

Comment 55

[Jonathan Baron]

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.

Comment 56

[Gerdinand]

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.

Comment 57

[Peter Greenwood]

Benjamin,

No, I have never tried it with anything other than a nightly build (which did
not work).

Comment 58

[benc]

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. 

Comment 59

[Jonathan Baron]

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)

Comment 60

[benc]

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.

Comment 61

[Darin Fisher]

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.

Comment 62

[Jonathan Baron]

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)

Comment 63

[Marc Boullet]

*** Bug 137971 has been marked as a duplicate of this bug. ***

Comment 64

[benc]

VERIFIED: Mozilla 1.4b (all/plats)

I finally figured how to verify the HTTP headers didn't have the proxy-auth lines.