Limit DHE groups to 4096-bit.
dh.c had a 10k-bit limit but it wasn't quite correctly enforced. However,
that's still 1.12s of jank on the IO thread, which is too long. Since the SSL
code consumes DHE groups from the network, it should be responsible for
enforcing what sanity it needs on them.
Costs of various bit lengths on 2013 Macbook Air:
1024 - 1.4ms
2048 - 14ms
3072 - 24ms
4096 - 55ms
5000 - 160ms
10000 - 1.12s
UMA says that DHE groups are 0.2% 4096-bit and otherwise are 5.5% 2048-bit and
94% 1024-bit and some noise. Set the limit to 4096-bit to be conservative,
although that's already quite a lot of jank.
BUG=554295
Change-Id: I8e167748a67e4e1adfb62d73dfff094abfa7d215
Reviewed-on: https://boringssl-review.googlesource.com/6464
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/dh/dh.c b/crypto/dh/dh.c
index a88520d..5409a65 100644
--- a/crypto/dh/dh.c
+++ b/crypto/dh/dh.c
@@ -239,11 +239,16 @@
int ok = 0;
int generate_new_key = 0;
unsigned l;
- BN_CTX *ctx;
+ BN_CTX *ctx = NULL;
BN_MONT_CTX *mont = NULL;
BIGNUM *pub_key = NULL, *priv_key = NULL;
BIGNUM local_priv;
+ if (BN_num_bits(dh->p) > OPENSSL_DH_MAX_MODULUS_BITS) {
+ OPENSSL_PUT_ERROR(DH, DH_R_MODULUS_TOO_LARGE);
+ goto err;
+ }
+
ctx = BN_CTX_new();
if (ctx == NULL) {
goto err;
diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata
index 0b30b13..41a657f 100644
--- a/crypto/err/ssl.errordata
+++ b/crypto/err/ssl.errordata
@@ -46,6 +46,7 @@
SSL,142,DECRYPTION_FAILED
SSL,143,DECRYPTION_FAILED_OR_BAD_RECORD_MAC
SSL,144,DH_PUBLIC_VALUE_LENGTH_IS_WRONG
+SSL,287,DH_P_TOO_LONG
SSL,145,DIGEST_CHECK_FAILED
SSL,146,DTLS_MESSAGE_TOO_BIG
SSL,147,ECC_CERT_NOT_FOR_SIGNING
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 38d838d..29adcb3 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -4545,6 +4545,7 @@
#define SSL_R_CUSTOM_EXTENSION_CONTENTS_TOO_LARGE 284
#define SSL_R_CUSTOM_EXTENSION_ERROR 285
#define SSL_R_NEGOTIATED_BOTH_NPN_AND_ALPN 286
+#define SSL_R_DH_P_TOO_LONG 287
#define SSL_R_SSLV3_ALERT_CLOSE_NOTIFY 1000
#define SSL_R_SSLV3_ALERT_UNEXPECTED_MESSAGE 1010
#define SSL_R_SSLV3_ALERT_BAD_RECORD_MAC 1020
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index b474352..eb96780 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -1168,6 +1168,13 @@
OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_DH_P_LENGTH);
goto err;
}
+ if (s->session->key_exchange_info > 4096) {
+ /* Overly large DHE groups are prohibitively expensive, so enforce a limit
+ * to prevent a server from causing us to perform too expensive of a
+ * computation. */
+ OPENSSL_PUT_ERROR(SSL, SSL_R_DH_P_TOO_LONG);
+ goto err;
+ }
DH_free(s->s3->tmp.peer_dh_tmp);
s->s3->tmp.peer_dh_tmp = dh;
dh = NULL;
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 6573871..a1801c8 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2167,9 +2167,25 @@
},
},
shouldFail: true,
- expectedError: "BAD_DH_P_LENGTH",
+ expectedError: ":BAD_DH_P_LENGTH:",
})
+ testCases = append(testCases, testCase{
+ name: "SillyDH",
+ config: Config{
+ CipherSuites: []uint16{TLS_DHE_RSA_WITH_AES_128_GCM_SHA256},
+ Bugs: ProtocolBugs{
+ // This is a 4097-bit prime number, generated
+ // with:
+ // openssl gendh 4097 | openssl asn1parse -i
+ DHGroupPrime: bigFromHex("01D366FA64A47419B0CD4A45918E8D8C8430F674621956A9F52B0CA592BC104C6E38D60C58F2CA66792A2B7EBDC6F8FFE75AB7D6862C261F34E96A2AEEF53AB7C21365C2E8FB0582F71EB57B1C227C0E55AE859E9904A25EFECD7B435C4D4357BD840B03649D4A1F8037D89EA4E1967DBEEF1CC17A6111C48F12E9615FFF336D3F07064CB17C0B765A012C850B9E3AA7A6984B96D8C867DDC6D0F4AB52042572244796B7ECFF681CD3B3E2E29AAECA391A775BEE94E502FB15881B0F4AC60314EA947C0C82541C3D16FD8C0E09BB7F8F786582032859D9C13187CE6C0CB6F2D3EE6C3C9727C15F14B21D3CD2E02BDB9D119959B0E03DC9E5A91E2578762300B1517D2352FC1D0BB934A4C3E1B20CE9327DB102E89A6C64A8C3148EDFC5A94913933853442FA84451B31FD21E492F92DD5488E0D871AEBFE335A4B92431DEC69591548010E76A5B365D346786E9A2D3E589867D796AA5E25211201D757560D318A87DFB27F3E625BC373DB48BF94A63161C674C3D4265CB737418441B7650EABC209CF675A439BEB3E9D1AA1B79F67198A40CEFD1C89144F7D8BAF61D6AD36F466DA546B4174A0E0CAF5BD788C8243C7C2DDDCC3DB6FC89F12F17D19FBD9B0BC76FE92891CD6BA07BEA3B66EF12D0D85E788FD58675C1B0FBD16029DCC4D34E7A1A41471BDEDF78BF591A8B4E96D88BEC8EDC093E616292BFC096E69A916E8D624B"),
+ },
+ },
+ shouldFail: true,
+ expectedError: ":DH_P_TOO_LONG:",
+ })
+
+
// versionSpecificCiphersTest specifies a test for the TLS 1.0 and TLS
// 1.1 specific cipher suite settings. A server is setup with the given
// cipher lists and then a connection is made for each member of
