Remove renegotiation deferral logic.
When the peer or caller requests a renegotiation, OpenSSL doesn't
renegotiate immediately. It sets a flag to begin a renegotiation as soon
as record-layer read and write buffers are clear. One reason is that
OpenSSL's record layer cannot write a handshake record while an
application data record is being written. The buffer consistency checks
around partial writes will break.
None of these cases are relevant for the client auth hack. We already
require that renego come in at a quiescent part of the application
protocol by forbidding handshake/app_data interleave.
The new behavior is now: when a HelloRequest comes in, if the record
layer is not idle, the renegotiation is rejected as if
SSL_set_reject_peer_renegotiations were set. Otherwise we immediately
begin the new handshake. The server may not send any application data
between HelloRequest and completing the handshake. The HelloRequest may
not be consumed if an SSL_write is pending.
Note this does require that Chromium's HTTP stack not attempt to read
the HTTP response until the request has been written, but the
renegotiation logic already assumes it. Were Chromium to drive the
SSL_read state machine early and the server, say, sent a HelloRequest
after reading the request headers but before we've sent the whole POST
body, the SSL state machine may racily enter renegotiate early, block
writing the POST body on the new handshake, which would break Chromium's
ERR_SSL_CLIENT_AUTH_CERT_NEEDED plumbing.
BUG=429450
Change-Id: I6278240c3bceb5d2e1a2195bdb62dd9e0f4df718
Reviewed-on: https://boringssl-review.googlesource.com/4825
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/include/openssl/ssl3.h b/include/openssl/ssl3.h
index 24d4aa6..640a228 100644
--- a/include/openssl/ssl3.h
+++ b/include/openssl/ssl3.h
@@ -418,9 +418,6 @@
int alert_dispatch;
uint8_t send_alert[2];
- /* This flag is set when we should renegotiate ASAP, basically when there is
- * no more data in the read or write buffers */
- int renegotiate;
int total_renegotiations;
/* State pertaining to the pending handshake.
diff --git a/ssl/internal.h b/ssl/internal.h
index 669a637..0c0ecc3 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -892,8 +892,6 @@
int ssl3_send_finished(SSL *s, int a, int b, const char *sender, int slen);
size_t ssl3_num_ciphers(void);
const SSL_CIPHER *ssl3_get_cipher(size_t i);
-int ssl3_renegotiate(SSL *ssl);
-int ssl3_renegotiate_check(SSL *ssl);
int ssl3_dispatch_alert(SSL *s);
int ssl3_expect_change_cipher_spec(SSL *s);
int ssl3_read_bytes(SSL *s, int type, uint8_t *buf, int len, int peek);
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index 4d4086c..fe5e89d 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -193,9 +193,6 @@
state = s->state;
switch (s->state) {
- case SSL_ST_RENEGOTIATE:
- s->state = SSL_ST_CONNECT;
- /* fallthrough */
case SSL_ST_CONNECT:
case SSL_ST_BEFORE | SSL_ST_CONNECT:
if (cb != NULL) {
diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c
index 101ea83..b4c4d2e 100644
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -1162,18 +1162,12 @@
int ssl3_write(SSL *s, const void *buf, int len) {
ERR_clear_system_error();
- if (s->s3->renegotiate) {
- ssl3_renegotiate_check(s);
- }
return s->method->ssl_write_bytes(s, SSL3_RT_APPLICATION_DATA, buf, len);
}
static int ssl3_read_internal(SSL *s, void *buf, int len, int peek) {
ERR_clear_system_error();
- if (s->s3->renegotiate) {
- ssl3_renegotiate_check(s);
- }
return s->method->ssl_read_bytes(s, SSL3_RT_APPLICATION_DATA, buf, len, peek);
}
@@ -1186,29 +1180,6 @@
return ssl3_read_internal(s, buf, len, 1);
}
-int ssl3_renegotiate(SSL *s) {
- if (s->handshake_func == NULL) {
- return 1;
- }
-
- s->s3->renegotiate = 1;
- return 1;
-}
-
-int ssl3_renegotiate_check(SSL *s) {
- if (s->s3->renegotiate && s->s3->rbuf.left == 0 && s->s3->wbuf.left == 0 &&
- !SSL_in_init(s)) {
- /* if we are the server, and we have sent a 'RENEGOTIATE' message, we
- * need to go to SSL_ST_ACCEPT. */
- s->state = SSL_ST_RENEGOTIATE;
- s->s3->renegotiate = 0;
- s->s3->total_renegotiations++;
- return 1;
- }
-
- return 0;
-}
-
/* If we are using default SHA1+MD5 algorithms switch to new SHA256 PRF and
* handshake macs if required. */
uint32_t ssl_get_algorithm2(SSL *s) {
diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index 171b11d..d2182dd 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -871,21 +871,36 @@
s->s3->handshake_fragment, 4, s, s->msg_callback_arg);
}
- if (SSL_is_init_finished(s) && !s->s3->renegotiate) {
- ssl3_renegotiate(s);
- if (ssl3_renegotiate_check(s)) {
- 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;
- }
- }
+ if (!SSL_is_init_finished(s) || !s->s3->initial_handshake_complete) {
+ /* This cannot happen. If a handshake is in progress, |type| must be
+ * |SSL3_RT_HANDSHAKE|. */
+ assert(0);
+ OPENSSL_PUT_ERROR(SSL, ssl3_read_bytes, ERR_R_INTERNAL_ERROR);
+ goto err;
}
- /* we either finished a handshake or ignored the request, now try again to
- * obtain the (application) data we were asked for */
+
+ /* Renegotiation is only supported at quiescent points in the application
+ * protocol, namely in HTTPS, just before reading the HTTP response. Require
+ * the record-layer be idle and avoid complexities of sending a handshake
+ * record while an application_data record is being written. */
+ if (s->s3->wbuf.left != 0 || s->s3->rbuf.left != 0) {
+ al = SSL_AD_NO_RENEGOTIATION;
+ OPENSSL_PUT_ERROR(SSL, ssl3_read_bytes, SSL_R_NO_RENEGOTIATION);
+ goto f_err;
+ }
+
+ /* Begin a new handshake. */
+ s->state = SSL_ST_CONNECT;
+ 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;
+ }
+
+ /* The handshake completed synchronously. Continue reading records. */
goto start;
}
