149868 - Fix PSM's proxy logic

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P2)

Description

[Kai Engert (:KaiE:)]

Fix PSM's proxy logic

Comment 1

[Kai Engert (:KaiE:)]

The following are comments from bugscape 12934, where connecting to a secure
site over proxy did not work, but delayed very long and finally showed error -12263.


nelsonb said:

"If you contact an SSL2 only server with a client that does not send
ssl2-compatible client hello messages, you get precisely this behavior.
Note that disabling SSL2 does not (er, should not) disable the sending
of ssl2-compatible client hello messages for SSL3 and TLS.

So, I'd guess that your browser had disabled SSL2-compatibile client
hellos.  That's a bad thing to do for https servers.  https servers
should, IMO, always have ssl2-compatible client hellos enabled. 

Only certain protocols (i.e. other than https) that are known to never
use ssl2 should ever disable ssl2-compatible client hellos.

This seems like a PSM bug.

...

Apparently, this user's browser was attempting to visit an https server
(http over SSL) and the SSL socket was configured to disable SSL v2 
compatible client hellos.  IMO, that should never happen; that is, 
all SSL sockets used by http (as opposed to those used by, say IMAP)
should always have SSL v2 compatible client hellos enabled.  Whatever
code is disabling SSL v2 compatible client hellos is in error (IMO)
in the context of https.  

I'd guess that some code has confused the meanings and purposes of the 
libSSL socket options SSL_ENABLE_SSL2 and SSL_V2_COMPATIBLE_HELLO.
I'd guess that the user had disabled SSL2 (using the GUI), and as a 
result, the code had disabled SSL_V2_COMPATIBLE_HELLO, perhaps because
the programmer thought (incorrectly) that this is necessary to disable 
the SSL v2 protocol.  

Of course, if SSL v2 is disabled, the user is not going to succesfully 
be able to read email from an ssl-v2-only https server such as 
https://aolmail.aol.com/, but at least the user will not experience a hung 
connection, as this user did.  Instead, the user should get back a message
saying something like "The browser and this server cannot communicate
securely because they have no SSL ciphers in common".  That's what 
C4.7x says when you visit this site with SSL2 disabled.

...

BTW, PSM has a class called a "TLSSocket" which IMO is sadly misnamed.  
This socket type exists to be used in protocols that do "Start-TLS"
(and hence should be called StartTLSSocket).  Start-TLS is a means for
starting TLS (and/or SSL3) on a TCP connection that is already in use
without SSL/TLS.  In order to do Start-TLS, it _IS_ necessary to 
disable SSL_V2_COMPATIBLE_HELLO.  AFAIK, this is the ONLY occasion on
which it is appropriate to disable SSL_V2_COMPATIBLE_HELLO, and it 
should only be done for protocols that do the Start-TLS, such as IMAP.  
https NEVER uses Start-TLS, and https sockets should never use 
"StartTLSSocket" sockets.  

Note that the Start-TLS protocol is sometimes erroneously described 
in the PSM code with the words "SSL Step UP" or "TLS Step UP".  That is 
unfortunate.  "Step Up" is a feature of the SSL and TLS protocols and
has NOTHING to do with Start-TLS, so it is unfortunate that these terms
are confused in PSM.

...

One more important clarification.  
Going through a proxy is NOT the same as using Start-TLS! 

A proxy (e.g. socks or http) does an initial handshake with the client,
and thereafter passes all data through without interpretation.  This
is true of SOCKS proxies, and of any proxy that implements the CONNECT
protocol.  

For such proxies, the decision of whether or not to use SSL_V2_COMPATIBLE_HELLO
is purely a function of the protocol being spoken by the final server.  If you 
disable SSL_V2_COMPATIBLE_HELLO when not using a proxy, then you would also 
disable it when using a proxy.  If you enable SSL_V2_COMPATIBLE_HELLO when not 
using a proxy, then you would also enable it when using a proxy.  So, https 
always should use SSL_V2_COMPATIBLE_HELLO, whether using a proxy or not.   
If IMAPS doen't SSL_V2_COMPATIBLE_HELLO when not using a proxy, then it also 
will not use SSL_V2_COMPATIBLE_HELLO with a proxy."


kaie said:

"Looking at Nelsons explanations, chatting with Nelson, and looking at the
current Mozilla+PSM code, I come to the following statements:

PSM currently only has one place in the code, where it tries to disable SSL2
compatible hellos.

PSM is currently not making a difference between connections that use STARTTLS
or switch a proxy connection to SSL/TLS. It prepares sockets the same way.

With STARTTLS, it is necessary to disable SSL_V2_COMPATIBLE_HELLOS.
With https over a proxy, there is no SSL protocol restriction for the https part.

Currently, PSM is unable to distinguish, whether STARTTLS or a "protocol over
proxy" is happening.

In order to solve the problem of this bug, and to not break anything else (smtp
over ssl), we must add the ability to distinguish.

Whenever the application is going to do STARTTLS, PSM should disable SSL2
compatible hellos, and because there is no need to use SSL2, it should disable
SSL2 at all.

Whenever the application is intending to speak SSL over a proxy, it should not
disable any SSL2 options (unless the user in general disabled SSL2), in order to
be able to speak with any server.

...

I made a simple change, and did neither disable SSL2 nor the "hellos".
By doing so, I could successfully connect to the site through a proxy.

But we can't take that simple fix, because it breaks SMTP-over-SSL."


nelsonb said:

"Kai, 
are you proposing to always leave ssl2 fully enabled when using a proxy?  
What effect will this have on (say) imaps or smpts over a proxy?
Or are we certain that imaps and smpts _never_ use a proxy?"


kaie said:

"At least I was unable to find code in the tree that would use a proxy with email
protocols.

But even if it were used, I suggest an implementation that would support that.

I did not say, to leave it always enabled, whenever a proxy it detected.
I wanted to say, I don't care whether a proxy is used or not.
I would not do anything special when a proxy is used.

But if the application informs PSM, that STARTTLS will be used on the socket, we
will disable SSL2 and the compatible hellos, regardless of whether a proxy is used."

Comment 2

[Kai Engert (:KaiE:)]

It turns out that PSM code is not as wrong as I thought.
I have hope that the fix is much simpler.


Confusion 1)
What is called "tlsstepup" in PSM should be called "starttls".


Confusion 2)
"tlsstepup" aka "starttls" has nothing to do with proxies.


Result of Confusion:

In bug 89500 you changed how http connections use SSL. When you detect that a
proxy is configured, you use a "tlsstepup" socket.

But using "tlsstepup" (aka "starttls") must not be used with https, because it
disables a SSL protocol option (v2 compatible hello), which is required for
https to work correctly.

Comment 3

[Kai Engert (:KaiE:)]

Attachment: Patch to http protocol handler / never use start tls logic (currently uncorrectly named as tlsstepup)

Using this patch, I see correct behaviour when trying to connect to a SSL v2
only site using a proxy.

Testcase: 
- have a proxy for SSL configured
- go to https://aolmail.aol.com

Without the patch, connecting does not succeed, but seems to be stale.

With the patch, and having the SSL2 pref enabled, connecting succeeds.

With the patch, and having the SSL2 pref disabled, the correct error message is
shown.

Comment 4

[Stephane Saux]

Kai, Darin.  Is this something we want for RTM?

Comment 5

[Kai Engert (:KaiE:)]

I think we should definitively take it for RTM.
- We know that there are many problems with https over proxies, and from what I
learned from Nelson this patch is the right thing to do.
- Testing the known bugs with https over proxies seem all to be fixed by this patch.
- This patch can not have any impact besides https over proxies, so I consider
it low risk.

Darin, Bradley, can you please r/sr?

If you want me to, I can provide test builds for Windows and Linux having this
patch applied, and we can ask QA to test.

Comment 6

[Bradley Baetz (:bbaetz)]

Comment on attachment 86789 [details] [diff] [review]
Patch to http protocol handler / never use start tls logic (currently uncorrectly named as tlsstepup)

r=bbaetz

kaie explained this to me. I can't remember why I changed this in bug 89500 -
if I could, I'd get kaie to change the docs I must have seen.

Comment 7

[Darin Fisher]

Comment on attachment 86789 [details] [diff] [review]
Patch to http protocol handler / never use start tls logic (currently uncorrectly named as tlsstepup)

sr=darin

Comment 8

[Darin Fisher]

kaie: can you please trim the { }'s that are no longer required.  thx!

Comment 9

[Kai Engert (:KaiE:)]

Attachment: Updated fix

This has just two unnecessary brackets removed, as requested by Darin.

Comment 10

[Kai Engert (:KaiE:)]

Comment on attachment 87199 [details] [diff] [review]
Updated fix 

Carrying forward r=bbaetz and sr=darin

Comment 11

[Kai Engert (:KaiE:)]

I'd say, let's land it for RTM.

Comment 12

[Calum Mackay]

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

Comment 13

[Kai Engert (:KaiE:)]

Checked in to trunk.

Comment 14

[Paul Wyskoczka]

John, can you verify this fix on the trunk.  Thx.

Comment 15

[Kai Engert (:KaiE:)]

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

Comment 16

[Kai Engert (:KaiE:)]

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

Comment 17

[Kai Engert (:KaiE:)]

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

Comment 18

[John Unruh]

Verified on trunk.

Comment 19

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) approval for checkin on the 1.0 branch. pls check
this in asap, then add the keyword "fixed1.0.1".

Comment 20

[Kai Engert (:KaiE:)]

actually changing keyword to +

Comment 21

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 22

[Kai Engert (:KaiE:)]

Checked in to 1_0 branch.

Comment 23

[John Unruh]

verified on 6/20 branch.

Comment 24

[Matthias Versen [:Matti]]

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

Comment 25

[John Unruh]

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