341708 - selfserv is sometimes silent when client aborts handshake in client key exchange

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

The test case for this is :
- build NSS with "more than suite b" (all curves)
- build with Sun studio 11 on Sparc
- use the -fsimple=2 option in freebl/Makefile for the two FPU builds on Solaris Sparc

Run the following :

#!/bin/tcsh
selfserv -D -p 8443 -d ../../../tests_results/security/bigbelly.1/ext_server -n
bigbelly.red.iplanet.com -B -s \
         -e bigbelly.red.iplanet.com-ec -w nss -c :C013

Client :

#!/bin/tcsh
./strsclnt -q -p 8443 -d ../../../tests_results/security/bigbelly.1/ext_client
-w nss -c 1000 -C :C013 -T bigbelly.red.iplanet.com

Only the client will report errors :

strsclnt: -- SSL: Server Certificate Validated.
strsclnt: PR_Send returned error -12219:
Unspecified failure while processing SSL Client Key Exchange handshake.

The server should also report errors, but it's totally silent.

Comment 1

[Julien Pierre]

The error reported by the client is SSL_ERROR_CLIENT_KEY_EXCHANGE_FAILURE .

The reason selfserv is silent is the following code :

  961           if (rv == 0 ||
  962               (rv < 0 && PR_END_OF_FILE_ERROR == PR_GetError())) {
  963               if (verbose)
  964                   errWarn("HDX PR_Read hit EOF");
  965               break;
  966           }

In this case, rv is -1 and the error is PR_END_OF_FILE_ERROR . But by default selfserv doesn't print it.
However, this does not seem like the appropriate error in this case. I know the error is caused by a problem in the server during the ECC computations. So, the server should report something more severe.
I'll run ssltap and report.

Comment 2

[Julien Pierre]

The client is failing in ssl3ecc.c in the following code :

  365       pms = PK11_PubDeriveWithKDF(privKey, svrPubKey, PR_FALSE, NULL, NULL,
  366                               CKM_ECDH1_DERIVE, target, CKA_DERIVE, 0,
  367                               CKD_NULL, NULL, NULL);
  368
  369       if (pms == NULL) {
  370           ssl_MapLowLevelError(SSL_ERROR_CLIENT_KEY_EXCHANGE_FAILURE);
  371           goto loser;
  372       }

The problem is that it received a bad EC public key from the server, and the key derivation fails deep down in softoken in EC_ValidatePublicKey.

Unfortunately, the client sent no alert to the server to notify it of the problem with its key. It just closed the connection. The bug is in the client for not sending an alert. The ssltap output is below :

--> [
alloclen = 30 bytes
(30 bytes of 30)
 [Tue Jun 27 19:11:11 2006] [ssl2]  ClientHelloV2 {
           version = {0x03, 0x00}
           cipher-specs-length = 3 (0x03)
           sid-length = 0 (0x00)
           challenge-length = 16 (0x10)
           cipher-suites = {
                (0x00c013) TLS/ECDHE-RSA/AES128-CBC/SHA
                }
           session-id = { }
           challenge = { 0x82db 0xf97d 0xd9db 0xba4b 0xb8e5 0xe6ac 0x28ab 0xd6b6 }
}
]
<-- [
(2878 bytes of 2873)
SSLRecord { [Tue Jun 27 19:11:11 2006]
   type    = 22 (handshake)
   version = { 3,0 }
   length  = 2873 (0xb39)
   handshake {
      type = 2 (server_hello)
      length = 70 (0x000046)
         ServerHello {
            server_version = {3, 0}
            random = {...}
            session ID = {
                length = 32
                contents = {..}
            }
            cipher_suite = (0xc013) TLS/ECDHE-RSA/AES128-CBC/SHA
            compression method = 00
         }
      type = 11 (certificate)
      length = 2604 (0x000a2c)
         CertificateChain {
            chainlength = 2601 (0x0a29)
            Certificate {
               size = 654 (0x028e)
               data = { saved in file 'cert.001' }
            }
            Certificate {
               size = 652 (0x028c)
               data = { saved in file 'cert.002' }
            }
            Certificate {
               size = 645 (0x0285)
               data = { saved in file 'cert.003' }
            }
            Certificate {
               size = 638 (0x027e)
               data = { saved in file 'cert.004' }
            }
         }
      type = 12 (server_key_exchange)
      length = 183 (0x0000b7)
      type = 14 (server_hello_done)
      length = 0 (0x000000)
   }
}
]
Read EOF on Client socket. [Tue Jun 27 19:11:11 2006]
Read EOF on Server socket. [Tue Jun 27 19:11:11 2006]
Connection 1 Complete [Tue Jun 27 19:11:11 2006]

Comment 3

[Julien Pierre]

Attachment: make client send "illegal parameter" alert after key derivation error

Comment 4

[Julien Pierre]

After adding the patch attached, the server reports :

selfserv: HDX PR_Read returned error -12226:
SSL peer rejected a handshake message for unacceptable content.

Comment 5

[Robert Relyea]

You should probably not check in the ssl3con.c patch with the #if 1's in it. Using CERT_DecodeDERCert in this context could be fraught with  problems (CERT_DecodeDERCert does not return a full CERTCertificate, so if you do anything other than look at decoded certificate fields bad things(TM) could happen).

bob

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 227343 [details] [diff] [review]
make client send "illegal parameter" alert after key derivation error

r+ for the ssl3ecc.c part of this patch.
r- for the ssl3con.c part.

This patch makes the client send the expected alert, which is good.  
But perhaps the server should convert the EOF into an SSL handshake error code when it occurs during the handshake. 
Consider that some other clients don't ever send alerts.  It would be unfortunate if NSS-based servers never reported errors for those clients.

Comment 7

[Julien Pierre]

Yes, of course I only meant to attach the patch in ssl3ecc.c . The other patch was meant for isolating problems with race conditions in the Stan cache.

I checked in this patch to the tip .

Checking in ssl3ecc.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ecc.c,v  <--  ssl3ecc.c
new revision: 1.14; previous revision: 1.13
done

I will wait until the NSS_3_11_BRANCH reopens for checkins before closing this bug.

Re: comment 6, that sounds like a good idea. I'll see if I can figure out the state machine to make it do the right thing.

Comment 8

[Julien Pierre]

Sorry, I reassigned this in error.

Comment 9

[Julien Pierre]

Comment on attachment 227343 [details] [diff] [review]
make client send "illegal parameter" alert after key derivation error

Wan-Teh,

Since this didn't get checked in to NSS_3_11_BRANCH yet, I'm requesting a second review . The request is only for the ssl3ecc.c file . Please ignore the change to ssl3con.c . Thanks.

Comment 10

[Wan-Teh Chang]

Comment on attachment 227343 [details] [diff] [review]
make client send "illegal parameter" alert after key derivation error

r=wtc.  But I have a question: why is illegal_parameter the right
alert message for this error condition?  Why don't we use, for
example, decrypt_error?

   decrypt_error
       A handshake cryptographic operation failed, including being
       unable to correctly verify a signature, decrypt a key exchange,
       or validate a finished message.

I also found that ssl3con.c:sendDHClientKeyExchange and
ssl3ecc.c:ssl3_HandleECDHClientKeyExchange have the same
code:
    /*  Determine the PMS */
    pms = PK11_PubDerive[WithKDF](...);

    if (pms == NULL) {
        ...
    }

Should they also send an alert message?

Comment 11

[Wan-Teh Chang]

Let me broaden my second question above: should we sent an alert
message in all the places in ssl3con.c and ssl3ecc.c where we
have "if (pms == NULL)", including the RSA handshake as well?
Run the command
    grep "pms == NULL" *
in the lib/ssl directory to find these places.

Comment 12

[Julien Pierre]

Wan-Teh,

Nelson had suggested I use illegal_parameter when I wrote the patch. I see that the decrypt_error alert is only valid for TLS, but not SSL3 . It simplifies the code not to use decrypt_error since we don't have to do a version check . 

I think the other places might need an alert as well. Nelson ?

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #11)
> Let me broaden my second question above: should we sent an alert
> message in all the places in ssl3con.c and ssl3ecc.c where we
> have "if (pms == NULL)", including the RSA handshake as well?

No!  Not in *all* those places.
That would ressurect Bleichenbacher's "million question" attack.
But there are undoubtedly *some* places where we could.
More study is required to determine the places where we should.

Comment 14

[Julien Pierre]

I have checked in this patch to NSS_3_11_BRANCH since it had 2 reviews, and to be consistent with the tip .

Checking in ssl3ecc.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ecc.c,v  <--  ssl3ecc.c
new revision: 1.3.2.9; previous revision: 1.3.2.8
done

It sounds like there is more work left to do, based on comment 6 and comments 10/11 . 

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #10)
> But I have a question: why is illegal_parameter the right
> alert message for this error condition?  Why don't we use, for
> example, decrypt_error?

In my view, a distinction between decrypt_error and illegal_parameter is 
whether the problem is due to a bad key, or bad signature / ciphertext block.
When the key used for verifying the signature, or doing the decryption,
is known to be good, but the results are bad, then it's clearly a candidate
for decrypt_error.  But when the key itself is suspect, then IMO the better
error condition is illegal parameter (that parameter being the key).

In this case, IIRC, the problem we encountered was that the key received from
the server contained a point that was not on the curve, and hence was an 
invalid key.  The key itself was not decrypted in any way.  The failure 
occured while trying to use the key to (effectively) encrypt (derive, actually)
with that key, and the fault was entirely due to the validity of the key.
So, I think illegal_parameter was correct in this case.  

But I observe that there are many potential reasons this function could 
fail, and perhaps it is not right to ignore the actual cause when determining
the reason code.

If you wanted to send decrypt_error for TLS (where it is legal), 
you could trivially make this change:

-    SSL3AlertDescription desc  = illegal_parameter;
+    SSL3AlertDescription desc  = isTLS ? decrypt_error : illegal_parameter;


> I also found that ssl3con.c:sendDHClientKeyExchange and
> ssl3ecc.c:ssl3_HandleECDHClientKeyExchange have the same code:

Send*ClientKeyExchange happens in the client.  
In the client, it is safe to alert for a bad PMS.  

Handle*ClientKeyExchange happens in the server. 
Alerting there will ressurect the million question attack.  

In general, I think it is safe to send alerts just about anywhere, 
except in the server for problems that occur with PMS or MS derivation.  
Servers must not be quick to shortcut the handshake over PMS or MS derivation 
failures.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Julien committed this last June.  
I am marking it resolved/fixed now.