From b16346b0ada83c2a7ecddc8aed790ce81b00f993 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 8 Apr 2015 19:16:58 -0400 Subject: [PATCH] Add SSL_set_reject_peer_renegotiations. This causes any unexpected handshake records to be met with a fatal no_renegotiation alert. In addition, restore the redundant version sanity-checks in the handshake state machines. Some code would zero the version field as a hacky way to break the handshake on renego. Those will be removed when switching to this API. The spec allows for a non-fatal no_renegotiation alert, but ssl3_read_bytes makes it difficult to find the end of a ClientHello and skip it entirely. Given that OpenSSL goes out of its way to map non-fatal no_renegotiation alerts to fatal ones, this seems probably fine. This avoids needing to account for another source of the library consuming an unbounded number of bytes without returning data up. Change-Id: Ie5050d9c9350c29cfe32d03a3c991bdc1da9e0e4 Reviewed-on: https://boringssl-review.googlesource.com/4300 Reviewed-by: Adam Langley --- include/openssl/ssl.h | 9 +++++++++ ssl/s3_clnt.c | 9 +++++++++ ssl/s3_pkt.c | 8 ++++++++ ssl/s3_srvr.c | 9 +++++++++ ssl/ssl_lib.c | 4 ++++ ssl/test/bssl_shim.cc | 3 +++ ssl/test/runner/runner.go | 17 +++++++++++++++++ ssl/test/test_config.cc | 1 + ssl/test/test_config.h | 1 + 9 files changed, 61 insertions(+) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index fa8916b26..e6545dabc 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -1135,6 +1135,11 @@ OPENSSL_EXPORT void SSL_get0_alpn_selected(const SSL *ssl, const uint8_t **data, * causes 3G radios to switch to DCH mode (high data rate). */ OPENSSL_EXPORT void SSL_enable_fastradio_padding(SSL *ssl, char on_off); +/* SSL_set_reject_peer_renegotiations controls whether renegotiation attempts by + * the peer are rejected. It may be set at any point in a connection's lifetime + * to disallow future renegotiations programmatically. */ +OPENSSL_EXPORT void SSL_set_reject_peer_renegotiations(SSL *ssl, int reject); + /* the maximum length of the buffer given to callbacks containing the resulting * identity/psk */ #define PSK_MAX_IDENTITY_LEN 128 @@ -1395,6 +1400,10 @@ struct ssl_st { * data rate) state in 3G networks. */ char fastradio_padding; + /* reject_peer_renegotiations, if one, causes causes renegotiation attempts + * from the peer to be rejected with a fatal error. */ + char reject_peer_renegotiations; + /* These fields are always NULL and exist only to keep wpa_supplicant happy * about the change to EVP_AEAD. They are only needed for EAP-FAST, which we * don't support. */ diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c index 17cc1adee..00f15cc93 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c @@ -203,6 +203,15 @@ int ssl3_connect(SSL *s) { cb(s, SSL_CB_HANDSHAKE_START, 1); } + if ((s->version >> 8) != 3) { + /* TODO(davidben): Some consumers clear |s->version| to break the + * handshake in a callback. Remove this when they're using proper + * APIs. */ + OPENSSL_PUT_ERROR(SSL, ssl3_connect, ERR_R_INTERNAL_ERROR); + ret = -1; + goto end; + } + if (s->init_buf == NULL) { buf = BUF_MEM_new(); if (buf == NULL || diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c index aa3c59645..a2db1d450 100644 --- a/ssl/s3_pkt.c +++ b/ssl/s3_pkt.c @@ -885,6 +885,14 @@ start: * that we can process the data at a fixed place. */ if (rr->type == SSL3_RT_HANDSHAKE) { + /* If peer renegotiations are disabled, all out-of-order handshake records + * are fatal. */ + if (s->reject_peer_renegotiations) { + al = SSL_AD_NO_RENEGOTIATION; + OPENSSL_PUT_ERROR(SSL, ssl3_read_bytes, SSL_R_NO_RENEGOTIATION); + goto f_err; + } + const size_t size = sizeof(s->s3->handshake_fragment); const size_t avail = size - s->s3->handshake_fragment_len; const size_t todo = (rr->length < avail) ? rr->length : avail; diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c index 0ba1467df..222cc66b3 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c @@ -276,6 +276,15 @@ int ssl3_accept(SSL *s) { cb(s, SSL_CB_HANDSHAKE_START, 1); } + if ((s->version >> 8) != 3) { + /* TODO(davidben): Some consumers clear |s->version| to break the + * handshake in a callback. Remove this when they're using proper + * APIs. */ + OPENSSL_PUT_ERROR(SSL, ssl3_accept, ERR_R_INTERNAL_ERROR); + ret = -1; + goto end; + } + if (s->init_buf == NULL) { buf = BUF_MEM_new(); if (!buf || !BUF_MEM_grow(buf, SSL3_RT_MAX_PLAIN_LENGTH)) { diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index ee260ea1f..f8e9bab2b 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -3105,6 +3105,10 @@ void SSL_enable_fastradio_padding(SSL *s, char on_off) { s->fastradio_padding = on_off; } +void SSL_set_reject_peer_renegotiations(SSL *s, int reject) { + s->reject_peer_renegotiations = !!reject; +} + const SSL_CIPHER *SSL_get_cipher_by_value(uint16_t value) { return ssl3_get_cipher_by_value(value); } diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index 091dc30ff..fd4bd601c 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc @@ -663,6 +663,9 @@ static bool DoExchange(ScopedSSL_SESSION *out_session, SSL_CTX *ssl_ctx, !SSL_set_cipher_list(ssl.get(), config->cipher.c_str())) { return false; } + if (config->reject_peer_renegotiations) { + SSL_set_reject_peer_renegotiations(ssl.get(), 1); + } int sock = Connect(config->port); if (sock == -1) { diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 650c14f72..ce0271f71 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -2873,6 +2873,15 @@ func addRenegotiationTests() { }, flags: []string{"-allow-unsafe-legacy-renegotiation"}, }) + testCases = append(testCases, testCase{ + testType: serverTest, + name: "Renegotiate-Server-ClientInitiated-Forbidden", + renegotiate: true, + flags: []string{"-reject-peer-renegotiations"}, + shouldFail: true, + expectedError: ":NO_RENEGOTIATION:", + expectedLocalError: "remote error: no renegotiation", + }) // Regression test for CVE-2015-0291. testCases = append(testCases, testCase{ testType: serverTest, @@ -2938,6 +2947,14 @@ func addRenegotiationTests() { }, renegotiateCiphers: []uint16{TLS_RSA_WITH_RC4_128_SHA}, }) + testCases = append(testCases, testCase{ + name: "Renegotiate-Client-Forbidden", + renegotiate: true, + flags: []string{"-reject-peer-renegotiations"}, + shouldFail: true, + expectedError: ":NO_RENEGOTIATION:", + expectedLocalError: "remote error: no renegotiation", + }) testCases = append(testCases, testCase{ name: "Renegotiate-SameClientVersion", renegotiate: true, diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index 25278370b..25906f720 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc @@ -80,6 +80,7 @@ const Flag kBoolFlags[] = { { "-fail-second-ddos-callback", &TestConfig::fail_second_ddos_callback }, { "-handshake-never-done", &TestConfig::handshake_never_done }, { "-use-export-context", &TestConfig::use_export_context }, + { "-reject-peer-renegotiations", &TestConfig::reject_peer_renegotiations }, }; const Flag kStringFlags[] = { diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index de1eda666..f107a0fbc 100644 --- a/ssl/test/test_config.h +++ b/ssl/test/test_config.h @@ -77,6 +77,7 @@ struct TestConfig { std::string export_label; std::string export_context; bool use_export_context = false; + bool reject_peer_renegotiations = false; }; bool ParseConfig(int argc, char **argv, TestConfig *out_config);