Bug 1138554 - Raising minimum key size on DH and RSA to 1023, r=wtc
--- a/lib/cryptohi/keyhi.h
+++ b/lib/cryptohi/keyhi.h
@@ -32,16 +32,21 @@ extern SECStatus SECKEY_CopySubjectPubli
/*
** Update the PQG parameters for a cert's public key.
** Only done for DSA certs
*/
extern SECStatus
SECKEY_UpdateCertPQG(CERTCertificate * subjectCert);
+/*
+** Return the number of bits in the provided big integer. This assumes that the
+** SECItem contains a big-endian number and counts from the first non-zero bit.
+*/
+extern unsigned SECKEY_BigIntegerBitLength(const SECItem *number);
/*
** Return the strength of the public key in bytes
*/
extern unsigned SECKEY_PublicKeyStrength(const SECKEYPublicKey *pubk);
/*
** Return the strength of the public key in bits
--- a/lib/cryptohi/seckey.c
+++ b/lib/cryptohi/seckey.c
@@ -173,18 +173,18 @@ SECKEY_CreateRSAPrivateKey(int keySizeIn
*/
SECKEYPrivateKey *
SECKEY_CreateDHPrivateKey(SECKEYDHParams *param, SECKEYPublicKey **pubk, void *cx)
{
SECKEYPrivateKey *privk;
PK11SlotInfo *slot;
if (!param || !param->base.data || !param->prime.data ||
- param->prime.len < 512/8 || param->base.len == 0 ||
- param->base.len > param->prime.len + 1 ||
+ SECKEY_BigIntegerBitLength(¶m->prime) < DH_MIN_P_BITS ||
+ param->base.len == 0 || param->base.len > param->prime.len + 1 ||
(param->base.len == 1 && param->base.data[0] == 0)) {
PORT_SetError(SEC_ERROR_INVALID_ARGS);
return NULL;
}
slot = PK11_GetBestSlot(CKM_DH_PKCS_KEY_PAIR_GEN,cx);
if (!slot) {
return NULL;
@@ -936,71 +936,86 @@ SECKEY_ECParamsToBasePointOrderLen(const
return 570;
default:
PORT_SetError(SEC_ERROR_UNSUPPORTED_ELLIPTIC_CURVE);
return 0;
}
}
+/* The number of bits in the number from the first non-zero bit onward. */
+unsigned
+SECKEY_BigIntegerBitLength(const SECItem *number)
+{
+ const unsigned char *p;
+ unsigned octets;
+ unsigned bits;
+
+ if (!number || !number->data) {
+ PORT_SetError(SEC_ERROR_INVALID_KEY);
+ return 0;
+ }
+
+ p = number->data;
+ octets = number->len;
+ while (octets > 0 && !*p) {
+ ++p;
+ --octets;
+ }
+ if (octets == 0) {
+ return 0;
+ }
+ /* bits = 7..1 because we know at least one bit is set already */
+ /* Note: This could do a binary search, but this is faster for keys if we
+ * assume that good keys will have the MSB set. */
+ for (bits = 7; bits > 0; --bits) {
+ if (*p & (1 << bits)) {
+ break;
+ }
+ }
+ return octets * 8 + bits - 7;
+}
+
/* returns key strength in bytes (not bits) */
unsigned
SECKEY_PublicKeyStrength(const SECKEYPublicKey *pubk)
{
- unsigned char b0;
- unsigned size;
-
- /* interpret modulus length as key strength */
- if (!pubk)
- goto loser;
- switch (pubk->keyType) {
- case rsaKey:
- if (!pubk->u.rsa.modulus.data) break;
- b0 = pubk->u.rsa.modulus.data[0];
- return b0 ? pubk->u.rsa.modulus.len : pubk->u.rsa.modulus.len - 1;
- case dsaKey:
- if (!pubk->u.dsa.publicValue.data) break;
- b0 = pubk->u.dsa.publicValue.data[0];
- return b0 ? pubk->u.dsa.publicValue.len :
- pubk->u.dsa.publicValue.len - 1;
- case dhKey:
- if (!pubk->u.dh.publicValue.data) break;
- b0 = pubk->u.dh.publicValue.data[0];
- return b0 ? pubk->u.dh.publicValue.len :
- pubk->u.dh.publicValue.len - 1;
- case ecKey:
- /* Get the key size in bits and adjust */
- size = SECKEY_ECParamsToKeySize(&pubk->u.ec.DEREncodedParams);
- return (size + 7)/8;
- default:
- break;
- }
-loser:
- PORT_SetError(SEC_ERROR_INVALID_KEY);
- return 0;
+ return (SECKEY_PublicKeyStrengthInBits(pubk) + 7) / 8;
}
/* returns key strength in bits */
unsigned
SECKEY_PublicKeyStrengthInBits(const SECKEYPublicKey *pubk)
{
- unsigned size;
+ unsigned bitSize = 0;
+
+ if (!pubk) {
+ PORT_SetError(SEC_ERROR_INVALID_KEY);
+ return 0;
+ }
+
+ /* interpret modulus length as key strength */
switch (pubk->keyType) {
case rsaKey:
+ bitSize = SECKEY_BigIntegerBitLength(&pubk->u.rsa.modulus);
+ break;
case dsaKey:
+ bitSize = SECKEY_BigIntegerBitLength(&pubk->u.dsa.publicValue);
+ break;
case dhKey:
- return SECKEY_PublicKeyStrength(pubk) * 8; /* 1 byte = 8 bits */
+ bitSize = SECKEY_BigIntegerBitLength(&pubk->u.dh.publicValue);
+ break;
case ecKey:
- size = SECKEY_ECParamsToKeySize(&pubk->u.ec.DEREncodedParams);
- return size;
+ bitSize = SECKEY_ECParamsToKeySize(&pubk->u.ec.DEREncodedParams);
+ break;
default:
- break;
+ PORT_SetError(SEC_ERROR_INVALID_KEY);
+ break;
}
- PORT_SetError(SEC_ERROR_INVALID_KEY);
- return 0;
+ return bitSize;
}
/* returns signature length in bytes (not bits) */
unsigned
SECKEY_SignatureLen(const SECKEYPublicKey *pubk)
{
unsigned char b0;
unsigned size;
--- a/lib/freebl/blapit.h
+++ b/lib/freebl/blapit.h
@@ -133,20 +133,20 @@ typedef int __BLAPI_DEPRECATED __attribu
#define SEED_KEY_LENGTH 16 /* bytes */
#define NSS_FREEBL_DEFAULT_CHUNKSIZE 2048
/*
* These values come from the initial key size limits from the PKCS #11
* module. They may be arbitrarily adjusted to any value freebl supports.
*/
-#define RSA_MIN_MODULUS_BITS 128
+#define RSA_MIN_MODULUS_BITS 512
#define RSA_MAX_MODULUS_BITS 16384
#define RSA_MAX_EXPONENT_BITS 64
-#define DH_MIN_P_BITS 128
+#define DH_MIN_P_BITS 1023
#define DH_MAX_P_BITS 16384
/*
* The FIPS 186-1 algorithm for generating primes P and Q allows only 9
* distinct values for the length of P, and only one value for the
* length of Q.
* The algorithm uses a variable j to indicate which of the 9 lengths
* of P is to be used.
@@ -176,17 +176,17 @@ typedef int __BLAPI_DEPRECATED __attribu
* 3072 256
*
* The FIPS-186-3 complaiant PQG generator (PQG V2) takes arbitrary p and q
* lengths as input and returns an error if they aren't in this list.
*/
#define DSA1_Q_BITS 160
#define DSA_MAX_P_BITS 3072
-#define DSA_MIN_P_BITS 512
+#define DSA_MIN_P_BITS 1023
#define DSA_MAX_Q_BITS 256
#define DSA_MIN_Q_BITS 160
#if DSA_MAX_Q_BITS != DSA_MAX_SUBPRIME_LEN*8
#error "Inconsistent declaration of DSA SUBPRIME/Q parameters in blapit.h"
#endif
--- a/lib/nss/nss.def
+++ b/lib/nss/nss.def
@@ -1071,8 +1071,14 @@ SEC_GetCrlTimes;
;+ *;
;+};
;+NSS_3.19 { # NSS 3.19 release
;+ global:
CERT_GetImposedNameConstraints;
;+ local:
;+ *;
;+};
+;+NSS_3.19.1 { # NSS 3.19.1 release
+;+ global:
+SECKEY_BigIntegerBitLength;
+;+ local:
+;+ *;
+;+};
--- a/lib/ssl/SSLerrs.h
+++ b/lib/ssl/SSLerrs.h
@@ -417,8 +417,11 @@ ER3(SSL_ERROR_NEXT_PROTOCOL_NO_CALLBACK,
"The next protocol negotiation extension was enabled, but the callback was cleared prior to being needed.")
ER3(SSL_ERROR_NEXT_PROTOCOL_NO_PROTOCOL, (SSL_ERROR_BASE + 130),
"The server supports no protocols that the client advertises in the ALPN extension.")
ER3(SSL_ERROR_INAPPROPRIATE_FALLBACK_ALERT, (SSL_ERROR_BASE + 131),
"The server rejected the handshake because the client downgraded to a lower "
"TLS version than the server supports.")
+
+ER3(SSL_ERROR_WEAK_SERVER_CERT_KEY, (SSL_ERROR_BASE + 132),
+"The server certificate included a public key that was too weak.")
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -6594,39 +6594,16 @@ ssl3_HandleServerHello(sslSocket *ss, SS
alert_loser:
(void)SSL3_SendAlert(ss, alert_fatal, desc);
loser:
errCode = ssl_MapLowLevelError(errCode);
return SECFailure;
}
-/* ssl3_BigIntGreaterThanOne returns true iff |mpint|, taken as an unsigned,
- * big-endian integer is > 1 */
-static PRBool
-ssl3_BigIntGreaterThanOne(const SECItem* mpint) {
- unsigned char firstNonZeroByte = 0;
- unsigned int i;
-
- for (i = 0; i < mpint->len; i++) {
- if (mpint->data[i]) {
- firstNonZeroByte = mpint->data[i];
- break;
- }
- }
-
- if (firstNonZeroByte == 0)
- return PR_FALSE;
- if (firstNonZeroByte > 1)
- return PR_TRUE;
-
- /* firstNonZeroByte == 1, therefore mpint > 1 iff the first non-zero byte
- * is followed by another byte. */
- return (i < mpint->len - 1);
-}
/* Called from ssl3_HandleHandshakeMessage() when it has deciphered a complete
* ssl3 ServerKeyExchange message.
* Caller must hold Handshake and RecvBuf locks.
*/
static SECStatus
ssl3_HandleServerKeyExchange(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
{
@@ -6661,16 +6638,22 @@ ssl3_HandleServerKeyExchange(sslSocket *
case kt_rsa: {
SECItem modulus = {siBuffer, NULL, 0};
SECItem exponent = {siBuffer, NULL, 0};
rv = ssl3_ConsumeHandshakeVariable(ss, &modulus, 2, &b, &length);
if (rv != SECSuccess) {
goto loser; /* malformed. */
}
+ /* This exchange method is only used by export cipher suites.
+ * Those are broken and so this code will eventually be removed. */
+ if (SECKEY_BigIntegerBitLength(&modulus) < 512) {
+ desc = isTLS ? insufficient_security : illegal_parameter;
+ goto alert_loser;
+ }
rv = ssl3_ConsumeHandshakeVariable(ss, &exponent, 2, &b, &length);
if (rv != SECSuccess) {
goto loser; /* malformed. */
}
if (isTLS12) {
rv = ssl3_ConsumeSignatureAndHashAlgorithm(ss, &b, &length,
&sigAndHash);
if (rv != SECSuccess) {
@@ -6746,36 +6729,43 @@ ssl3_HandleServerKeyExchange(sslSocket *
ss->ssl3.hs.ws = wait_cert_request;
return SECSuccess;
}
case kt_dh: {
SECItem dh_p = {siBuffer, NULL, 0};
SECItem dh_g = {siBuffer, NULL, 0};
SECItem dh_Ys = {siBuffer, NULL, 0};
+ unsigned dh_p_bits;
+ unsigned dh_g_bits;
+ unsigned dh_Ys_bits;
rv = ssl3_ConsumeHandshakeVariable(ss, &dh_p, 2, &b, &length);
if (rv != SECSuccess) {
goto loser; /* malformed. */
}
- if (dh_p.len < 512/8) {
+ dh_p_bits = SECKEY_BigIntegerBitLength(&dh_p);
+ if (dh_p_bits < DH_MIN_P_BITS) {
errCode = SSL_ERROR_WEAK_SERVER_EPHEMERAL_DH_KEY;
goto alert_loser;
}
rv = ssl3_ConsumeHandshakeVariable(ss, &dh_g, 2, &b, &length);
if (rv != SECSuccess) {
goto loser; /* malformed. */
}
- if (dh_g.len > dh_p.len || !ssl3_BigIntGreaterThanOne(&dh_g))
+ /* Abort if dh_g is 0, 1, or obviously too big. */
+ dh_g_bits = SECKEY_BigIntegerBitLength(&dh_g);
+ if (dh_g_bits > dh_p_bits || dh_g_bits <= 1)
goto alert_loser;
rv = ssl3_ConsumeHandshakeVariable(ss, &dh_Ys, 2, &b, &length);
if (rv != SECSuccess) {
goto loser; /* malformed. */
}
- if (dh_Ys.len > dh_p.len || !ssl3_BigIntGreaterThanOne(&dh_Ys))
+ dh_Ys_bits = SECKEY_BigIntegerBitLength(&dh_Ys);
+ if (dh_Ys_bits > dh_p_bits || dh_Ys_bits <= 1)
goto alert_loser;
if (isTLS12) {
rv = ssl3_ConsumeSignatureAndHashAlgorithm(ss, &b, &length,
&sigAndHash);
if (rv != SECSuccess) {
goto loser; /* malformed or unsupported. */
}
rv = ssl3_CheckSignatureAndHashAlgorithmConsistency(
@@ -10052,43 +10042,35 @@ ssl3_AuthCertificate(sslSocket *ss)
** it will get fixed when we handle the server key exchange message.
*/
SECKEYPublicKey * pubKey = CERT_ExtractPublicKey(cert);
ss->sec.authAlgorithm = ss->ssl3.hs.kea_def->signKeyType;
ss->sec.keaType = ss->ssl3.hs.kea_def->exchKeyType;
if (pubKey) {
ss->sec.keaKeyBits = ss->sec.authKeyBits =
SECKEY_PublicKeyStrengthInBits(pubKey);
-#ifndef NSS_DISABLE_ECC
- if (ss->sec.keaType == kt_ecdh) {
- /* Get authKeyBits from signing key.
- * XXX The code below uses a quick approximation of
- * key size based on cert->signatureWrap.signature.data
- * (which contains the DER encoded signature). The field
- * cert->signatureWrap.signature.len contains the
- * length of the encoded signature in bits.
- */
- if (ss->ssl3.hs.kea_def->kea == kea_ecdh_ecdsa) {
- ss->sec.authKeyBits =
- cert->signatureWrap.signature.data[3]*8;
- if (cert->signatureWrap.signature.data[4] == 0x00)
- ss->sec.authKeyBits -= 8;
- /*
- * XXX: if cert is not signed by ecdsa we should
- * destroy pubKey and goto bad_cert
- */
- } else if (ss->ssl3.hs.kea_def->kea == kea_ecdh_rsa) {
- ss->sec.authKeyBits = cert->signatureWrap.signature.len;
- /*
- * XXX: if cert is not signed by rsa we should
- * destroy pubKey and goto bad_cert
- */
- }
- }
-#endif /* NSS_DISABLE_ECC */
+ KeyType pubKeyType = SECKEY_GetPublicKeyType(pubKey);
+ /* Too small: not good enough. Send a fatal alert. */
+ /* TODO: Use 1023 for RSA because a higher RSA_MIN_MODULUS_BITS
+ * breaks export cipher suites; when those are removed, increase
+ * RSA_MIN_MODULUS_BITS and use that here. */
+ /* We aren't checking EC here on the understanding that we only
+ * support curves we like, a decision that might need revisiting. */
+ if (((pubKeyType == rsaKey || pubKeyType == rsaPssKey ||
+ pubKeyType == rsaOaepKey) && ss->sec.authKeyBits < 1023) ||
+ (pubKeyType == dsaKey && ss->sec.authKeyBits < DSA_MIN_P_BITS) ||
+ (pubKeyType == dhKey && ss->sec.authKeyBits < DH_MIN_P_BITS)) {
+ PORT_SetError(SSL_ERROR_WEAK_SERVER_CERT_KEY);
+ (void)SSL3_SendAlert(ss, alert_fatal,
+ ss->version >= SSL_LIBRARY_VERSION_TLS_1_0
+ ? insufficient_security
+ : illegal_parameter);
+ SECKEY_DestroyPublicKey(pubKey);
+ return SECFailure;
+ }
SECKEY_DestroyPublicKey(pubKey);
pubKey = NULL;
}
if (ss->ssl3.hs.kea_def->ephemeral) {
ss->ssl3.hs.ws = wait_server_key; /* require server_key_exchange */
} else {
ss->ssl3.hs.ws = wait_cert_request; /* disallow server_key_exchange */
--- a/lib/ssl/sslerr.h
+++ b/lib/ssl/sslerr.h
@@ -193,13 +193,15 @@ SSL_ERROR_UNSUPPORTED_HASH_ALGORITHM
SSL_ERROR_DIGEST_FAILURE = (SSL_ERROR_BASE + 127),
SSL_ERROR_INCORRECT_SIGNATURE_ALGORITHM = (SSL_ERROR_BASE + 128),
SSL_ERROR_NEXT_PROTOCOL_NO_CALLBACK = (SSL_ERROR_BASE + 129),
SSL_ERROR_NEXT_PROTOCOL_NO_PROTOCOL = (SSL_ERROR_BASE + 130),
SSL_ERROR_INAPPROPRIATE_FALLBACK_ALERT = (SSL_ERROR_BASE + 131),
+SSL_ERROR_WEAK_SERVER_CERT_KEY = (SSL_ERROR_BASE + 132),
+
SSL_ERROR_END_OF_LIST /* let the c compiler determine the value of this. */
} SSLErrorCodes;
#endif /* NO_SECURITY_ERROR_ENUM */
#endif /* __SSL_ERR_H_ */