From a7810c12e99ed8cf2e3b2a049e124b4a3b1b274b Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 6 Jun 2016 18:54:51 -0400 Subject: [PATCH] Make tls_open_record always in-place. The business with ssl_record_prefix_len is rather a hassle. Instead, have tls_open_record always decrypt in-place and give back a CBS to where the body is. This way the caller doesn't need to do an extra check all to avoid creating an invalid pointer and underflow in subtraction. Change-Id: I4e12b25a760870d8f8a503673ab00a2d774fc9ee Reviewed-on: https://boringssl-review.googlesource.com/8173 Reviewed-by: Steven Valdez Reviewed-by: David Benjamin --- crypto/bytestring/bytestring_test.cc | 7 +++-- crypto/bytestring/cbs.c | 9 ++++++ include/openssl/bytestring.h | 4 +++ ssl/d1_pkt.c | 19 ++++--------- ssl/dtls_record.c | 21 ++++++-------- ssl/internal.h | 41 ++++++++++++---------------- ssl/s3_pkt.c | 25 ++++++----------- ssl/ssl_aead_ctx.c | 24 ++++++++-------- ssl/tls_record.c | 39 +++++++++++--------------- 9 files changed, 88 insertions(+), 101 deletions(-) diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc index 84ecffcd..e1d16f49 100644 --- a/crypto/bytestring/bytestring_test.cc +++ b/crypto/bytestring/bytestring_test.cc @@ -43,7 +43,7 @@ static bool TestSkip() { } static bool TestGetUint() { - static const uint8_t kData[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}; + static const uint8_t kData[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11}; uint8_t u8; uint16_t u16; uint32_t u32; @@ -58,7 +58,10 @@ static bool TestGetUint() { u32 == 0x40506 && CBS_get_u32(&data, &u32) && u32 == 0x708090a && - !CBS_get_u8(&data, &u8); + CBS_get_last_u8(&data, &u8) && + u8 == 0xb && + !CBS_get_u8(&data, &u8) && + !CBS_get_last_u8(&data, &u8); } static bool TestGetPrefixed() { diff --git a/crypto/bytestring/cbs.c b/crypto/bytestring/cbs.c index ed54b499..c86afbda 100644 --- a/crypto/bytestring/cbs.c +++ b/crypto/bytestring/cbs.c @@ -128,6 +128,15 @@ int CBS_get_u32(CBS *cbs, uint32_t *out) { return cbs_get_u(cbs, out, 4); } +int CBS_get_last_u8(CBS *cbs, uint8_t *out) { + if (cbs->len == 0) { + return 0; + } + *out = cbs->data[cbs->len - 1]; + cbs->len--; + return 1; +} + int CBS_get_bytes(CBS *cbs, CBS *out, size_t len) { const uint8_t *v; if (!cbs_get(cbs, &v, len)) { diff --git a/include/openssl/bytestring.h b/include/openssl/bytestring.h index 68ede2dc..3a8d4e53 100644 --- a/include/openssl/bytestring.h +++ b/include/openssl/bytestring.h @@ -95,6 +95,10 @@ OPENSSL_EXPORT int CBS_get_u24(CBS *cbs, uint32_t *out); * and advances |cbs|. It returns one on success and zero on error. */ OPENSSL_EXPORT int CBS_get_u32(CBS *cbs, uint32_t *out); +/* CBS_get_last_u8 sets |*out| to the last uint8_t from |cbs| and shortens + * |cbs|. It returns one on success and zero on error. */ +OPENSSL_EXPORT int CBS_get_last_u8(CBS *cbs, uint8_t *out); + /* CBS_get_bytes sets |*out| to the next |len| bytes from |cbs| and advances * |cbs|. It returns one on success and zero on error. */ OPENSSL_EXPORT int CBS_get_bytes(CBS *cbs, CBS *out, size_t len); diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c index 68e6a4d6..4f05f0fe 100644 --- a/ssl/d1_pkt.c +++ b/ssl/d1_pkt.c @@ -159,18 +159,11 @@ again: } assert(ssl_read_buffer_len(ssl) > 0); - /* Ensure the packet is large enough to decrypt in-place. */ - if (ssl_read_buffer_len(ssl) < ssl_record_prefix_len(ssl)) { - ssl_read_buffer_clear(ssl); - goto again; - } - - uint8_t *out = ssl_read_buffer(ssl) + ssl_record_prefix_len(ssl); - size_t max_out = ssl_read_buffer_len(ssl) - ssl_record_prefix_len(ssl); + CBS body; uint8_t type, alert; - size_t len, consumed; + size_t consumed; enum ssl_open_record_t open_ret = - dtls_open_record(ssl, &type, out, &len, &consumed, &alert, max_out, + dtls_open_record(ssl, &type, &body, &consumed, &alert, ssl_read_buffer(ssl), ssl_read_buffer_len(ssl)); ssl_read_buffer_consume(ssl, consumed); switch (open_ret) { @@ -179,15 +172,15 @@ again: break; case ssl_open_record_success: - if (len > 0xffff) { + if (CBS_len(&body) > 0xffff) { OPENSSL_PUT_ERROR(SSL, ERR_R_OVERFLOW); return -1; } SSL3_RECORD *rr = &ssl->s3->rrec; rr->type = type; - rr->length = (uint16_t)len; - rr->data = out; + rr->length = (uint16_t)CBS_len(&body); + rr->data = (uint8_t *)CBS_data(&body); return 1; case ssl_open_record_discard: diff --git a/ssl/dtls_record.c b/ssl/dtls_record.c index 94dfb28c..76ae8e52 100644 --- a/ssl/dtls_record.c +++ b/ssl/dtls_record.c @@ -171,10 +171,10 @@ static void dtls1_bitmap_record(DTLS1_BITMAP *bitmap, } } -enum ssl_open_record_t dtls_open_record( - SSL *ssl, uint8_t *out_type, uint8_t *out, size_t *out_len, - size_t *out_consumed, uint8_t *out_alert, size_t max_out, const uint8_t *in, - size_t in_len) { +enum ssl_open_record_t dtls_open_record(SSL *ssl, uint8_t *out_type, CBS *out, + size_t *out_consumed, + uint8_t *out_alert, uint8_t *in, + size_t in_len) { *out_consumed = 0; CBS cbs; @@ -211,11 +211,9 @@ enum ssl_open_record_t dtls_open_record( return ssl_open_record_discard; } - /* Decrypt the body. */ - size_t plaintext_len; - if (!SSL_AEAD_CTX_open(ssl->s3->aead_read_ctx, out, &plaintext_len, max_out, - type, version, sequence, CBS_data(&body), - CBS_len(&body))) { + /* Decrypt the body in-place. */ + if (!SSL_AEAD_CTX_open(ssl->s3->aead_read_ctx, out, type, version, sequence, + (uint8_t *)CBS_data(&body), CBS_len(&body))) { /* Bad packets are silently dropped in DTLS. See section 4.2.1 of RFC 6347. * Clear the error queue of any errors decryption may have added. Drop the * entire packet as it must not have come from the peer. @@ -229,7 +227,7 @@ enum ssl_open_record_t dtls_open_record( *out_consumed = in_len - CBS_len(&cbs); /* Check the plaintext length. */ - if (plaintext_len > SSL3_RT_MAX_PLAIN_LENGTH) { + if (CBS_len(out) > SSL3_RT_MAX_PLAIN_LENGTH) { OPENSSL_PUT_ERROR(SSL, SSL_R_DATA_LENGTH_TOO_LONG); *out_alert = SSL_AD_RECORD_OVERFLOW; return ssl_open_record_error; @@ -241,13 +239,12 @@ enum ssl_open_record_t dtls_open_record( * useful if we also limit discarded packets. */ if (type == SSL3_RT_ALERT) { - return ssl_process_alert(ssl, out_alert, out, plaintext_len); + return ssl_process_alert(ssl, out_alert, CBS_data(out), CBS_len(out)); } ssl->s3->warning_alert_count = 0; *out_type = type; - *out_len = plaintext_len; return ssl_open_record_success; } diff --git a/ssl/internal.h b/ssl/internal.h index 13e7935a..48569695 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -313,15 +313,13 @@ size_t SSL_AEAD_CTX_explicit_nonce_len(SSL_AEAD_CTX *ctx); * |SSL_AEAD_CTX_seal|. |ctx| may be NULL to denote the null cipher. */ size_t SSL_AEAD_CTX_max_overhead(SSL_AEAD_CTX *ctx); -/* SSL_AEAD_CTX_open authenticates and decrypts |in_len| bytes from |in| and - * writes the result to |out|. It returns one on success and zero on - * error. |ctx| may be NULL to denote the null cipher. - * - * If |in| and |out| alias then |out| must be <= |in| + |explicit_nonce_len|. */ -int SSL_AEAD_CTX_open(SSL_AEAD_CTX *ctx, uint8_t *out, size_t *out_len, - size_t max_out, uint8_t type, uint16_t wire_version, - const uint8_t seqnum[8], const uint8_t *in, - size_t in_len); +/* SSL_AEAD_CTX_open authenticates and decrypts |in_len| bytes from |in| + * in-place. On success, it sets |*out| to the plaintext in |in| and returns + * one. Otherwise, it returns zero. |ctx| may be NULL to denote the null cipher. + * The output will always be |explicit_nonce_len| bytes ahead of |in|. */ +int SSL_AEAD_CTX_open(SSL_AEAD_CTX *ctx, CBS *out, uint8_t type, + uint16_t wire_version, const uint8_t seqnum[8], + uint8_t *in, size_t in_len); /* SSL_AEAD_CTX_seal encrypts and authenticates |in_len| bytes from |in| and * writes the result to |out|. It returns one on success and zero on @@ -370,7 +368,7 @@ enum ssl_open_record_t { ssl_open_record_error, }; -/* tls_open_record decrypts a record from |in|. +/* tls_open_record decrypts a record from |in| in-place. * * If the input did not contain a complete record, it returns * |ssl_open_record_partial|. It sets |*out_consumed| to the total number of @@ -382,8 +380,8 @@ enum ssl_open_record_t { * decrypted. * * On success, it returns |ssl_open_record_success|. It sets |*out_type| to the - * record type, |*out_len| to the plaintext length, and writes the record body - * to |out|. Note that |*out_len| may be zero. + * record type and |*out| to the record body in |in|. Note that |*out| may be + * empty. * * If a record was successfully processed but should be discarded, it returns * |ssl_open_record_discard|. @@ -392,20 +390,17 @@ enum ssl_open_record_t { * it returns |ssl_open_record_close_notify| or |ssl_open_record_fatal_alert|. * * On failure, it returns |ssl_open_record_error| and sets |*out_alert| to an - * alert to emit. - * - * If |in| and |out| alias, |out| must be <= |in| + |ssl_record_prefix_len|. */ -enum ssl_open_record_t tls_open_record( - SSL *ssl, uint8_t *out_type, uint8_t *out, size_t *out_len, - size_t *out_consumed, uint8_t *out_alert, size_t max_out, const uint8_t *in, - size_t in_len); + * alert to emit. */ +enum ssl_open_record_t tls_open_record(SSL *ssl, uint8_t *out_type, CBS *out, + size_t *out_consumed, uint8_t *out_alert, + uint8_t *in, size_t in_len); /* dtls_open_record implements |tls_open_record| for DTLS. It never returns * |ssl_open_record_partial| but otherwise behaves analogously. */ -enum ssl_open_record_t dtls_open_record( - SSL *ssl, uint8_t *out_type, uint8_t *out, size_t *out_len, - size_t *out_consumed, uint8_t *out_alert, size_t max_out, const uint8_t *in, - size_t in_len); +enum ssl_open_record_t dtls_open_record(SSL *ssl, uint8_t *out_type, CBS *out, + size_t *out_consumed, + uint8_t *out_alert, uint8_t *in, + size_t in_len); /* ssl_seal_prefix_len returns the length of the prefix before the ciphertext * when sealing a record with |ssl|. Note that this value may differ from diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c index 04d41be3..cd6de5d5 100644 --- a/ssl/s3_pkt.c +++ b/ssl/s3_pkt.c @@ -138,41 +138,34 @@ again: return 0; } - /* Ensure the buffer is large enough to decrypt in-place. */ - int read_ret = ssl_read_buffer_extend_to(ssl, ssl_record_prefix_len(ssl)); - if (read_ret <= 0) { - return read_ret; - } - assert(ssl_read_buffer_len(ssl) >= ssl_record_prefix_len(ssl)); - - uint8_t *out = ssl_read_buffer(ssl) + ssl_record_prefix_len(ssl); - size_t max_out = ssl_read_buffer_len(ssl) - ssl_record_prefix_len(ssl); + CBS body; uint8_t type, alert; - size_t len, consumed; + size_t consumed; enum ssl_open_record_t open_ret = - tls_open_record(ssl, &type, out, &len, &consumed, &alert, max_out, + tls_open_record(ssl, &type, &body, &consumed, &alert, ssl_read_buffer(ssl), ssl_read_buffer_len(ssl)); if (open_ret != ssl_open_record_partial) { ssl_read_buffer_consume(ssl, consumed); } switch (open_ret) { - case ssl_open_record_partial: - read_ret = ssl_read_buffer_extend_to(ssl, consumed); + case ssl_open_record_partial: { + int read_ret = ssl_read_buffer_extend_to(ssl, consumed); if (read_ret <= 0) { return read_ret; } goto again; + } case ssl_open_record_success: - if (len > 0xffff) { + if (CBS_len(&body) > 0xffff) { OPENSSL_PUT_ERROR(SSL, ERR_R_OVERFLOW); return -1; } SSL3_RECORD *rr = &ssl->s3->rrec; rr->type = type; - rr->length = (uint16_t)len; - rr->data = out; + rr->length = (uint16_t)CBS_len(&body); + rr->data = (uint8_t *)CBS_data(&body); return 1; case ssl_open_record_discard: diff --git a/ssl/ssl_aead_ctx.c b/ssl/ssl_aead_ctx.c index 1e549ea9..88daddd9 100644 --- a/ssl/ssl_aead_ctx.c +++ b/ssl/ssl_aead_ctx.c @@ -166,22 +166,16 @@ static size_t ssl_aead_ctx_get_ad(SSL_AEAD_CTX *aead, uint8_t out[13], return len; } -int SSL_AEAD_CTX_open(SSL_AEAD_CTX *aead, uint8_t *out, size_t *out_len, - size_t max_out, uint8_t type, uint16_t wire_version, - const uint8_t seqnum[8], const uint8_t *in, - size_t in_len) { +int SSL_AEAD_CTX_open(SSL_AEAD_CTX *aead, CBS *out, uint8_t type, + uint16_t wire_version, const uint8_t seqnum[8], + uint8_t *in, size_t in_len) { #if defined(BORINGSSL_UNSAFE_FUZZER_MODE) aead = NULL; #endif if (aead == NULL) { /* Handle the initial NULL cipher. */ - if (in_len > max_out) { - OPENSSL_PUT_ERROR(SSL, SSL_R_BUFFER_TOO_SMALL); - return 0; - } - memmove(out, in, in_len); - *out_len = in_len; + CBS_init(out, in, in_len); return 1; } @@ -239,8 +233,14 @@ int SSL_AEAD_CTX_open(SSL_AEAD_CTX *aead, uint8_t *out, size_t *out_len, } } - return EVP_AEAD_CTX_open(&aead->ctx, out, out_len, max_out, nonce, nonce_len, - in, in_len, ad, ad_len); + /* Decrypt in-place. */ + size_t len; + if (!EVP_AEAD_CTX_open(&aead->ctx, in, &len, in_len, nonce, nonce_len, + in, in_len, ad, ad_len)) { + return 0; + } + CBS_init(out, in, len); + return 1; } int SSL_AEAD_CTX_seal(SSL_AEAD_CTX *aead, uint8_t *out, size_t *out_len, diff --git a/ssl/tls_record.c b/ssl/tls_record.c index 869831c4..24dfb215 100644 --- a/ssl/tls_record.c +++ b/ssl/tls_record.c @@ -192,10 +192,9 @@ size_t ssl_max_seal_overhead(const SSL *ssl) { return ret; } -enum ssl_open_record_t tls_open_record( - SSL *ssl, uint8_t *out_type, uint8_t *out, size_t *out_len, - size_t *out_consumed, uint8_t *out_alert, size_t max_out, const uint8_t *in, - size_t in_len) { +enum ssl_open_record_t tls_open_record(SSL *ssl, uint8_t *out_type, CBS *out, + size_t *out_consumed, uint8_t *out_alert, + uint8_t *in, size_t in_len) { *out_consumed = 0; CBS cbs; @@ -236,10 +235,9 @@ enum ssl_open_record_t tls_open_record( ssl_do_msg_callback(ssl, 0 /* read */, 0, SSL3_RT_HEADER, in, SSL3_RT_HEADER_LENGTH); - /* Decrypt the body. */ - size_t plaintext_len; - if (!SSL_AEAD_CTX_open(ssl->s3->aead_read_ctx, out, &plaintext_len, max_out, - type, version, ssl->s3->read_sequence, CBS_data(&body), + /* Decrypt the body in-place. */ + if (!SSL_AEAD_CTX_open(ssl->s3->aead_read_ctx, out, type, version, + ssl->s3->read_sequence, (uint8_t *)CBS_data(&body), CBS_len(&body))) { OPENSSL_PUT_ERROR(SSL, SSL_R_DECRYPTION_FAILED_OR_BAD_RECORD_MAC); *out_alert = SSL_AD_BAD_RECORD_MAC; @@ -256,28 +254,24 @@ enum ssl_open_record_t tls_open_record( if (ssl->s3->have_version && ssl3_protocol_version(ssl) >= TLS1_3_VERSION && ssl->s3->aead_read_ctx != NULL) { - while (plaintext_len != 0 && out[plaintext_len - 1] == 0) { - plaintext_len--; - } - - if (plaintext_len == 0) { - OPENSSL_PUT_ERROR(SSL, SSL_R_DECRYPTION_FAILED_OR_BAD_RECORD_MAC); - *out_alert = SSL_AD_DECRYPT_ERROR; - return ssl_open_record_error; - } - type = out[plaintext_len - 1]; - plaintext_len--; + do { + if (!CBS_get_last_u8(out, &type)) { + OPENSSL_PUT_ERROR(SSL, SSL_R_DECRYPTION_FAILED_OR_BAD_RECORD_MAC); + *out_alert = SSL_AD_DECRYPT_ERROR; + return ssl_open_record_error; + } + } while (type == 0); } /* Check the plaintext length. */ - if (plaintext_len > SSL3_RT_MAX_PLAIN_LENGTH) { + if (CBS_len(out) > SSL3_RT_MAX_PLAIN_LENGTH) { OPENSSL_PUT_ERROR(SSL, SSL_R_DATA_LENGTH_TOO_LONG); *out_alert = SSL_AD_RECORD_OVERFLOW; return ssl_open_record_error; } /* Limit the number of consecutive empty records. */ - if (plaintext_len == 0) { + if (CBS_len(out) == 0) { ssl->s3->empty_record_count++; if (ssl->s3->empty_record_count > kMaxEmptyRecords) { OPENSSL_PUT_ERROR(SSL, SSL_R_TOO_MANY_EMPTY_FRAGMENTS); @@ -291,13 +285,12 @@ enum ssl_open_record_t tls_open_record( } if (type == SSL3_RT_ALERT) { - return ssl_process_alert(ssl, out_alert, out, plaintext_len); + return ssl_process_alert(ssl, out_alert, CBS_data(out), CBS_len(out)); } ssl->s3->warning_alert_count = 0; *out_type = type; - *out_len = plaintext_len; return ssl_open_record_success; }