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