Forbid caller-initiated renegotiations and all renego as a servers.
The only case where renego is supported is if we are a client and the
server sends a HelloRequest. That is still needed to support the renego
+ client auth hack in Chrome. Beyond that, no other forms of renego will
work.
The messy logic where the handshake loop is repurposed to send
HelloRequest and the extremely confusing tri-state s->renegotiate (which
makes SSL_renegotiate_pending a lie during the initial handshake as a
server) are now gone. The next change will further simplify things by
removing ssl->s3->renegotiate and the renego deferral logic. There's
also some server-only renegotiation checks that can go now.
Also clean up ssl3_read_bytes' HelloRequest handling. The old logic relied on
the handshake state machine to reject bad HelloRequests which... actually that
code probably lets you initiate renego by sending the first four bytes of a
ServerHello and expecting the peer to read it later.
BUG=429450
Change-Id: Ie0f87d0c2b94e13811fe8e22e810ab2ffc8efa6c
Reviewed-on: https://boringssl-review.googlesource.com/4824
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/d1_clnt.c b/ssl/d1_clnt.c
index 90123ea..bd72e2f 100644
--- a/ssl/d1_clnt.c
+++ b/ssl/d1_clnt.c
@@ -468,7 +468,6 @@
ssl_free_wbio_buffer(s);
s->init_num = 0;
- s->renegotiate = 0;
s->s3->initial_handshake_complete = 1;
ssl_update_cache(s, SSL_SESS_CACHE_CLIENT);
diff --git a/ssl/d1_meth.c b/ssl/d1_meth.c
index a11fbdd..5d75d02 100644
--- a/ssl/d1_meth.c
+++ b/ssl/d1_meth.c
@@ -68,8 +68,6 @@
ssl3_peek,
ssl3_write,
dtls1_shutdown,
- ssl3_renegotiate,
- ssl3_renegotiate_check,
dtls1_get_message,
dtls1_read_bytes,
dtls1_write_app_data_bytes,
diff --git a/ssl/d1_srvr.c b/ssl/d1_srvr.c
index 856e2e9..3fbfb0c 100644
--- a/ssl/d1_srvr.c
+++ b/ssl/d1_srvr.c
@@ -207,7 +207,6 @@
case SSL3_ST_SW_SRVR_HELLO_A:
case SSL3_ST_SW_SRVR_HELLO_B:
- s->renegotiate = 2;
dtls1_start_timer(s);
ret = ssl3_send_server_hello(s);
if (ret <= 0) {
@@ -435,7 +434,6 @@
ssl_free_wbio_buffer(s);
s->init_num = 0;
- s->renegotiate = 0;
s->s3->initial_handshake_complete = 1;
ssl_update_cache(s, SSL_SESS_CACHE_SERVER);
diff --git a/ssl/internal.h b/ssl/internal.h
index 08a74dc..669a637 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -641,8 +641,6 @@
int (*ssl_peek)(SSL *s, void *buf, int len);
int (*ssl_write)(SSL *s, const void *buf, int len);
int (*ssl_shutdown)(SSL *s);
- int (*ssl_renegotiate)(SSL *s);
- int (*ssl_renegotiate_check)(SSL *s);
long (*ssl_get_message)(SSL *s, int header_state, int body_state,
int msg_type, long max,
enum ssl_hash_message_t hash_message, int *ok);
@@ -1002,7 +1000,6 @@
int ssl3_get_v2_client_hello(SSL *s);
int ssl3_get_client_hello(SSL *s);
int ssl3_send_server_hello(SSL *s);
-int ssl3_send_hello_request(SSL *s);
int ssl3_send_server_key_exchange(SSL *s);
int ssl3_send_certificate_request(SSL *s);
int ssl3_send_server_done(SSL *s);
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index eb0bbd0..4d4086c 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -194,7 +194,6 @@
switch (s->state) {
case SSL_ST_RENEGOTIATE:
- s->renegotiate = 1;
s->state = SSL_ST_CONNECT;
/* fallthrough */
case SSL_ST_CONNECT:
@@ -551,7 +550,6 @@
ssl_free_wbio_buffer(s);
s->init_num = 0;
- s->renegotiate = 0;
s->s3->tmp.in_false_start = 0;
s->s3->initial_handshake_complete = 1;
diff --git a/ssl/s3_meth.c b/ssl/s3_meth.c
index 28b9051..9652444 100644
--- a/ssl/s3_meth.c
+++ b/ssl/s3_meth.c
@@ -67,8 +67,6 @@
ssl3_peek,
ssl3_write,
ssl3_shutdown,
- ssl3_renegotiate,
- ssl3_renegotiate_check,
ssl3_get_message,
ssl3_read_bytes,
ssl3_write_bytes,
diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index 6f86751..171b11d 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -831,22 +831,18 @@
return n;
}
-
- /* If we get here, then type != rr->type; if we have a handshake message,
- * then it was unexpected (Hello Request or Client Hello). */
-
- /* In case of record types for which we have 'fragment' storage, fill that so
- * that we can process the data at a fixed place. */
+ /* Process unexpected records. */
if (rr->type == SSL3_RT_HANDSHAKE) {
/* If peer renegotiations are disabled, all out-of-order handshake records
- * are fatal. */
- if (!s->accept_peer_renegotiations) {
+ * are fatal. Renegotiations as a server are never supported. */
+ if (!s->accept_peer_renegotiations || s->server) {
al = SSL_AD_NO_RENEGOTIATION;
OPENSSL_PUT_ERROR(SSL, ssl3_read_bytes, SSL_R_NO_RENEGOTIATION);
goto f_err;
}
+ /* HelloRequests may be fragmented across multiple records. */
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;
@@ -858,24 +854,17 @@
if (s->s3->handshake_fragment_len < size) {
goto start; /* fragment was too small */
}
- }
- /* s->s3->handshake_fragment_len == 4 iff rr->type == SSL3_RT_HANDSHAKE;
- * (Possibly rr is 'empty' now, i.e. rr->length may be 0.) */
-
- /* If we are a client, check for an incoming 'Hello Request': */
- if (!s->server && s->s3->handshake_fragment_len >= 4 &&
- s->s3->handshake_fragment[0] == SSL3_MT_HELLO_REQUEST &&
- s->session != NULL && s->session->cipher != NULL) {
- s->s3->handshake_fragment_len = 0;
-
- if (s->s3->handshake_fragment[1] != 0 ||
+ /* Parse out and consume a HelloRequest. */
+ if (s->s3->handshake_fragment[0] != SSL3_MT_HELLO_REQUEST ||
+ s->s3->handshake_fragment[1] != 0 ||
s->s3->handshake_fragment[2] != 0 ||
s->s3->handshake_fragment[3] != 0) {
al = SSL_AD_DECODE_ERROR;
OPENSSL_PUT_ERROR(SSL, ssl3_read_bytes, SSL_R_BAD_HELLO_REQUEST);
goto f_err;
}
+ s->s3->handshake_fragment_len = 0;
if (s->msg_callback) {
s->msg_callback(0, s->version, SSL3_RT_HANDSHAKE,
@@ -1015,24 +1004,6 @@
}
}
- /* Unexpected handshake message (Client Hello, or protocol violation) */
- if (s->s3->handshake_fragment_len >= 4 && !s->in_handshake) {
- if ((s->state & SSL_ST_MASK) == SSL_ST_OK) {
- s->state = s->server ? SSL_ST_ACCEPT : SSL_ST_CONNECT;
- s->renegotiate = 1;
- }
- i = s->handshake_func(s);
- if (i < 0) {
- return i;
- }
- if (i == 0) {
- OPENSSL_PUT_ERROR(SSL, ssl3_read_bytes, SSL_R_SSL_HANDSHAKE_FAILURE);
- return -1;
- }
-
- goto start;
- }
-
/* We already handled these. */
assert(rr->type != SSL3_RT_CHANGE_CIPHER_SPEC && rr->type != SSL3_RT_ALERT &&
rr->type != SSL3_RT_HANDSHAKE);
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index a5280e9..5445358 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -207,66 +207,8 @@
state = s->state;
switch (s->state) {
- case SSL_ST_RENEGOTIATE:
- /* This state is the renegotiate entry point. It sends a HelloRequest
- * and nothing else. */
- s->renegotiate = 1;
-
- if (cb != NULL) {
- cb(s, SSL_CB_HANDSHAKE_START, 1);
- }
-
- if (s->init_buf == NULL) {
- buf = BUF_MEM_new();
- if (!buf || !BUF_MEM_grow(buf, SSL3_RT_MAX_PLAIN_LENGTH)) {
- ret = -1;
- goto end;
- }
- s->init_buf = buf;
- buf = NULL;
- }
- s->init_num = 0;
-
- if (!s->s3->send_connection_binding &&
- !(s->options & SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION)) {
- /* Server attempting to renegotiate with client that doesn't support
- * secure renegotiation. */
- OPENSSL_PUT_ERROR(SSL, ssl3_accept,
- SSL_R_UNSAFE_LEGACY_RENEGOTIATION_DISABLED);
- ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);
- ret = -1;
- goto end;
- }
-
- s->state = SSL3_ST_SW_HELLO_REQ_A;
- break;
-
- case SSL3_ST_SW_HELLO_REQ_A:
- case SSL3_ST_SW_HELLO_REQ_B:
- s->shutdown = 0;
- ret = ssl3_send_hello_request(s);
- if (ret <= 0) {
- goto end;
- }
- s->s3->tmp.next_state = SSL3_ST_SW_HELLO_REQ_C;
- s->state = SSL3_ST_SW_FLUSH;
- s->init_num = 0;
-
- if (!ssl3_init_finished_mac(s)) {
- OPENSSL_PUT_ERROR(SSL, ssl3_accept, ERR_R_INTERNAL_ERROR);
- ret = -1;
- goto end;
- }
- break;
-
- case SSL3_ST_SW_HELLO_REQ_C:
- s->state = SSL_ST_OK;
- break;
-
case SSL_ST_ACCEPT:
case SSL_ST_BEFORE | SSL_ST_ACCEPT:
- /* This state is the entry point for the handshake itself (initial and
- * renegotiation). */
if (cb != NULL) {
cb(s, SSL_CB_HANDSHAKE_START, 1);
}
@@ -337,7 +279,6 @@
if (ret <= 0) {
goto end;
}
- s->renegotiate = 2;
s->state = SSL3_ST_SW_SRVR_HELLO_A;
s->init_num = 0;
break;
@@ -640,16 +581,12 @@
s->session->peer = NULL;
}
- if (s->renegotiate == 2) {
- /* skipped if we just sent a HelloRequest */
- s->renegotiate = 0;
- s->s3->initial_handshake_complete = 1;
+ s->s3->initial_handshake_complete = 1;
- ssl_update_cache(s, SSL_SESS_CACHE_SERVER);
+ ssl_update_cache(s, SSL_SESS_CACHE_SERVER);
- if (cb != NULL) {
- cb(s, SSL_CB_HANDSHAKE_DONE, 1);
- }
+ if (cb != NULL) {
+ cb(s, SSL_CB_HANDSHAKE_DONE, 1);
}
ret = 1;
@@ -898,18 +835,6 @@
return 1;
}
-int ssl3_send_hello_request(SSL *s) {
- if (s->state == SSL3_ST_SW_HELLO_REQ_A) {
- if (!ssl_set_handshake_header(s, SSL3_MT_HELLO_REQUEST, 0)) {
- return -1;
- }
- s->state = SSL3_ST_SW_HELLO_REQ_B;
- }
-
- /* SSL3_ST_SW_HELLO_REQ_B */
- return ssl_do_write(s);
-}
-
int ssl3_get_client_hello(SSL *s) {
int i, ok, al = SSL_AD_INTERNAL_ERROR, ret = -1;
long n;
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index e53d12c..e84079a 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -931,24 +931,14 @@
return 1;
}
-int SSL_renegotiate(SSL *s) {
- if (SSL_IS_DTLS(s)) {
- /* Renegotiation is not supported for DTLS. */
- OPENSSL_PUT_ERROR(SSL, SSL_renegotiate, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
- return 0;
- }
-
- if (s->renegotiate == 0) {
- s->renegotiate = 1;
- }
-
- return s->method->ssl_renegotiate(s);
+int SSL_renegotiate(SSL *ssl) {
+ /* Caller-initiated renegotiation is not supported. */
+ OPENSSL_PUT_ERROR(SSL, SSL_renegotiate, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
+ return 0;
}
-int SSL_renegotiate_pending(SSL *s) {
- /* becomes true when negotiation is requested; false again once a handshake
- * has finished */
- return s->renegotiate != 0;
+int SSL_renegotiate_pending(SSL *ssl) {
+ return SSL_in_init(ssl) && ssl->s3->initial_handshake_complete;
}
uint32_t SSL_CTX_set_options(SSL_CTX *ctx, uint32_t options) {
@@ -2112,8 +2102,6 @@
return -1;
}
- s->method->ssl_renegotiate_check(s);
-
if (SSL_in_init(s)) {
ret = s->handshake_func(s);
}
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index 71c5bf0..40c3e42 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -840,30 +840,6 @@
}
}
- if (config->renegotiate) {
- if (config->async) {
- fprintf(stderr, "-renegotiate is not supported with -async.\n");
- return false;
- }
- if (config->implicit_handshake) {
- fprintf(stderr, "-renegotiate is not supported with -implicit-handshake.\n");
- return false;
- }
-
- SSL_renegotiate(ssl.get());
-
- ret = SSL_do_handshake(ssl.get());
- if (ret != 1) {
- return false;
- }
-
- SSL_set_state(ssl.get(), SSL_ST_ACCEPT);
- ret = SSL_do_handshake(ssl.get());
- if (ret != 1) {
- return false;
- }
- }
-
if (config->export_keying_material > 0) {
std::vector<uint8_t> result(
static_cast<size_t>(config->export_keying_material));
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index 57524a7..9e6cce3 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -690,10 +690,6 @@
// client offers a resumption or the server accepts one.
FailIfResumeOnRenego bool
- // NoSignatureAlgorithmsOnRenego, if true, causes renegotiations to omit
- // the signature_algorithms extension.
- NoSignatureAlgorithmsOnRenego bool
-
// IgnorePeerCipherPreferences, if true, causes the peer's cipher
// preferences to be ignored.
IgnorePeerCipherPreferences bool
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go
index 5129b8f..1f9e84f 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -133,7 +133,7 @@
return errors.New("tls: short read from Rand: " + err.Error())
}
- if hello.vers >= VersionTLS12 && !c.config.Bugs.NoSignatureAndHashes && (c.cipherSuite == nil || !c.config.Bugs.NoSignatureAlgorithmsOnRenego) {
+ if hello.vers >= VersionTLS12 && !c.config.Bugs.NoSignatureAndHashes {
hello.signatureAndHashes = c.config.signatureAndHashesForClient()
}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index e4a3f9a..6510e33 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2817,92 +2817,16 @@
}
func addRenegotiationTests() {
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "Renegotiate-Server",
- config: Config{
- Bugs: ProtocolBugs{
- FailIfResumeOnRenego: true,
- },
- },
- flags: []string{"-renegotiate"},
- shimWritesFirst: true,
- })
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "Renegotiate-Server-EmptyExt",
- config: Config{
- Bugs: ProtocolBugs{
- EmptyRenegotiationInfo: true,
- },
- },
- flags: []string{"-renegotiate"},
- shimWritesFirst: true,
- shouldFail: true,
- expectedError: ":RENEGOTIATION_MISMATCH:",
- })
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "Renegotiate-Server-BadExt",
- config: Config{
- Bugs: ProtocolBugs{
- BadRenegotiationInfo: true,
- },
- },
- flags: []string{"-renegotiate"},
- shimWritesFirst: true,
- shouldFail: true,
- expectedError: ":RENEGOTIATION_MISMATCH:",
- })
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "Renegotiate-Server-ClientInitiated",
- renegotiate: true,
- })
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "Renegotiate-Server-ClientInitiated-NoExt",
- renegotiate: true,
- config: Config{
- Bugs: ProtocolBugs{
- NoRenegotiationInfo: true,
- },
- },
- shouldFail: true,
- expectedError: ":UNSAFE_LEGACY_RENEGOTIATION_DISABLED:",
- })
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "Renegotiate-Server-ClientInitiated-NoExt-Allowed",
- renegotiate: true,
- config: Config{
- Bugs: ProtocolBugs{
- NoRenegotiationInfo: true,
- },
- },
- flags: []string{"-allow-unsafe-legacy-renegotiation"},
- })
+ // Servers cannot renegotiate.
testCases = append(testCases, testCase{
testType: serverTest,
- name: "Renegotiate-Server-ClientInitiated-Forbidden",
+ name: "Renegotiate-Server-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,
- name: "Renegotiate-Server-NoSignatureAlgorithms",
- config: Config{
- Bugs: ProtocolBugs{
- NoSignatureAlgorithmsOnRenego: true,
- },
- },
- flags: []string{"-renegotiate"},
- shimWritesFirst: true,
- })
// TODO(agl): test the renegotiation info SCSV.
testCases = append(testCases, testCase{
name: "Renegotiate-Client",
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc
index 4b24da6..df8553c 100644
--- a/ssl/test/test_config.cc
+++ b/ssl/test/test_config.cc
@@ -65,7 +65,6 @@
{ "-expect-session-miss", &TestConfig::expect_session_miss },
{ "-expect-extended-master-secret",
&TestConfig::expect_extended_master_secret },
- { "-renegotiate", &TestConfig::renegotiate },
{ "-allow-unsafe-legacy-renegotiation",
&TestConfig::allow_unsafe_legacy_renegotiation },
{ "-enable-ocsp-stapling", &TestConfig::enable_ocsp_stapling },
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h
index 4bac561..ff801db 100644
--- a/ssl/test/test_config.h
+++ b/ssl/test/test_config.h
@@ -54,7 +54,6 @@
bool expect_extended_master_secret = false;
std::string psk;
std::string psk_identity;
- bool renegotiate = false;
bool allow_unsafe_legacy_renegotiation = false;
std::string srtp_profiles;
bool enable_ocsp_stapling = false;
