587234 - Better error reporting for tiny ephemeral Diffie-Hellman (DHE) keys in Server Key Exchange

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Attachment: Add SSL_ERROR_WEAK_SERVER_KEY (checked in)

The proposed patch adds a new error code
SSL_ERROR_WEAK_SERVER_KEY for the dh_p size
check in ssl3_HandleServerKeyExchange.

Note that Firefox will display the string
    ssl_error_weak_server_key
in its error page. So the name of this error
code needs to be descriptive enough and needs
to make it clear the server is at fault.

We can consider adding a new error code for
invalid dh_g and dh_Ys parameters in the
same function.  Right now we use the default
SSL_ERROR_RX_MALFORMED_SERVER_KEY_EXCH, which
seems appropriate for invalid DH parameters.
Would SSL_ERROR_SERVER_KEY_EXCHANGE_FAILURE
be better?

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 465905 [details] [diff] [review]
Add SSL_ERROR_WEAK_SERVER_KEY (checked in)

r=nelson

Comment 2

[Kaspar]

Comment on attachment 465905 [details] [diff] [review]
Add SSL_ERROR_WEAK_SERVER_KEY (checked in)

>+ER3(SSL_ERROR_WEAK_SERVER_KEY,                 (SSL_ERROR_BASE + 115),
>+"SSL received a weak key in Server Key Exchange handshake message.")

Adding precise error codes is a very useful thing, but WEAK_SERVER_KEY
might be somewhat too broad, in this case. Will it only be used for
weak DHE keys? There are other places where weak key checks would
make sense, IMO, and these would be additional places where
WEAK_SERVER_KEY would apply, too.

From some very limited and cursory testing, NSS will e.g. allow to
connect to a server with a 480-bit RSA key, using an RSA key exchange:

> tstclnt: SSL version 3.1 using 128-bit AES with 160-bit SHA1 MAC
> tstclnt: Server Auth: 480-bit RSA, Key Exchange: 480-bit RSA

Shouldn't NSS refuse such keys in a similar way, and return
WEAK_SERVER_KEY as well? Or would you use SSL_ERROR_BAD_CERTIFICATE
for this case?

Comment 3

[Wan-Teh Chang]

Kaspar: thanks for your comment.  I intend to use
SSL_ERROR_WEAK_SERVER_KEY for other types of weak
server keys.  If you know of places in the NSS
source tree that should report the new
SSL_ERROR_WEAK_SERVER_KEY error code, please let
me know or attach a patch.  Thanks!

Comment 4

[Wan-Teh Chang]

Comment on attachment 465905 [details] [diff] [review]
Add SSL_ERROR_WEAK_SERVER_KEY (checked in)

Checked in on the NSS trunk (NSS 3.13) and
NSS_3_12_BRANCH (NSS 3.12.8).

Checking in cmd/lib/SSLerrs.h;
/cvsroot/mozilla/security/nss/cmd/lib/SSLerrs.h,v  <--  SSLerrs.h
new revision: 1.11; previous revision: 1.10
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.145; previous revision: 1.144
done
Checking in lib/ssl/sslerr.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslerr.h,v  <--  sslerr.h
new revision: 1.12; previous revision: 1.11
done

Checking in cmd/lib/SSLerrs.h;
/cvsroot/mozilla/security/nss/cmd/lib/SSLerrs.h,v  <--  SSLerrs.h
new revision: 1.10.2.1; previous revision: 1.10
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.142.2.2; previous revision: 1.142.2.1
done
Checking in lib/ssl/sslerr.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslerr.h,v  <--  sslerr.h
new revision: 1.11.2.1; previous revision: 1.11
done

Comment 5

[Kaspar]

Attachment: Reject certificates with weak keys (proof of concept only)

(In reply to comment #3)
> Kaspar: thanks for your comment.  I intend to use
> SSL_ERROR_WEAK_SERVER_KEY for other types of weak
> server keys.

Ok, I probably failed to make my point in comment 2: in that case, the wording "in Server Key Exchange handshake message" seems to specific to me.

> If you know of places in the NSS
> source tree that should report the new
> SSL_ERROR_WEAK_SERVER_KEY error code, please let
> me know or attach a patch.

I'm attaching a proof of concept for ssl3_HandleCertificate. Just quickly hacked up, but this illustrates a case where SSL_ERROR_WEAK_SERVER_KEY seems a good error code, but the error description doesn't really fit (we reject the key before getting to the ServerKeyExchange message). - Not sure if assigning errCode is enough at that point (or should we also PORT_SetError?), and we would probably be better of defining a constant for those (currently hardcoded) 512 bits.

Comment 6

[Wan-Teh Chang]

Kaspar: I can change the error message to
"The SSL server uses a weak cryptographic key."
How does that sound?

Comment 7

[Kaspar]

(In reply to comment #6)
> Kaspar: I can change the error message to
> "The SSL server uses a weak cryptographic key."
> How does that sound?

I think that's fine, yes. I can see four points in the handshake where we should check for weak keys:

1) when receiving a Certificate message from the server
2) when receiving a ServerKeyExchange message (from the server)
3) when receiving a Certificate message from the client
4) when receiving a ClientKeyExchange message (from the client)

I.e., we would use SSL_ERROR_WEAK_SERVER_KEY for 1 and 2, and possibly add a separate error code for 3 and 4.

Considering the preliminary version of my patch for ssl3_HandleCertificate: would you prefer a separate bug for this, or do we want to keep it here (and change the subject accordingly)?

Also, what about increasing the minimum length for "strong" asymmetric keys to 1024? The latest NIST draft 800-131 (http://csrc.nist.gov/groups/ST/toolkit/documents/draftSP800-130_June15_2010.pdf) already considers 1024-bit keys "acceptable through 2010" only, and deprecates them "from 2011".

Comment 8

[Wan-Teh Chang]

Kaspar: if you think you can finish the additional work within 2-3
weeks, then we can just use this bug.  Otherwise, it's better to
open a new bug.

Since the key size checks in NSS will result in a hard failure,
we need to be very conservative.  Otherwise we'll need to add
new functions for applications to specify their minimum key
sizes.  An application can perform additional key size checks
at the completion of the handshake and decide what to do
(abort the connection or show a broken lock icon, etc.).  We
should also find out if NSS has a function that returns the
server's ephemeral DH public key.

Comment 9

[Kaspar]

Attachment: Reject more weak server keys, patch v1

(In reply to comment #8)
> Kaspar: if you think you can finish the additional work within 2-3
> weeks, then we can just use this bug.

Ok, this should be doable. I suggest we concentrate on the client side for the time being and address cases 1) and 2) from comment 7. I will adapt the bug subject accordingly.

> Since the key size checks in NSS will result in a hard failure,
> we need to be very conservative.

I agree, but also to spur some discussion (Nelson, Kai?), I put 1024 into the current version of the patch. The URL given in comment 7 is incorrect, BTW, it should be http://csrc.nist.gov/publications/drafts/800-131/draft-sp800-131_spd-june2010.pdf instead.

> We should also find out if NSS has a function that returns the
> server's ephemeral DH public key.

What exactly do you mean by this? Note that libSSL only does DHE, no DH - what we are checking in ssl3_HandleServerKeyExchange/ssl3_HandleECDHServerKeyExchange are ephemeral keys, only.

Some additional notes:

- I have added the reworded error description (as suggested in comment 6) to the patch

- when rejecting a weak key and we're speaking TLS, then my patch changes the alert to insufficient_security (if it's SSL, then either send bad_cert or illegal_parameter)

- the ss->sec.peerKey assignment in ssl3ecc.c has been removed on purpose, as it saves us from setting it to NULL when we detect a weak key (and destroy peerKey). There's already another assignment further down in the code, which is completely sufficient.

- I didn't add any checks for the SSLv2 code so far - does anybody think it's worth the effort?

Comment 10

[Kaspar]

Adapting the subject to better reflect what we're focusing on.

Comment 11

[Wan-Teh Chang]

Kaspar: thanks for the patch.  I will review it soon.

What I meant by
    We should also find out if NSS has a function that
    returns the server's ephemeral DH public key.
is that an application such as Mozilla Firefox may want
to show a broken lock icon if the server's ephemeral DH
key size is >= 512 bits (hence allowed by libSSL) but
< 1024 bits, so if NSS has a function that returns either
the server's ephemeral DH public key or its size, it'll
enable an application to do additional key size checks.

Don't add checks for the SSLv2 code.  (I want to remove
the SSLv2 code from the NSS trunk soon.)

Comment 12

[Kaspar]

Attachment: Reject more weak server keys, patch v2

I realized that I missed one case with v1 of my patch - the one of ephemeral RSA keys. SSL 3.0 and TLS 1.0 (but not 1.1 and later) also allow sending temporary RSA keys in the ServerKeyExchange message. In patch v2, I have therefore added a similar check for the kt_rsa case in ssl3_HandleServerKeyExchange.

Speaking of a conservative-but-sensible value for SSL_MIN_PUBKEY_STRENGTH, I can add at least one recent data point, from Ivan Ristic's Internet SSL survey (https://community.qualys.com/docs/DOC-1421): of the about 600,000 certs he collected, only about 0.5% (~3000) had a key with less than 1024 bits.

(In reply to comment #11)
> What I meant by
>     We should also find out if NSS has a function that
>     returns the server's ephemeral DH public key.
> is that an application such as Mozilla Firefox may want
> to show a broken lock icon if the server's ephemeral DH
> key size is >= 512 bits (hence allowed by libSSL) but
> < 1024 bits, so if NSS has a function that returns either
> the server's ephemeral DH public key or its size, it'll
> enable an application to do additional key size checks.

I see. SSL_GetChannelInfo() is what you want - its keaKeyBits member gives that information (note that you need SSL_GetCipherSuiteInfo() as well, for getting at keaTypeName/keaType, especially when ECC is enabled).

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

> SSL_GetChannelInfo() is what you want.
Yes, See http://mxr.mozilla.org/security/ident?i=SSL_GetChannelInfo for 
examples of its use.

Comment 14

[Wan-Teh Chang]

Attachment: Reject more weak server keys, patch v3

Kaspar: thanks for the patch.  Here is my "counter proposal".
Most of my changes are self-explanatory.  Additional notes:

1. I like sending the insufficient_security alert, but it
is specified to be sent by servers only.

2. I still need to review your change to ssl3_HandleCertificate
carefully.  For now, I just added an "if (!isServer)" test
for setting the SSL_ERROR_WEAK_SERVER_KEY error.  This does
show the need for an SSL_ERROR_WEAK_CLIENT_KEY error, or the
error code should be renamed SSL_ERROR_WEAK_PEER_KEY or
SSL_ERROR_WEAK_PUBLIC_KEY.

Comment 15

[Kaspar]

Comment on attachment 467770 [details] [diff] [review]
Reject more weak server keys, patch v3

Thanks for your review, Wan-Teh. Not sure I'm the best person to review this "counter proposal" (which is still quite similar to patch v2, after all), but anyway, I'm giving it r- for the time being:

- I'm aware of the 512 bit upper bound for RSA_EXPORT key exchanges, but I think we can still keep SSL_MIN_PUB_KEY_BITS at 1024, if that's your only reason for going down to 512: in ssl3_HandleCertificate, when dealing with the kt_rsa case, simply replace "modulus.len * BPB < SSL_MIN_PUB_KEY_BITS" by "modulus.len != EXPORT_RSA_KEY_LENGTH".

- The "if (!isServer)" line is redundant, all the code I added is already part of an "if (!ss->sec.isServer)" block (I assume that both conditions are equivalent).

- I don't understand why you lowered SSL_MIN_EC_PUB_KEY_BITS to 112... we're checking key sizes for TLS with this code, and none of the curves listed in RFC 4492 have less than 160 bits - neither do any of those available in NSS. (Note that in the NIST documents, a strength of "112 bits" actually means 224-bit EC keys, while 160-bit EC keys are equivalent to "80 bits of security strength", as shown in table 2 of the SP 800-131 draft.)

And yes, it's true that there's a need for SSL_ERROR_WEAK_CLIENT_KEY (see also comment 9, we're only covering cases 1 and 2). I suggest do deal with this in a separate bug, that's why I added "server [keys]" to the subject.

Comment 16

[Wan-Teh Chang]

Comment on attachment 467770 [details] [diff] [review]
Reject more weak server keys, patch v3

Kaspar: thanks for your comments.

The 512 bits come from two sources:

1. TLS 1.0 RFC 2246, Appendix C, pages 61-62:

      Key
      Exchange
      Algorithm       Description                        Key size limit

      DHE_DSS         Ephemeral DH with DSS signatures   None
      DHE_DSS_EXPORT  Ephemeral DH with DSS signatures   DH = 512 bits
      DHE_RSA         Ephemeral DH with RSA signatures   None
      DHE_RSA_EXPORT  Ephemeral DH with RSA signatures   DH = 512 bits,
                                                         RSA = none
      DH_anon         Anonymous DH, no signatures        None
      DH_anon_EXPORT  Anonymous DH, no signatures        DH = 512 bits

      DH_DSS          DH with DSS-based certificates     None
      DH_DSS_EXPORT   DH with DSS-based certificates     DH = 512 bits
      DH_RSA          DH with RSA-based certificates     None
      DH_RSA_EXPORT   DH with RSA-based certificates     DH = 512 bits,
                                                         RSA = none
      NULL            No key exchange                    N/A
      RSA             RSA key exchange                   None
      RSA_EXPORT      RSA key exchange                   RSA = 512 bits

   Key size limit
       The key size limit gives the size of the largest public key that
       can be legally used for encryption in cipher suites that are
       exportable.

Note: NSS only supports RSA_EXPORT, so your suggestion of testing
modulus.len != EXPORT_RSA_KEY_LENGTH would prevent NSS from
contradicting itself.

2. The "openssl dhparam" command generates 512-bit DH parameters
by default: http://www.openssl.org/docs/apps/dhparam.html

So it is risky to reject 512-bit ephemeral DH public keys today.
I will add this to the comment in the patch.

If you know how to file an OpenSSL bug report, please file one
to have the default changed to 1024 bits.

The 112 bits for EC public keys come from these NSS functions:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/cryptohi/seckey.c&rev=1.57&mark=1548,1557-1558#1546

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/cryptohi/seckey.c&rev=1.57&mark=1245-1247,1249-1251,1231#1230

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/freebl/ecl/ecl-curve.h&rev=1.2.28.2&mark=585-586,624,625,634,635#585

NSS supports those curves if you compile NSS with
NSS_ECC_MORE_THAN_SUITE_B=1.

Comment 17

[Kaspar]

(In reply to comment #16)
> Note: NSS only supports RSA_EXPORT, so your suggestion of testing
> modulus.len != EXPORT_RSA_KEY_LENGTH would prevent NSS from
> contradicting itself.

Yes, I'm proposing that for RSA_EXPORT, we require a size of 512 bits in the ServerKeyExchange message. Do you agree that this is a workable solution, or do you disagree? (Note that both TLS 1.0 and TLS 1.1 disallow ServerKeyExchange messages for RSA, so we only have to deal with the RSA_EXPORT case in ssl3_HandleServerKeyExchange.)

> 2. The "openssl dhparam" command generates 512-bit DH parameters
> by default: http://www.openssl.org/docs/apps/dhparam.html
> 
> So it is risky to reject 512-bit ephemeral DH public keys today.

That doesn't really sound like a convincing argument to me. How many admins of OpenSSL based server software generate these parameters by hand? mod_ssl e.g. uses built-in temporary parameters by default (with both 512 and 1024 bits). 

> If you know how to file an OpenSSL bug report, please file one
> to have the default changed to 1024 bits.

I'll check rt.openssl.org and/or the mailing list and see if this has already been brought up ("openssl genrsa" also has DEFBITS 512 - even with 1.0.0 -, and would be another candidate for an upgrade).

> NSS supports those curves if you compile NSS with
> NSS_ECC_MORE_THAN_SUITE_B=1.

Ok, that's true. But still: a) we're adding these checks to libSSL, and no TLS-related RFC ever allowed ECC keys smaller than 160 bits and b) I'm somewhat doubtful if NSS still compiles with NSS_ECC_MORE_THAN_SUITE_B these days (and even if it does, what's wrong with having SSL_MIN_EC_PUB_KEY_BITS at 160?).

Comment 18

[Wan-Teh Chang]

Let's not let the good ideas in this patch (minimum key size
checks and better error reporting) get bogged down by a long
debate on the exact minimum key sizes.

As a general-purpose SSL library, NSS's libSSL needs to
strike a balance between "secure by default" and "allowing
experiments with various key sizes".  This is why I started
with less secure minimum key sizes, intended only to detect
horrible mistakes such as 256-bit DH parameters.  Once the
checks are in place, we can easily increase the minimum key
sizes.

Comment 19

[Wan-Teh Chang]

Attachment: Reject more weak server keys, patch v4

The comment justifying the minimum key sizes still needs
work, but I'm attaching this patch as a checkpoint.

Kaspar, you're right that all the elliptic curves used
for SSL are at least 160 bits.  I forgot that NSS defines
some curves that can't be used for SSL.

The EC public key size checks are actually redundant
because we perform key size checks implicitly  when we
check the elliptic curve is one of the supported ones.

Comment 20

[Kaspar]

Thanks for patch v4, Wan-Teh. I guess we now need an additional reviewer, as both of us have worked on the code (patch v4 is missing the change to SSLErrs.h, btw).

As for the minimum key sizes, my proposal is still:

  #define SSL_MIN_PUB_KEY_BITS 1024
  #define SSL_MIN_EC_PUB_KEY_BITS 160

... for the following reasons:

- it's in line with the current NIST recommendations of using a minimum strength of 80 bits (~1024 bits for RSA/DH keys, ~160 bits for EC keys) in 2010      

- the SSL world has been / is moving away from 1024 (see e.g. the EV guidelines - by 1 January 2011, the minimum size will be at 2048)

- 512 bits were already considered weak 10 years ago - after all, that was the reason of that original US export control limit (to limit the "power" of the exported products). About 8 years have passed since many of the restrictions have been lifted (so most toolkits now hopefully do strong crypto), and if we wanted to compare this with current limits in US export regulations, they are now at 64/768/128 (http://www.access.gpo.gov/bis/ear/txt/ccl5-pt2.txt).

- for ECC, it's true that the check for the supported curves currently obviates the need for enforcing a minimum of 160, but I would prefer to add that code nevertheless, as it makes it easier to raise that limit in later versions of NSS. (Note that wouldn't make a difference between NSS_ENABLE_ECC and NSS_ECC_MORE_THAN_SUITE_B, 160 is suitable for both.)

- I doubt that many application developers using NSS will really add more strict checks based on SSL_GetChannelInfo etc. (not even the Mozilla clients have done so in the past). NSS should use settings which prevent the use of weak keys out of the box - I can't see a good argument why anybody would still want to use certificates with 512-bit RSA keys these days. Also, it feels quite strange that NSS would reject a 504-bit RSA key with SSL_ERROR_WEAK_SERVER_KEY ("SSL server attempted to use a weak key"), but a 512-bit key would work fine... that's not the message which NSS should convey to its users.

As an aside, I would also propose to rip out the RSA_EXPORT stuff from libSSL. I'm willing to work on this (in a separate bug), provided that there is a generel agreement on this among the NSS team members.

Comment 21

[Wan-Teh Chang]

Attachment: Rename error code to be specific about key type (checked in)

We can add more error codes when we check the sizes
of other key types.

Comment 22

[Wan-Teh Chang]

Comment on attachment 470115 [details] [diff] [review]
Rename error code to be specific about key type (checked in)

I checked in this trivial patch, to be reviewed by kaie, on the
NSS trunk (NSS 3.13) and NSS_3_12_BRANCH (NSS 3.12.8).

Checking in cmd/lib/SSLerrs.h;
/cvsroot/mozilla/security/nss/cmd/lib/SSLerrs.h,v  <--  SSLerrs.h
new revision: 1.12; previous revision: 1.11
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.148; previous revision: 1.147
done
Checking in lib/ssl/sslerr.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslerr.h,v  <--  sslerr.h
new revision: 1.13; previous revision: 1.12
done

Checking in cmd/lib/SSLerrs.h;
/cvsroot/mozilla/security/nss/cmd/lib/SSLerrs.h,v  <--  SSLerrs.h
new revision: 1.10.2.2; previous revision: 1.10.2.1
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.142.2.4; previous revision: 1.142.2.3
done
Checking in lib/ssl/sslerr.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslerr.h,v  <--  sslerr.h
new revision: 1.11.2.2; previous revision: 1.11.2.1
done

Comment 23

[Wan-Teh Chang]

Marked the bug fixed.  Changed the bug's summary back to the
original to reflect what was done in NSS 3.12.8.