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 <agl@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index fa8916b..e6545da 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -1135,6 +1135,11 @@
* 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 @@
* 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 17cc1ad..00f15cc 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -203,6 +203,15 @@
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 aa3c596..a2db1d4 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -885,6 +885,14 @@
* 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 0ba1467..222cc66 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -276,6 +276,15 @@
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 ee260ea..f8e9bab 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -3105,6 +3105,10 @@
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 091dc30..fd4bd60 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -663,6 +663,9 @@
!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 650c14f..ce0271f 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2873,6 +2873,15 @@
},
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,
@@ -2939,6 +2948,14 @@
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,
config: Config{
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc
index 2527837..25906f7 100644
--- a/ssl/test/test_config.cc
+++ b/ssl/test/test_config.cc
@@ -80,6 +80,7 @@
{ "-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<std::string> kStringFlags[] = {
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h
index de1eda6..f107a0f 100644
--- a/ssl/test/test_config.h
+++ b/ssl/test/test_config.h
@@ -77,6 +77,7 @@
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);
