Add CBB_zero to set a CBB to the zero state.
One tedious thing about using CBB is that you can't safely CBB_cleanup
until CBB_init is successful, which breaks the general 'goto err' style
of cleanup. This makes it possible:
CBB_zero ~ EVP_MD_CTX_init
CBB_init ~ EVP_DigestInit
CBB_cleanup ~ EVP_MD_CTX_cleanup
Change-Id: I085ecc4405715368886dc4de02285a47e7fc4c52
Reviewed-on: https://boringssl-review.googlesource.com/5267
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc
index 870d7c6..e987e1b 100644
--- a/crypto/bytestring/bytestring_test.cc
+++ b/crypto/bytestring/bytestring_test.cc
@@ -649,6 +649,14 @@
return true;
}
+static int TestZero() {
+ CBB cbb;
+ CBB_zero(&cbb);
+ // Calling |CBB_cleanup| on a zero-state |CBB| must not crash.
+ CBB_cleanup(&cbb);
+ return 1;
+}
+
int main(void) {
CRYPTO_library_init();
@@ -665,7 +673,8 @@
!TestCBBASN1() ||
!TestBerConvert() ||
!TestASN1Uint64() ||
- !TestGetOptionalASN1Bool()) {
+ !TestGetOptionalASN1Bool() ||
+ !TestZero()) {
return 1;
}
diff --git a/crypto/bytestring/cbb.c b/crypto/bytestring/cbb.c
index 290d16f..b9291ce 100644
--- a/crypto/bytestring/cbb.c
+++ b/crypto/bytestring/cbb.c
@@ -20,6 +20,10 @@
#include <openssl/mem.h>
+void CBB_zero(CBB *cbb) {
+ memset(cbb, 0, sizeof(CBB));
+}
+
static int cbb_init(CBB *cbb, uint8_t *buf, size_t cap) {
struct cbb_buffer_st *base;
diff --git a/include/openssl/bytestring.h b/include/openssl/bytestring.h
index 8583c4e..3419275 100644
--- a/include/openssl/bytestring.h
+++ b/include/openssl/bytestring.h
@@ -248,6 +248,12 @@
char is_top_level;
};
+/* CBB_zero sets an uninitialised |cbb| to the zero state. It must be
+ * initialised with |CBB_init| or |CBB_init_fixed| before use, but it is safe to
+ * call |CBB_cleanup| without a successful |CBB_init|. This may be used for more
+ * uniform cleanup of a |CBB|. */
+OPENSSL_EXPORT void CBB_zero(CBB *cbb);
+
/* CBB_init initialises |cbb| with |initial_capacity|. Since a |CBB| grows as
* needed, the |initial_capacity| is just a hint. It returns one on success or
* zero on error. */
diff --git a/ssl/d1_both.c b/ssl/d1_both.c
index f8724ba..74fc201 100644
--- a/ssl/d1_both.c
+++ b/ssl/d1_both.c
@@ -592,18 +592,15 @@
goto err;
}
+ /* Reconstruct the assembled message. */
+ size_t len;
CBB cbb;
+ CBB_zero(&cbb);
if (!BUF_MEM_grow(s->init_buf,
(size_t)frag->msg_header.msg_len +
DTLS1_HM_HEADER_LENGTH) ||
- !CBB_init_fixed(&cbb, (uint8_t *)s->init_buf->data, s->init_buf->max)) {
- OPENSSL_PUT_ERROR(SSL, dtls1_get_message, ERR_R_MALLOC_FAILURE);
- goto err;
- }
-
- /* Reconstruct the assembled message. */
- size_t len;
- if (!CBB_add_u8(&cbb, frag->msg_header.type) ||
+ !CBB_init_fixed(&cbb, (uint8_t *)s->init_buf->data, s->init_buf->max) ||
+ !CBB_add_u8(&cbb, frag->msg_header.type) ||
!CBB_add_u24(&cbb, frag->msg_header.msg_len) ||
!CBB_add_u16(&cbb, frag->msg_header.seq) ||
!CBB_add_u24(&cbb, 0 /* frag_off */) ||
@@ -611,7 +608,7 @@
!CBB_add_bytes(&cbb, frag->fragment, frag->msg_header.msg_len) ||
!CBB_finish(&cbb, NULL, &len)) {
CBB_cleanup(&cbb);
- OPENSSL_PUT_ERROR(SSL, dtls1_get_message, ERR_R_INTERNAL_ERROR);
+ OPENSSL_PUT_ERROR(SSL, dtls1_get_message, ERR_R_MALLOC_FAILURE);
goto err;
}
assert(len == (size_t)frag->msg_header.msg_len + DTLS1_HM_HEADER_LENGTH);
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index 1d9b164..b5c35f0 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -1963,19 +1963,16 @@
uint8_t *new_pms;
size_t new_pms_len;
- if (!CBB_init(&cbb, 2 + psk_len + 2 + pms_len)) {
- OPENSSL_PUT_ERROR(SSL, ssl3_send_client_key_exchange,
- ERR_R_MALLOC_FAILURE);
- goto err;
- }
- if (!CBB_add_u16_length_prefixed(&cbb, &child) ||
+ CBB_zero(&cbb);
+ if (!CBB_init(&cbb, 2 + psk_len + 2 + pms_len) ||
+ !CBB_add_u16_length_prefixed(&cbb, &child) ||
!CBB_add_bytes(&child, pms, pms_len) ||
!CBB_add_u16_length_prefixed(&cbb, &child) ||
!CBB_add_bytes(&child, psk, psk_len) ||
!CBB_finish(&cbb, &new_pms, &new_pms_len)) {
CBB_cleanup(&cbb);
OPENSSL_PUT_ERROR(SSL, ssl3_send_client_key_exchange,
- ERR_R_INTERNAL_ERROR);
+ ERR_R_MALLOC_FAILURE);
goto err;
}
OPENSSL_cleanse(pms, pms_len);
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index 0a26689..b699b18 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -747,12 +747,10 @@
rand_len);
/* Write out an equivalent SSLv3 ClientHello. */
+ CBB_zero(&client_hello);
if (!CBB_init_fixed(&client_hello, (uint8_t *)s->init_buf->data,
- s->init_buf->max)) {
- OPENSSL_PUT_ERROR(SSL, ssl3_get_v2_client_hello, ERR_R_MALLOC_FAILURE);
- return -1;
- }
- if (!CBB_add_u8(&client_hello, SSL3_MT_CLIENT_HELLO) ||
+ s->init_buf->max) ||
+ !CBB_add_u8(&client_hello, SSL3_MT_CLIENT_HELLO) ||
!CBB_add_u24_length_prefixed(&client_hello, &hello_body) ||
!CBB_add_u16(&hello_body, version) ||
!CBB_add_bytes(&hello_body, random, SSL3_RANDOM_SIZE) ||
@@ -760,7 +758,7 @@
!CBB_add_u8(&hello_body, 0) ||
!CBB_add_u16_length_prefixed(&hello_body, &cipher_suites)) {
CBB_cleanup(&client_hello);
- OPENSSL_PUT_ERROR(SSL, ssl3_get_v2_client_hello, ERR_R_INTERNAL_ERROR);
+ OPENSSL_PUT_ERROR(SSL, ssl3_get_v2_client_hello, ERR_R_MALLOC_FAILURE);
return -1;
}
@@ -1977,18 +1975,15 @@
uint8_t *new_data;
size_t new_len;
- if (!CBB_init(&new_premaster, 2 + psk_len + 2 + premaster_secret_len)) {
- OPENSSL_PUT_ERROR(SSL, ssl3_get_client_key_exchange,
- ERR_R_MALLOC_FAILURE);
- goto err;
- }
- if (!CBB_add_u16_length_prefixed(&new_premaster, &child) ||
+ CBB_zero(&new_premaster);
+ if (!CBB_init(&new_premaster, 2 + psk_len + 2 + premaster_secret_len) ||
+ !CBB_add_u16_length_prefixed(&new_premaster, &child) ||
!CBB_add_bytes(&child, premaster_secret, premaster_secret_len) ||
!CBB_add_u16_length_prefixed(&new_premaster, &child) ||
!CBB_add_bytes(&child, psk, psk_len) ||
!CBB_finish(&new_premaster, &new_data, &new_len)) {
OPENSSL_PUT_ERROR(SSL, ssl3_get_client_key_exchange,
- ERR_R_INTERNAL_ERROR);
+ ERR_R_MALLOC_FAILURE);
CBB_cleanup(&new_premaster);
goto err;
}
diff --git a/ssl/ssl_asn1.c b/ssl/ssl_asn1.c
index 76052df..531e0d5 100644
--- a/ssl/ssl_asn1.c
+++ b/ssl/ssl_asn1.c
@@ -163,11 +163,9 @@
return 0;
}
- if (!CBB_init(&cbb, 0)) {
- return 0;
- }
-
- if (!CBB_add_asn1(&cbb, &session, CBS_ASN1_SEQUENCE) ||
+ CBB_zero(&cbb);
+ if (!CBB_init(&cbb, 0) ||
+ !CBB_add_asn1(&cbb, &session, CBS_ASN1_SEQUENCE) ||
!CBB_add_asn1_uint64(&session, SSL_SESSION_ASN1_VERSION) ||
!CBB_add_asn1_uint64(&session, in->ssl_version) ||
!CBB_add_asn1(&session, &child, CBS_ASN1_OCTETSTRING) ||
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 61546ca..91c66a4 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -2582,11 +2582,9 @@
return 0;
}
- if (!CBB_init(&cbb, 4 + 16 + 1 + premaster_len * 2 + 1)) {
- return 0;
- }
-
- if (!CBB_add_bytes(&cbb, (const uint8_t *)"RSA ", 4) ||
+ CBB_zero(&cbb);
+ if (!CBB_init(&cbb, 4 + 16 + 1 + premaster_len * 2 + 1) ||
+ !CBB_add_bytes(&cbb, (const uint8_t *)"RSA ", 4) ||
/* Only the first 8 bytes of the encrypted premaster secret are
* logged. */
!cbb_add_hex(&cbb, encrypted_premaster, 8) ||
@@ -2624,11 +2622,9 @@
return 0;
}
- if (!CBB_init(&cbb, 14 + 64 + 1 + master_len * 2 + 1)) {
- return 0;
- }
-
- if (!CBB_add_bytes(&cbb, (const uint8_t *)"CLIENT_RANDOM ", 14) ||
+ CBB_zero(&cbb);
+ if (!CBB_init(&cbb, 14 + 64 + 1 + master_len * 2 + 1) ||
+ !CBB_add_bytes(&cbb, (const uint8_t *)"CLIENT_RANDOM ", 14) ||
!cbb_add_hex(&cbb, client_random, 32) ||
!CBB_add_bytes(&cbb, (const uint8_t *)" ", 1) ||
!cbb_add_hex(&cbb, master, master_len) ||
