From a01deee96b1c9a28ff7d09ac6bcc75b91171b458 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 8 Dec 2015 18:56:31 -0500 Subject: [PATCH] Make CBB_len relative to its argument. Rather than the length of the top-level CBB, which is kind of odd when ASN.1 length prefixes are not yet determined, return the number of bytes written to the CBB so far. This can be computed without increasing the size of CBB at all. Have offset and pending_*. This means functions which take in a CBB as argument will not be sensitive to whether the CBB is a top-level or child CBB. The extensions logic had to be careful to only ever compare differences of lengths, which was awkward. The reversal will also allow for the following pattern in the future, once CBB_add_space is split into, say, CBB_reserve and CBB_did_write and we add a CBB_data: uint8_t *signature; size_t signature_len = 0; if (!CBB_add_asn1(out, &cert, CBB_ASN1_SEQUENCE) || /* Emit the TBSCertificate. */ !CBB_add_asn1(&cert, &tbs_cert, CBS_ASN1_SEQUENCE) || !CBB_add_tbs_cert_stuff(&tbs_cert, stuff) || !CBB_flush(&cert) || /* Feed it into md_ctx. */ !EVP_DigestSignInit(&md_ctx, NULL, EVP_sha256(), NULL, pkey) || !EVP_DigestSignUpdate(&md_ctx, CBB_data(&cert), CBB_len(&cert)) || /* Emit the signature algorithm. */ !CBB_add_asn1(&cert, &sig_alg, CBS_ASN1_SEQUENCE) || !CBB_add_sigalg_stuff(&sig_alg, other_stuff) || /* Emit the signature. */ !EVP_DigestSignFinal(&md_ctx, NULL, &signature_len) || !CBB_reserve(&cert, &signature, signature_len) || !EVP_DigestSignFinal(&md_ctx, signature, &signature_len) || !CBB_did_write(&cert, signature_len)) { goto err; } (Were TBSCertificate not the first field, we'd still have to sample CBB_len(&cert), but at least that's reasonable straight-forward. The alternative would be if CBB_data and CBB_len somehow worked on recently-invalidated CBBs, but that would go wrong once the invalidated CBB's parent flushed and possibly shifts everything.) And similar for signing ServerKeyExchange. Change-Id: I7761e492ae472d7632875b5666b6088970261b14 Reviewed-on: https://boringssl-review.googlesource.com/6681 Reviewed-by: Adam Langley --- crypto/bytestring/bytestring_test.cc | 10 ++++--- crypto/bytestring/cbb.c | 44 +++++++++++++--------------- include/openssl/bytestring.h | 19 ++++++------ ssl/t1_lib.c | 17 ++++------- 4 files changed, 41 insertions(+), 49 deletions(-) diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc index 99f8bd1d..8a178126 100644 --- a/crypto/bytestring/bytestring_test.cc +++ b/crypto/bytestring/bytestring_test.cc @@ -344,12 +344,14 @@ static bool TestCBBPrefixed() { size_t buf_len; CBB cbb, contents, inner_contents, inner_inner_contents; - if (!CBB_init(&cbb, 0)) { - return false; - } - if (!CBB_add_u8_length_prefixed(&cbb, &contents) || + if (!CBB_init(&cbb, 0) || + CBB_len(&cbb) != 0 || + !CBB_add_u8_length_prefixed(&cbb, &contents) || !CBB_add_u8_length_prefixed(&cbb, &contents) || !CBB_add_u8(&contents, 1) || + CBB_len(&contents) != 1 || + !CBB_flush(&cbb) || + CBB_len(&cbb) != 3 || !CBB_add_u16_length_prefixed(&cbb, &contents) || !CBB_add_u16(&contents, 0x203) || !CBB_add_u24_length_prefixed(&cbb, &contents) || diff --git a/crypto/bytestring/cbb.c b/crypto/bytestring/cbb.c index 74573c90..a1bc0121 100644 --- a/crypto/bytestring/cbb.c +++ b/crypto/bytestring/cbb.c @@ -179,28 +179,28 @@ int CBB_flush(CBB *cbb) { return 0; } - if (cbb->child == NULL || cbb->pending_len_len == 0) { + if (cbb->child == NULL || cbb->child->pending_len_len == 0) { return 1; } - child_start = cbb->offset + cbb->pending_len_len; + child_start = cbb->child->offset + cbb->child->pending_len_len; if (!CBB_flush(cbb->child) || - child_start < cbb->offset || + child_start < cbb->child->offset || cbb->base->len < child_start) { return 0; } len = cbb->base->len - child_start; - if (cbb->pending_is_asn1) { + if (cbb->child->pending_is_asn1) { /* For ASN.1 we assume that we'll only need a single byte for the length. * If that turned out to be incorrect, we have to move the contents along * in order to make space. */ size_t len_len; uint8_t initial_length_byte; - assert (cbb->pending_len_len == 1); + assert (cbb->child->pending_len_len == 1); if (len > 0xfffffffe) { /* Too large. */ @@ -232,12 +232,13 @@ int CBB_flush(CBB *cbb) { memmove(cbb->base->buf + child_start + extra_bytes, cbb->base->buf + child_start, len); } - cbb->base->buf[cbb->offset++] = initial_length_byte; - cbb->pending_len_len = len_len - 1; + cbb->base->buf[cbb->child->offset++] = initial_length_byte; + cbb->child->pending_len_len = len_len - 1; } - for (i = cbb->pending_len_len - 1; i < cbb->pending_len_len; i--) { - cbb->base->buf[cbb->offset + i] = len; + for (i = cbb->child->pending_len_len - 1; i < cbb->child->pending_len_len; + i--) { + cbb->base->buf[cbb->child->offset + i] = len; len >>= 8; } if (len != 0) { @@ -246,17 +247,15 @@ int CBB_flush(CBB *cbb) { cbb->child->base = NULL; cbb->child = NULL; - cbb->pending_len_len = 0; - cbb->pending_is_asn1 = 0; - cbb->offset = 0; return 1; } size_t CBB_len(const CBB *cbb) { assert(cbb->child == NULL); + assert(cbb->offset + cbb->pending_len_len <= cbb->base->len); - return cbb->base->len; + return cbb->base->len - cbb->offset - cbb->pending_len_len; } static int cbb_add_length_prefixed(CBB *cbb, CBB *out_contents, @@ -267,7 +266,7 @@ static int cbb_add_length_prefixed(CBB *cbb, CBB *out_contents, return 0; } - cbb->offset = cbb->base->len; + size_t offset = cbb->base->len; if (!cbb_buffer_add(cbb->base, &prefix_bytes, len_len)) { return 0; } @@ -276,8 +275,9 @@ static int cbb_add_length_prefixed(CBB *cbb, CBB *out_contents, memset(out_contents, 0, sizeof(CBB)); out_contents->base = cbb->base; cbb->child = out_contents; - cbb->pending_len_len = len_len; - cbb->pending_is_asn1 = 0; + cbb->child->offset = offset; + cbb->child->pending_len_len = len_len; + cbb->child->pending_is_asn1 = 0; return 1; } @@ -305,7 +305,7 @@ int CBB_add_asn1(CBB *cbb, CBB *out_contents, uint8_t tag) { return 0; } - cbb->offset = cbb->base->len; + size_t offset = cbb->base->len; if (!CBB_add_u8(cbb, 0)) { return 0; } @@ -313,8 +313,9 @@ int CBB_add_asn1(CBB *cbb, CBB *out_contents, uint8_t tag) { memset(out_contents, 0, sizeof(CBB)); out_contents->base = cbb->base; cbb->child = out_contents; - cbb->pending_len_len = 1; - cbb->pending_is_asn1 = 1; + cbb->child->offset = offset; + cbb->child->pending_len_len = 1; + cbb->child->pending_is_asn1 = 1; return 1; } @@ -367,13 +368,10 @@ void CBB_discard_child(CBB *cbb) { return; } - cbb->base->len = cbb->offset; + cbb->base->len = cbb->child->offset; cbb->child->base = NULL; cbb->child = NULL; - cbb->pending_len_len = 0; - cbb->pending_is_asn1 = 0; - cbb->offset = 0; } int CBB_add_asn1_uint64(CBB *cbb, uint64_t value) { diff --git a/include/openssl/bytestring.h b/include/openssl/bytestring.h index 906e7e88..9e12d498 100644 --- a/include/openssl/bytestring.h +++ b/include/openssl/bytestring.h @@ -238,13 +238,13 @@ struct cbb_buffer_st { struct cbb_st { struct cbb_buffer_st *base; - /* offset is the offset from the start of |base->buf| to the position of any - * pending length-prefix. */ - size_t offset; /* child points to a child CBB if a length-prefix is pending. */ CBB *child; - /* pending_len_len contains the number of bytes in a pending length-prefix, - * or zero if no length-prefix is pending. */ + /* offset is the number of bytes from the start of |base->buf| to this |CBB|'s + * pending length prefix. */ + size_t offset; + /* pending_len_len contains the number of bytes in this |CBB|'s pending + * length-prefix, or zero if no length-prefix is pending. */ uint8_t pending_len_len; char pending_is_asn1; /* is_top_level is true iff this is a top-level |CBB| (as opposed to a child @@ -292,12 +292,11 @@ OPENSSL_EXPORT int CBB_finish(CBB *cbb, uint8_t **out_data, size_t *out_len); * on error. */ OPENSSL_EXPORT int CBB_flush(CBB *cbb); -/* CBB_len returns the number of bytes written to |cbb|'s top-level |CBB|. It - * may be compared before and after an operation to determine how many bytes - * were written. +/* CBB_len returns the number of bytes written to |cbb|. It does not flush + * |cbb|. * - * It is a fatal error to call this on a CBB with any active children. This does - * not flush |cbb|. */ + * To avoid unfinalized length prefixes, it is a fatal error to call this on a + * CBB with any active children. */ OPENSSL_EXPORT size_t CBB_len(const CBB *cbb); /* CBB_add_u8_length_prefixed sets |*out_contents| to a new child of |cbb|. The diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index f0b792e5..c0ef97ef 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -2185,7 +2185,6 @@ int ssl_add_clienthello_tlsext(SSL *ssl, CBB *out, size_t header_len) { return 1; } - size_t orig_len = CBB_len(out); CBB extensions; if (!CBB_add_u16_length_prefixed(out, &extensions)) { goto err; @@ -2219,7 +2218,7 @@ int ssl_add_clienthello_tlsext(SSL *ssl, CBB *out, size_t header_len) { } if (!SSL_IS_DTLS(ssl)) { - header_len += CBB_len(&extensions) - orig_len; + header_len += 2 + CBB_len(&extensions); if (header_len > 0xff && header_len < 0x200) { /* Add padding to workaround bugs in F5 terminators. See RFC 7685. * @@ -2246,10 +2245,8 @@ int ssl_add_clienthello_tlsext(SSL *ssl, CBB *out, size_t header_len) { } } - /* If only two bytes have been written then the extensions are actually empty - * and those two bytes are the zero length. In that case, we don't bother - * sending the extensions length. */ - if (CBB_len(&extensions) - orig_len == 2) { + /* Discard empty extensions blocks. */ + if (CBB_len(&extensions) == 0) { CBB_discard_child(out); } @@ -2261,8 +2258,6 @@ err: } int ssl_add_serverhello_tlsext(SSL *ssl, CBB *out) { - const size_t orig_len = CBB_len(out); - CBB extensions; if (!CBB_add_u16_length_prefixed(out, &extensions)) { goto err; @@ -2286,10 +2281,8 @@ int ssl_add_serverhello_tlsext(SSL *ssl, CBB *out) { goto err; } - /* If only two bytes have been written then the extensions are actually empty - * and those two bytes are the zero length. In that case, we don't bother - * sending the extensions length. */ - if (CBB_len(&extensions) - orig_len == 2) { + /* Discard empty extensions blocks. */ + if (CBB_len(&extensions) == 0) { CBB_discard_child(out); }