412833 - TLS intolerant server condition is concluded prematurely

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

When an attempt to connect to a server using a TLS client hello fails, 
PSM marks the server as "TLS intolerant", which causes all future handshake
attempts to use an alternative "fall back" handshake (currently an SSL v3
handshake in SSLv2 compatible format).  PSM decides that all future 
handshake attempts will use the alternative handshake, EVEN IF the 
alternative handshake never succeeds.  

Consequently, if the user is trying to visit a server that (say) only 
understands TLS client hellos and does not understand the alternative 
fall back handshake, and the user is unlucky enough that the first TLS 
handshake attempt fails, the user must restart the browser to get it to 
ever try a TLS handshake again.

I can imagine these solutions to this problem:

1) Have a way to try an alternative handshake once, and don't mark the 
server as always requiring the alternative handshake until an alternative
handshake succeeds.  Kai points out that this requires changes to the 
code path for successful handshakes, as well as in the error paths.

2) Toggle the "TLS intolerant" flag on every failure.  That is, if an 
attempt to connect with TLS fails, set the TLS intolerant flag and retry.  
If an attempt to connect with SSL3 fails, clear the TLS intolerant flag
and retry.  Limit the number of retries to 1 (may be difficult).

3) Like 2, but if an attempt to connect with SSL3 fails, clear the TLS
intolerant flag, but do not immediately retry.  This solves the issue 
with limiting the number of retries, I think.

Comment 1

[Honza Bambas (:mayhemer)]

I like option 3. There is no need for a counter. Natural is to check TLS and then SSL on and on (on user demand by pressing F5) until user decides to give up. The option 3 will give us this behavior. When we fail to connect with TLS, try with SSL. If we fail with SSL as well, tell a user about it with easy "Try Again" option. User tries again to connect with TLS and then SSL if that fails until we connect if it was just a link congestion.

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: v1

Implemented the option 3. The logic is:

- TLS connection succeeded => remember the site as TLS tolerant

or

- TLS connection failed with an intolerant err => remember the site as intolerant but only if that site has not already been marked as tolerant
- SSL connection (for TLS intolerant server) failed with an intolerant err => unmark that site as TLS intolerant

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 386844 [details] [diff] [review]
v1

First, my usual disclaimer:  There's so much about Firefox's C++ 
conventions and classes that is unknown to me that I scarcely feel
qualified to give a full r+ to this Firefox code.  (I feel more 
qualified to give r- than r+ :)

Now some comments:

1. This new function is unused.

>+void
>+nsSSLIOLayerHelpers::forgetPossibleTLSProblemSite(PRFileDesc* ssl_layer_fd, nsNSSSocketInfo *socketInfo)
>+{
>+  nsCAutoString key;
>+  getSiteKey(socketInfo, key);
>+
>+  removeIntolerantSite(key);
>+}

2. There is only one caller of this new function, and it passes (si->mFd, si).
Is there any foreseeable circumstance in which the first argument would be
a value other than the second argument's mFd variable?  
If not, maybe this method should just take one argument, the nsNSSSocketInfo*.

>+void
>+nsSSLIOLayerHelpers::rememberTolerantSite(PRFileDesc* ssl_layer_fd, nsNSSSocketInfo *socketInfo)

3. The new code below uses a magic number.  Why 16?  Why not 1 or 64?
At a minimum, a comment should provide a rationale.

>   mTLSIntolerantSites = new nsCStringHashSet();
>   if (!mTLSIntolerantSites)
>     return NS_ERROR_OUT_OF_MEMORY;
> 
>   mTLSIntolerantSites->Init(1);
> 
>+  mTLSTolerantSites = new nsCStringHashSet();
>+  if (!mTLSTolerantSites)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  mTLSTolerantSites->Init(16);

None of these issues are "must be changed" problems, so I won't give r-.

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1.1 [Checkin comment 5]

(In reply to comment #3)
> (From update of attachment 386844 [details] [diff] [review])
> First, my usual disclaimer:  There's so much about Firefox's C++ 
> conventions and classes that is unknown to me that I scarcely feel
> qualified to give a full r+ to this Firefox code.  (I feel more 
> qualified to give r- than r+ :)

You mean brace indention and so? :)

> 
> Now some comments:
> 
> 1. This new function is unused.
> 
> >+void
> >+nsSSLIOLayerHelpers::forgetPossibleTLSProblemSite(PRFileDesc* ssl_layer_fd, nsNSSSocketInfo *socketInfo)
> >+{
> >+  nsCAutoString key;
> >+  getSiteKey(socketInfo, key);
> >+
> >+  removeIntolerantSite(key);
> >+}

Removed.

> 
> 2. There is only one caller of this new function, and it passes (si->mFd, si).
> Is there any foreseeable circumstance in which the first argument would be
> a value other than the second argument's mFd variable?  
> If not, maybe this method should just take one argument, the nsNSSSocketInfo*.
> 
> >+void
> >+nsSSLIOLayerHelpers::rememberTolerantSite(PRFileDesc* ssl_layer_fd, nsNSSSocketInfo *socketInfo)
> 

I leave it as is because nsSSLIOLayerHelpers cannot access mFd private member of nsNSSSocketInfo. Caller does.

> 3. The new code below uses a magic number.  Why 16?  Why not 1 or 64?
> At a minimum, a comment should provide a rationale.
> 
> >   mTLSIntolerantSites = new nsCStringHashSet();
> >   if (!mTLSIntolerantSites)
> >     return NS_ERROR_OUT_OF_MEMORY;
> > 
> >   mTLSIntolerantSites->Init(1);
> > 
> >+  mTLSTolerantSites = new nsCStringHashSet();
> >+  if (!mTLSTolerantSites)
> >+    return NS_ERROR_OUT_OF_MEMORY;
> >+
> >+  mTLSTolerantSites->Init(16);
> 

Hmm.. I just want to preallocate the hash table array because most secure servers will be TLS tolerant and we don't want to reallocate the table on each connection to a new server. There is no rule for this, just 16 seems reasonable.

> None of these issues are "must be changed" problems, so I won't give r-.

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 387847 [details] [diff] [review]
v1.1 [Checkin comment 5]

http://hg.mozilla.org/mozilla-central/rev/5f27eedf78c9