331977 - No error dialog is presented if an SSL rehandshake fails

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P2)

Description

[Rob Crittenden]

This was discovered using Apache 2.0.54 with mod_nss 1.0. I have reproduced using a simple NSS server program. It also affects Apache with mod_ssl.

What happens is:

1. Browser connects to server
2. SSL negotiation occurs
3. Connection is successful
4. Client issues request to server
5. Server responds with a request for re-handshake, reducing the number of available ciphers to 1 that the client doesn't support (it's turned off)
6. The SSL rehandshake fails due to no common ciphers

The browser simply repaints the screen. No error message is displayed and in fact, it is hard to tell that any communication occured at all.

I discussed this with Nelson Bolyard from the NSS team. I will attach the ssltap output he refers to here as well as the NSS server program I used.

-------- begin Nelson's comments --------

Here's what I see, the boiled down version of the ssltap:

>> Looking up "darlene"...
>> Proxy socket ready and listening
>> Connection #1 [Wed Feb 22 16:27:06 2006]
>> Connected to darlene:8000
>> --> [
>>  [Wed Feb 22 16:27:06 2006] [ssl2]  ClientHelloV2 {
>>            version = {0x03, 0x01}
>>            cipher-suites = { 
>>                 (0x000004) SSL3/RSA/RC4-128/MD5

client sends client hello, using SSL2 format, listing a bunch of suites.
RSA_RC4-128_WITH_SHA1 is noticably absent from that list.

>> <--
>> SSLRecord { [Wed Feb 22 16:27:06 2006]
>>    type    = 22 (handshake)
>>    version = { 3,1 }
>>       type = 2 (server_hello)
>>             cipher_suite = (0x0004) SSL3/RSA/RC4-128/MD5

Server responds normally, using TLS (SSL 3.1) and choosing this suite.
Rest of first handshake goes completely normally, by the book, and so
is left out here.  Now, the first handshake is done, and the https client
sends its https request (below).  In the SSL protocol, this is
"application data".

>> -->
>> SSLRecord { [Wed Feb 22 16:27:08 2006]
>>    type    = 23 (application_data)
>>    version = { 3,1 }
>>    length  = 406 (0x196)
>>             < encrypted >

And the server responds (below) with an encrypted handshake message.
It's encrypted so we can't see its real contents, but there's only one
handshake message it can be.   Once a handshake finishes on a connection,
the only SSL record types that can be sent after that are, application
data, an error alert, or the server can send a "hello request" handshake
message, or the client can send another "client hello" handshake message.
So, we can be confident that the encrypted handshake message below is a
"hello request" from the server.

>> <-- [
>> SSLRecord { [Wed Feb 22 16:27:08 2006]
>>    0: 16 03 01 00  14                                   |.....
>>    type    = 22 (handshake)
>>    version = { 3,1 }
>>    length  = 20 (0x14)
>>             < encrypted >

Here (below) is the client's answer to that hello request.  It's a
handshake message, not an alert, not application data, so there's only
one handshake message it can be, a "client hello".  Unlike the first
client hello seen above, this client hello will be in SSL3/TLS format,
not in SSL2 format.  It presumably will contain the same list of
supported cipher suites, except that it will not include any SSL2
suites.  It presumably also attempts to negotiate the same version of
SSL, namely 3.1 (TLS), although we cannot deduce that from this output.

>> --> [
>> SSLRecord { [Wed Feb 22 16:27:08 2006]
>>    0: 16 03 01 00  61                                   |....a
>>    type    = 22 (handshake)
>>    version = { 3,1 }
>>    length  = 97 (0x61)
>>             < encrypted >

And here (below) is the server's response.  It's an alert.  It's very short.
It's undoubtedly an error alert.  (as opposed to a close-notify alert)

>> <-- [
>>    0: 15 03 01 00  12 c9 42 bd  a0 49 e9 79  ef 11 6d 5a  | ......B..I.y..mZ
>>   10: 79 4e 87 bb  a3 32 a3                             |yN...2.
>> (23 bytes of 18)
>> SSLRecord { [Wed Feb 22 16:27:08 2006]
>>    0: 15 03 01 00  12                                   |.....
>>    type    = 21 (alert)
>>    version = { 3,1 }
>>    length  = 18 (0x12)
>>             < encrypted >

According to the server output you sent to me, at this time in the
(failed) handshake, the server output this log message:

>>>> handshake is 0 result is -1.
>>>> Re-negotiation handshake failed -12286: Cannot communicate securely with
>>>> peer: no common encryption algorithm(s).

That pretty well confirms that the failure was due to the mismatch of
supported cipher suites. (Although, it could conceivably be a protocol
version mismatch, but I doubt it.)  And it tells us that the alert which
the server sent to the client (above) should be either "protocol_version"
or "handshake_failure".

Finally the client sends an alert to the server.  Unfortunately, we cannot
tell from this whether the client SSL code read the above alert first,
or whether it merely closed the socket.  But more on that below.

>> --> [
>> SSLRecord { [Wed Feb 22 16:27:08 2006]
>>    0: 15 03 01 00  12                                   |.....
>>    type    = 21 (alert)
>>    version = { 3,1 }
>>    length  = 18 (0x12)
>>             < encrypted >
>> Read EOF on Client socket. [Wed Feb 22 16:27:08 2006]

The client would have been waiting for a read to finish, that is, it
would likely have been in PR_Poll, waiting for an indication that the
socket had become "readable" or had gone into an "exception" state.
When either of those happened, the client should have called PR_Read
(or PR_Recv) again, and should have gotten one of these two error codes:
SSL_ERROR_NO_CYPHER_OVERLAP (if server sent "handshake_failure") or
SSL_ERROR_PROTOCOL_VERSION_ALERT (if the server sent "protocol_version").

Rob wrote:

>>>> The trouble is that if the rehandshake fails the client gets no data
>>>> back, apparently.
>>>> The mozilla browsers simply refresh the status bar.
>>>> Konqueror reports a connection error and IE 6 sits and spins.
>>>> It seems to work the same way with openssl, the connection just sort
>>>> of silently closes.

Well, having seen the ssltap output, I think there are only a few
possibilities here:

1. The SSL socket became ready, the browser did a read on it and got
   SSL_ERROR_NO_CYPHER_OVERLAP, but the browser didn't report it.

2. The SSL socket became ready, the browser did a read on it and got
   SSL_ERROR_PROTOCOL_VERSION_ALERT, but the browser didn't report it.

3. The SSL socket became ready, the browser did a read on it and got
   a zero-length result, indicating EOF, instead of getting one of the
   above error codes.

4. Somehow the client decided that socket had come to EOF, without
   having done a PR_Read on it, and so the browser closed the socket
   without ever having seen either of the above two error codes.

5. The SSL socket never became ready, PR_Poll never told the browser
   that the socket was ready, and the operation timed out.  Then the
   browser closed the socket without ever doing the final PR_Read on
   it, so the browser never saw either of the above error codes.

6. Some other error that I can't imagine. :-)

I think we can rule out #5, because the timestamps in the ssltap output
show that the client's alert was sent in the same second as the server's
alert was received.  So, a timeout is unlikely.

I think we can rule out #2, because there's no reason to think that the
client or server changed the set of enabled SSL protocol version numbers
between the two handshakes (or IS THERE?).

#3 seems unlikely, because the SSL code would have to read and process the
server's alert before seeing EOF on the socket.  If there is some means
by which the SSL code can fail to return the error code shown above,
and return EOF instead, that would be a very serious error.

One possiblity occurs to me, though.  If the browser did a PR_Read and got
one of the above error codes, and ignored that error and called read again,
it would get EOF on that second read. It would not get that error code again.
But I think this is just another way of saying case #1 above.

I can't think of any way that #4 could happen.

I think the most likely explanation is some combination of #1 and #3.

The crucial question is: what the browser's last PR_Read/PR_Recv call
on that socket return?  EOF?  One of the SSL errors?
If it returned one of the right values, then it seems the browser must
have ignored it.  If it returned a wrong value, then that's an NSS bug.

If debugging the browser is too difficult, you might try using NSS's
tstclnt program, sending the same https request using it.  That is a
non-blocking client (as is the browser), so it might produce meaningful
results.

------- end Nelson's comments -------

Note that I did try the NSS tstclnt program and it returns the correct error response, no common ciphers.

Here is the full output from the NSS program. Note that this affects newer browsers as well, I tested today using a build of "Bon Echo 2.0a1, Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20060328 Firefox/2.0a1" built by Kai Engert.

% ./server -d alias -p 8000 -n Server-Cert -c httptest

Ready to accept client connection:

reading from client
Cipher used is RC4, key size is 128, secret key size is 128
Security is ON
Client cert issued by no certificate, for no certificate
Read from the client 'GET / HTTP/1.1
Host: darlene:8000
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.2) Gecko/20040805 Netscape/7.2
Accept: text/xml,application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5
Accept-Language: en,fr;q=0.5
Accept-Encoding: gzip,deflate
Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7
Keep-Alive: 300
Connection: keep-alive

'
Performing full renegotation
handshake is 0 result is -1.
Re-negotiation handshake failed -12286: Cannot communicate securely with peer: no common encryption algorithm(s).
Handshake failed

Ready to accept client connection:

Comment 1

[Rob Crittenden]

Attachment: output from NSS utility ssltap

Comment 2

[Rob Crittenden]

Attachment: NSS sample server program

In order to build this you'll need the header files from the NSS sample server in mozilla/security/nss/cmd/SSLsample/

Comment 3

[Kai Engert (:KaiE:)]

error can be reproduced by visiting https://ecc.fedora.redhat.com/TLS_ECDH_RSA_WITH_RC4_128_SHA/

Comment 4

[Kai Engert (:KaiE:)]

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

Comment 5

[Kai Engert (:KaiE:)]

Nelson, thanks a lot for this analysis.

The reason why we do not report this error code to the user: it happens later than our PSM code expects.

Actually, our current PSM code only reports SSL data transfer errors to the user:
- at the time we expect the initial SSL/TLS data transfer over a socket
or
- at the time we receive the "bad cert" callback notification and are to make the decision, whether to continue or not

I do not know why this limitation was introduced in the past.

I assume it is a good idea to report SSL errors, regardless at what time they appear during the SSL socket lifetime.

Comment 6

[Kai Engert (:KaiE:)]

Attachment: Patch v1 (ignores whitespace changes for easier reviewing)

This patch will report an SSL error at any time during the SSL socket lifetime.

Comment 7

[Kai Engert (:KaiE:)]

Attachment: full version of Patch v1

Comment 8

[Robert Relyea]

Comment on attachment 227670 [details] [diff] [review]
Patch v1 (ignores whitespace changes for easier reviewing)

r+ = relyea

I just looks like the original problem was a bug, rather than an intential attempt to hide errors in certain cases.

One minor nit, in the PR_WOULD_BLOCK_ERROR if block the code which sets handleHandshakeResultNow to FALSE is now redundant. The could should be marginally easier to read if you remove it and it' preceding comment.

bob

Comment 9

[Kai Engert (:KaiE:)]

Attachment: Patch v2 (ignores whitespace changes for easier reviewing)

addressed Bob's change request

Comment 10

[Kai Engert (:KaiE:)]

Attachment: full version of Patch v2

Comment 11

[Kai Engert (:KaiE:)]

Comment on attachment 227729 [details] [diff] [review]
Patch v2 (ignores whitespace changes for easier reviewing)

checked in to trunk, this correct fix should go to the 1.8 branch

Comment 12

[Kai Engert (:KaiE:)]

fixed1.8.1