From fbdfefb76e8a81fd05f24be5d8c3f8732a5e6223 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 16 Feb 2015 19:33:53 -0500 Subject: [PATCH] Handle failures in ssl3_finish_mac. It may fail because the BIO_write to the memory BIO can allocate. Unfortunately, this bubbles up pretty far up now that we've moved the handshake hash to ssl3_set_handshake_header. Change-Id: I58884347a4456bb974ac4783078131522167e29d Reviewed-on: https://boringssl-review.googlesource.com/3483 Reviewed-by: Adam Langley --- ssl/d1_both.c | 6 ++++-- ssl/d1_lib.c | 8 ++++---- ssl/s3_both.c | 22 ++++++++++++++-------- ssl/s3_clnt.c | 22 ++++++++++++++++------ ssl/s3_enc.c | 6 +++--- ssl/s3_lib.c | 4 ++-- ssl/s3_srvr.c | 41 +++++++++++++++++++++++++++++++---------- ssl/ssl_locl.h | 12 ++++++------ 8 files changed, 80 insertions(+), 41 deletions(-) diff --git a/ssl/d1_both.c b/ssl/d1_both.c index 72dadc6e..7891ed7b 100644 --- a/ssl/d1_both.c +++ b/ssl/d1_both.c @@ -415,8 +415,9 @@ again: s->init_msg = (uint8_t *)s->init_buf->data + DTLS1_HM_HEADER_LENGTH; - if (hash_message != SSL_GET_MESSAGE_DONT_HASH_MESSAGE) { - ssl3_hash_current_message(s); + if (hash_message != SSL_GET_MESSAGE_DONT_HASH_MESSAGE && + !ssl3_hash_current_message(s)) { + goto err; } if (s->msg_callback) { s->msg_callback(0, s->version, SSL3_RT_HANDSHAKE, p, msg_len, s, @@ -431,6 +432,7 @@ again: f_err: ssl3_send_alert(s, SSL3_AL_FATAL, al); +err: *ok = 0; return -1; } diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c index dcf86e51..6d2f77f0 100644 --- a/ssl/d1_lib.c +++ b/ssl/d1_lib.c @@ -82,7 +82,7 @@ static void get_current_time(SSL *ssl, OPENSSL_timeval *out_clock); static OPENSSL_timeval *dtls1_get_timeout(SSL *s, OPENSSL_timeval *timeleft); -static void dtls1_set_handshake_header(SSL *s, int type, unsigned long len); +static int dtls1_set_handshake_header(SSL *s, int type, unsigned long len); static int dtls1_handshake_write(SSL *s); const SSL3_ENC_METHOD DTLSv1_enc_data = { @@ -415,7 +415,7 @@ static void get_current_time(SSL *ssl, OPENSSL_timeval *out_clock) { #endif } -static void dtls1_set_handshake_header(SSL *s, int htype, unsigned long len) { +static int dtls1_set_handshake_header(SSL *s, int htype, unsigned long len) { uint8_t *message = (uint8_t *)s->init_buf->data; const struct hm_header_st *msg_hdr = &s->d1->w_msg_hdr; uint8_t serialised_header[DTLS1_HM_HEADER_LENGTH]; @@ -438,8 +438,8 @@ static void dtls1_set_handshake_header(SSL *s, int htype, unsigned long len) { s2n(msg_hdr->seq, p); l2n3(0, p); l2n3(msg_hdr->msg_len, p); - ssl3_finish_mac(s, serialised_header, sizeof(serialised_header)); - ssl3_finish_mac(s, message + DTLS1_HM_HEADER_LENGTH, len); + return ssl3_finish_mac(s, serialised_header, sizeof(serialised_header)) && + ssl3_finish_mac(s, message + DTLS1_HM_HEADER_LENGTH, len); } static int dtls1_handshake_write(SSL *s) { diff --git a/ssl/s3_both.c b/ssl/s3_both.c index ddfc1fe7..a32be5fe 100644 --- a/ssl/s3_both.c +++ b/ssl/s3_both.c @@ -185,7 +185,9 @@ int ssl3_send_finished(SSL *s, int a, int b, const char *sender, int slen) { s->s3->previous_client_finished_len = n; } - ssl_set_handshake_header(s, SSL3_MT_FINISHED, n); + if (!ssl_set_handshake_header(s, SSL3_MT_FINISHED, n)) { + return 0; + } s->state = b; } @@ -232,7 +234,9 @@ int ssl3_get_finished(SSL *s, int a, int b) { /* Snapshot the finished hash before incorporating the new message. */ ssl3_take_mac(s); - ssl3_hash_current_message(s); + if (!ssl3_hash_current_message(s)) { + goto err; + } /* If this occurs, we have missed a message. * TODO(davidben): Is this check now redundant with SSL3_FLAGS_EXPECT_CCS? */ @@ -273,6 +277,7 @@ int ssl3_get_finished(SSL *s, int a, int b) { f_err: ssl3_send_alert(s, SSL3_AL_FATAL, al); +err: return 0; } @@ -308,8 +313,7 @@ int ssl3_output_cert_chain(SSL *s, CERT_PKEY *cpk) { p = ssl_handshake_start(s); l2n3(l, p); l += 3; - ssl_set_handshake_header(s, SSL3_MT_CERTIFICATE, l); - return 1; + return ssl_set_handshake_header(s, SSL3_MT_CERTIFICATE, l); } /* Obtain handshake message of message type |msg_type| (any if |msg_type| == -1), @@ -416,8 +420,9 @@ long ssl3_get_message(SSL *s, int header_state, int body_state, int msg_type, } /* Feed this message into MAC computation. */ - if (hash_message != SSL_GET_MESSAGE_DONT_HASH_MESSAGE) { - ssl3_hash_current_message(s); + if (hash_message != SSL_GET_MESSAGE_DONT_HASH_MESSAGE && + !ssl3_hash_current_message(s)) { + goto err; } if (s->msg_callback) { s->msg_callback(0, s->version, SSL3_RT_HANDSHAKE, s->init_buf->data, @@ -434,11 +439,12 @@ err: return -1; } -void ssl3_hash_current_message(SSL *s) { +int ssl3_hash_current_message(SSL *s) { /* The handshake header (different size between DTLS and TLS) is included in * the hash. */ size_t header_len = s->init_msg - (uint8_t *)s->init_buf->data; - ssl3_finish_mac(s, (uint8_t *)s->init_buf->data, s->init_num + header_len); + return ssl3_finish_mac(s, (uint8_t *)s->init_buf->data, + s->init_num + header_len); } /* ssl3_cert_verify_hash is documented as needing EVP_MAX_MD_SIZE because that diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c index c6c2b429..f70cfcbf 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c @@ -724,7 +724,9 @@ int ssl3_send_client_hello(SSL *s) { } l = p - d; - ssl_set_handshake_header(s, SSL3_MT_CLIENT_HELLO, l); + if (!ssl_set_handshake_header(s, SSL3_MT_CLIENT_HELLO, l)) { + goto err; + } s->state = SSL3_ST_CW_CLNT_HELLO_B; } @@ -2001,7 +2003,9 @@ int ssl3_send_client_key_exchange(SSL *s) { /* The message must be added to the finished hash before calculating the * master secret. */ - ssl_set_handshake_header(s, SSL3_MT_CLIENT_KEY_EXCHANGE, n); + if (!ssl_set_handshake_header(s, SSL3_MT_CLIENT_KEY_EXCHANGE, n)) { + goto err; + } s->state = SSL3_ST_CW_KEY_EXCH_B; s->session->master_key_length = s->enc_method->generate_master_secret( @@ -2097,7 +2101,9 @@ int ssl3_send_cert_verify(SSL *s) { s2n(signature_length, p); n += signature_length + 2; - ssl_set_handshake_header(s, SSL3_MT_CERTIFICATE_VERIFY, n); + if (!ssl_set_handshake_header(s, SSL3_MT_CERTIFICATE_VERIFY, n)) { + goto err; + } s->state = SSL3_ST_CW_CERT_VRFY_B; } @@ -2286,7 +2292,9 @@ int ssl3_send_next_proto(SSL *s) { memset(p, 0, padding_len); p += padding_len; - ssl_set_handshake_header(s, SSL3_MT_NEXT_PROTO, p - d); + if (!ssl_set_handshake_header(s, SSL3_MT_NEXT_PROTO, p - d)) { + return -1; + } s->state = SSL3_ST_CW_NEXT_PROTO_B; } @@ -2397,8 +2405,10 @@ int ssl3_send_channel_id(SSL *s) { goto err; } - ssl_set_handshake_header(s, SSL3_MT_ENCRYPTED_EXTENSIONS, - 2 + 2 + TLSEXT_CHANNEL_ID_SIZE); + if (!ssl_set_handshake_header(s, SSL3_MT_ENCRYPTED_EXTENSIONS, + 2 + 2 + TLSEXT_CHANNEL_ID_SIZE)) { + goto err; + } s->state = SSL3_ST_CW_CHANNEL_ID_B; ret = ssl_do_write(s); diff --git a/ssl/s3_enc.c b/ssl/s3_enc.c index 9630843f..7bf325a7 100644 --- a/ssl/s3_enc.c +++ b/ssl/s3_enc.c @@ -264,12 +264,11 @@ void ssl3_free_digest_list(SSL *s) { s->s3->handshake_dgst = NULL; } -void ssl3_finish_mac(SSL *s, const uint8_t *buf, int len) { +int ssl3_finish_mac(SSL *s, const uint8_t *buf, int len) { int i; if (s->s3->handshake_buffer) { - BIO_write(s->s3->handshake_buffer, (void *)buf, len); - return; + return BIO_write(s->s3->handshake_buffer, (void *)buf, len) >= 0; } for (i = 0; i < SSL_MAX_DIGEST; i++) { @@ -277,6 +276,7 @@ void ssl3_finish_mac(SSL *s, const uint8_t *buf, int len) { EVP_DigestUpdate(s->s3->handshake_dgst[i], buf, len); } } + return 1; } int ssl3_digest_cached_records( diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c index 8ed2be97..700fbafa 100644 --- a/ssl/s3_lib.c +++ b/ssl/s3_lib.c @@ -595,7 +595,7 @@ int ssl3_pending(const SSL *s) { : 0; } -void ssl3_set_handshake_header(SSL *s, int htype, unsigned long len) { +int ssl3_set_handshake_header(SSL *s, int htype, unsigned long len) { uint8_t *p = (uint8_t *)s->init_buf->data; *(p++) = htype; l2n3(len, p); @@ -603,7 +603,7 @@ void ssl3_set_handshake_header(SSL *s, int htype, unsigned long len) { s->init_off = 0; /* Add the message to the handshake hash. */ - ssl3_finish_mac(s, (uint8_t *)s->init_buf->data, s->init_num); + return ssl3_finish_mac(s, (uint8_t *)s->init_buf->data, s->init_num); } int ssl3_handshake_write(SSL *s) { return ssl3_do_write(s, SSL3_RT_HANDSHAKE); } diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c index c020b3a5..4e60d634 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c @@ -813,7 +813,10 @@ int ssl3_get_v2_client_hello(SSL *s) { /* The V2ClientHello without the length is incorporated into the Finished * hash. */ - ssl3_finish_mac(s, CBS_data(&v2_client_hello), CBS_len(&v2_client_hello)); + if (!ssl3_finish_mac(s, CBS_data(&v2_client_hello), + CBS_len(&v2_client_hello))) { + return -1; + } if (s->msg_callback) { s->msg_callback(0, SSL2_VERSION, 0, CBS_data(&v2_client_hello), CBS_len(&v2_client_hello), s, s->msg_callback_arg); @@ -912,7 +915,9 @@ int ssl3_get_v2_client_hello(SSL *s) { int ssl3_send_hello_request(SSL *s) { if (s->state == SSL3_ST_SW_HELLO_REQ_A) { - ssl_set_handshake_header(s, SSL3_MT_HELLO_REQUEST, 0); + if (!ssl_set_handshake_header(s, SSL3_MT_HELLO_REQUEST, 0)) { + return -1; + } s->state = SSL3_ST_SW_HELLO_REQ_B; } @@ -1299,7 +1304,9 @@ int ssl3_send_server_hello(SSL *s) { /* do the header */ l = (p - d); - ssl_set_handshake_header(s, SSL3_MT_SERVER_HELLO, l); + if (!ssl_set_handshake_header(s, SSL3_MT_SERVER_HELLO, l)) { + return -1; + } s->state = SSL3_ST_SW_SRVR_HELLO_B; } @@ -1309,7 +1316,9 @@ int ssl3_send_server_hello(SSL *s) { int ssl3_send_server_done(SSL *s) { if (s->state == SSL3_ST_SW_SRVR_DONE_A) { - ssl_set_handshake_header(s, SSL3_MT_SERVER_DONE, 0); + if (!ssl_set_handshake_header(s, SSL3_MT_SERVER_DONE, 0)) { + return -1; + } s->state = SSL3_ST_SW_SRVR_DONE_B; } @@ -1612,7 +1621,9 @@ int ssl3_send_server_key_exchange(SSL *s) { } } - ssl_set_handshake_header(s, SSL3_MT_SERVER_KEY_EXCHANGE, n); + if (!ssl_set_handshake_header(s, SSL3_MT_SERVER_KEY_EXCHANGE, n)) { + goto err; + } } s->state = SSL3_ST_SW_KEY_EXCH_B; @@ -1685,7 +1696,9 @@ int ssl3_send_certificate_request(SSL *s) { p = ssl_handshake_start(s) + off; s2n(nl, p); - ssl_set_handshake_header(s, SSL3_MT_CERTIFICATE_REQUEST, n); + if (!ssl_set_handshake_header(s, SSL3_MT_CERTIFICATE_REQUEST, n)) { + goto err; + } s->state = SSL3_ST_SW_CERT_REQ_B; } @@ -2193,7 +2206,9 @@ int ssl3_get_cert_verify(SSL *s) { !ssl3_digest_cached_records(s, free_handshake_buffer)) { goto err; } - ssl3_hash_current_message(s); + if (!ssl3_hash_current_message(s)) { + goto err; + } /* Parse and verify the signature. */ if (!CBS_get_u16_length_prefixed(&certificate_verify, &signature) || @@ -2466,7 +2481,9 @@ int ssl3_send_new_session_ticket(SSL *s) { p += placeholder_len; len = p - ssl_handshake_start(s); - ssl_set_handshake_header(s, SSL3_MT_NEWSESSION_TICKET, len); + if (!ssl_set_handshake_header(s, SSL3_MT_NEWSESSION_TICKET, len)) { + return -1; + } s->state = SSL3_ST_SW_SESSION_TICKET_B; return ssl_do_write(s); } @@ -2537,7 +2554,9 @@ int ssl3_send_new_session_ticket(SSL *s) { /* Skip ticket lifetime hint */ p = ssl_handshake_start(s) + 4; s2n(len - 6, p); - ssl_set_handshake_header(s, SSL3_MT_NEWSESSION_TICKET, len); + if (!ssl_set_handshake_header(s, SSL3_MT_NEWSESSION_TICKET, len)) { + return -1; + } s->state = SSL3_ST_SW_SESSION_TICKET_B; OPENSSL_free(session); } @@ -2638,7 +2657,9 @@ int ssl3_get_channel_id(SSL *s) { EVP_MD_CTX_cleanup(&md_ctx); assert(channel_id_hash_len == SHA256_DIGEST_LENGTH); - ssl3_hash_current_message(s); + if (!ssl3_hash_current_message(s)) { + return -1; + } /* s->state doesn't reflect whether ChangeCipherSpec has been received in * this handshake, but s->s3->change_cipher_spec does (will be reset by diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index c7abe79b..9d77c913 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -583,7 +583,7 @@ struct ssl3_enc_method { /* Handshake header length */ unsigned int hhlen; /* Set the handshake header */ - void (*set_handshake_header)(SSL *s, int type, unsigned long len); + int (*set_handshake_header)(SSL *s, int type, unsigned long len); /* Write out handshake message */ int (*do_write)(SSL *s); }; @@ -732,9 +732,9 @@ int ssl3_get_req_cert_type(SSL *s, uint8_t *p); long ssl3_get_message(SSL *s, int header_state, int body_state, int msg_type, long max, int hash_message, int *ok); -/* ssl3_hash_current_message incorporates the current handshake message into - * the handshake hash. */ -void ssl3_hash_current_message(SSL *s); +/* ssl3_hash_current_message incorporates the current handshake message into the + * handshake hash. It returns one on success and zero on allocation failure. */ +int ssl3_hash_current_message(SSL *s); /* ssl3_cert_verify_hash writes the CertificateVerify hash into the bytes * pointed to by |out| and writes the number of bytes to |*out_len|. |out| must @@ -756,7 +756,7 @@ int ssl3_read_bytes(SSL *s, int type, uint8_t *buf, int len, int peek); int ssl3_write_bytes(SSL *s, int type, const void *buf, int len); int ssl3_final_finish_mac(SSL *s, const char *sender, int slen, uint8_t *p); int ssl3_cert_verify_mac(SSL *s, int md_nid, uint8_t *p); -void ssl3_finish_mac(SSL *s, const uint8_t *buf, int len); +int ssl3_finish_mac(SSL *s, const uint8_t *buf, int len); void ssl3_free_digest_list(SSL *s); int ssl3_output_cert_chain(SSL *s, CERT_PKEY *cpk); const SSL_CIPHER *ssl3_choose_cipher( @@ -791,7 +791,7 @@ int ssl3_pending(const SSL *s); void ssl3_record_sequence_update(uint8_t *seq); int ssl3_do_change_cipher_spec(SSL *ssl); -void ssl3_set_handshake_header(SSL *s, int htype, unsigned long len); +int ssl3_set_handshake_header(SSL *s, int htype, unsigned long len); int ssl3_handshake_write(SSL *s); int dtls1_do_write(SSL *s, int type);