1173413 - export-1024 ciphersuites with 1024-bit server certificates are broken

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Kai Engert (:KaiE:)]

This bug was found when testing NSS 3.19.1 on RHEL 6, which has some backwards compatibility modification.

The only modification that matters here is the recent change to certutil, which changed the default RSA key size created to 2048.

If certutil is changed to create 1024 bit keys, then the NSS test suite fails with export-1024 ciphersuites.

In bug 1086145, in kea_defs, kea_rsa_export_1024 was defined as ephemeral.

If the server has a 1024 bit cert, the server concludes that it doesn't need to send a server key exchange message.

However, the client side code in ssl3_HandleServerHello is defined to always expect a key server key exchange message, if the ciphersuite is defined as ephemeral, and sets the wait state to wait_server_key.

As a result, when the client enforces the use of the TLS_RSA_EXPORT1024_WITH_DES_CBC_SHA or TLS_RSA_EXPORT1024_WITH_RC4_56_SHA ciphers, 
the client fails with SSL_ERROR_RX_UNEXPECTED_HELLO_DONE.

The client side doesn't know if a server key exchange message will follow, until it receives the server certificate.

Comment 1

[Martin Thomson [:mt:]]

Hmm, more reason to fast track the removal of export cipher suites.

It would appear that the same problem happens with regular export cipher suites and certificates with 512 bit keys.

My question is: fix or just note that this particular scenario will fail?  It's relatively easy to fix, but it's going to be fairly kludgy.  I guess the former if we haven't announced the end-of-life for those awfully insecure cipher suites already.

Comment 2

[Martin Thomson [:mt:]]

Attachment: bug1173413.patch

This should fix the problem.

Comment 3

[Martin Thomson [:mt:]]

Attachment: certutil1024.patch

I tested with `NSS_SSL_RUN=cov ./ssl.sh` with this patch added.   Prior to the fix being applied, it failed, afterwards it passed.  Not for checkin :)

Comment 4

[Yuhong Bao]

Well, they are at least better than the 40-bit/512-bit RSA cipher suites. It is unfortunate that OpenSSL disabled them at compile time back in 2006. This reminds me of a bug where IBM HTTP Server never sent the ServerKeyExchange message for them regardless of certificate key length, which is not a problem for SChannel/IE but is for Netscape/NSS.

Comment 5

[Wan-Teh Chang]

Comment on attachment 8623761 [details] [diff] [review]
bug1173413.patch

Review of attachment 8623761 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.

::: lib/ssl/ssl3con.c
@@ +6096,5 @@
>      /* enforce limits on kea key sizes. */
>      if (ss->ssl3.hs.kea_def->is_limited) {
> +        unsigned int keyLen = SECKEY_PublicKeyStrengthInBits(serverKey);
> +
> +        if (keyLen > ss->ssl3.hs.kea_def->key_size_limit) {

This change is an unrelated cleanup, right?

@@ +10070,5 @@
>  	    SECKEY_DestroyPublicKey(pubKey); 
>  	    pubKey = NULL;
>      	}
>  
> +        /* Ephemeral suites RequireServerKeyExchange, unless they are an export

Typo: "RequireServerKeyExchange" should be two words: require ServerKeyExchange

@@ +10074,5 @@
> +        /* Ephemeral suites RequireServerKeyExchange, unless they are an export
> +         * cipher suite and the authentication key fits within the limit. */
> +        if (ss->ssl3.hs.kea_def->ephemeral &&
> +            !(ss->ssl3.hs.kea_def->is_limited &&
> +              ss->sec.authKeyBits <= ss->ssl3.hs.kea_def->key_size_limit)) {

IMPORTANT: I am tempted to suggest we also require that
ss->ssl3.hs.kea_def->signKeyType == sign_rsa before checking
 ss->sec.authKeyBits <= ss->ssl3.hs.kea_def->key_size_limit.

I believe the export1024 cipher suites are specified in
https://tools.ietf.org/html/draft-ietf-tls-56-bit-ciphersuites-01#section-5

The original export cipher suites are specified in
https://tools.ietf.org/html/rfc2246#section-7.4.3

Your conditional expression seems to match the specifications.
(I didn't check carefully.)

Comment 6

[Yuhong Bao]

Yea, I don't think NSS/Netscape or for that matter IE/SChannel did DHE_RSA_EXPORT or DHE_DSS_EXPORT, right?

Comment 7

[Kai Engert (:KaiE:)]

Martin, thanks a lot for identifying this fix.

Wan-Teh, the first part of Martin's fix avoids a rounding mistake. The old rounded up to full bytes, then converted back to bits again. As a result, the new code seems to more precisely check if the key needs to be rejected as too big.

Comment 8

[Kai Engert (:KaiE:)]

(In reply to Wan-Teh Chang from comment #5)
> IMPORTANT: I am tempted to suggest we also require that
> ss->ssl3.hs.kea_def->signKeyType == sign_rsa before checking
>  ss->sec.authKeyBits <= ss->ssl3.hs.kea_def->key_size_limit.


The table kea_defs in ssl3con.c lists multiple entries that are limited, but which aren't using sign_rsa.

Do you think Martin's general comparison would be incorrect in the non-sign_rsa scenarios?

Comment 9

[Martin Thomson [:mt:]]

Attachment: bug1173413.patch

I realized that Wan-Teh's suggestion was good, but not quite correct.  If you look at the table[1], the key exchange algorithm is what is important here.  Only RSA key exchange is affected by this problem.

[1] https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c#287-312

Comment 10

[Robert Relyea]

Comment on attachment 8629508 [details] [diff] [review]
bug1173413.patch

Review of attachment 8629508 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Martin, r+ rrelyea

Comment 11

[Wan-Teh Chang]

Comment on attachment 8629508 [details] [diff] [review]
bug1173413.patch

Review of attachment 8629508 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.

::: lib/ssl/ssl3con.c
@@ +10346,5 @@
> +         * export cipher suite, with RSA key exchange, and the authentication
> +         * key fits within the key size limit. */
> +        if (ss->ssl3.hs.kea_def->ephemeral &&
> +            !(ss->ssl3.hs.kea_def->is_limited &&
> +              ss->ssl3.hs.kea_def->exchKeyType == ssl_kea_rsa &&

The check I suggested was ss->ssl3.hs.kea_def->signKeyType == sign_rsa.
I agree we should check ss->ssl3.hs.kea_def->exchKeyType == ssl_kea_rsa
instead because it covers both kea_rsa_export and kea_rsa_export_1024.

Ideally we should check
  (ss->ssl3.hs.kea_def->kea == kea_rsa_export ||
   ss->ssl3.hs.kea_def->kea == kea_rsa_export_1024)
but your check is equivalent and more compact.

Comment 12

[Kai Engert (:KaiE:)]

Comment on attachment 8629508 [details] [diff] [review]
bug1173413.patch

If this passes the test suite with certutil creating 1024 keys (as you already said), then I'm happy and Bob's and Wan-Teh's reviews are sufficient.
Thanks Martin!

Comment 13

[Wan-Teh Chang]

(In reply to Wan-Teh Chang from comment #11)
>
> Ideally we should check
>   (ss->ssl3.hs.kea_def->kea == kea_rsa_export ||
>    ss->ssl3.hs.kea_def->kea == kea_rsa_export_1024)
> but your check is equivalent and more compact.

Martin:

Actually it is
  (ss->ssl3.hs.kea_def->is_limited &&
   ss->ssl3.hs.kea_def->exchKeyType == ssl_kea_rsa)
that is equivalent to
  (ss->ssl3.hs.kea_def->kea == kea_rsa_export ||
   ss->ssl3.hs.kea_def->kea == kea_rsa_export_1024)

So you can also consider this:

  if (ss->ssl3.hs.kea_def->ephemeral &&
      !((ss->ssl3.hs.kea_def->kea == kea_rsa_export ||
         ss->ssl3.hs.kea_def->kea == kea_rsa_export_1024) &&
        ss->sec.authKeyBits <= ss->ssl3.hs.kea_def->key_size_limit)) {

But you may find this harder to understand.

Comment 14

[Robert Relyea]

I think Martins formulation is clearer.

Comment 15

[Wan-Teh Chang]

Attachment: bug1173413.patch, alternative solution

Martin: I finally realized that it is the current definition
of the "ephemeral" boolean field that makes the conditional
expression a little hard to understand. I asked you to mark
the RSA_EXPORT and RSA_EXPORT1024 key exchange methods as
ephemeral based on the assumption that they always require
ServerKeyExchange. That assumption is wrong. I believe that
if we modify the definition of the ephemeral boolean field
accordingly, the conditional expression will be easier to
understand. Here is an alternative patch.

Also, could you please confirm that the changes to
ssl3_SendClientKeyExchange are unrelated to this bug?
That code enforces the key size limit on the client
authentication key.

Comment 16

[Martin Thomson [:mt:]]

Comment on attachment 8631056 [details] [diff] [review]
bug1173413.patch, alternative solution

Review of attachment 8631056 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, that is better.

Comment 17

[Robert Relyea]

Comment on attachment 8631056 [details] [diff] [review]
bug1173413.patch, alternative solution

Review of attachment 8631056 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, this is better, thanks wtc.

In the initial patch I almost commented that we need to change ephemeral from true to false, but then I realized the test was a negative test to prevent sending the key exchange when it wasn't needed rather than a positive test to send it when it was....

Comment 18

[Wan-Teh Chang]

Comment on attachment 8631056 [details] [diff] [review]
bug1173413.patch, alternative solution

Martin: please go ahead and check in this patch because it is really
your work. Thanks.