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 <agl@google.com>
diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc
index 99f8bd1..8a17812 100644
--- a/crypto/bytestring/bytestring_test.cc
+++ b/crypto/bytestring/bytestring_test.cc
@@ -344,12 +344,14 @@
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 74573c9..a1bc012 100644
--- a/crypto/bytestring/cbb.c
+++ b/crypto/bytestring/cbb.c
@@ -179,28 +179,28 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 906e7e8..9e12d49 100644
--- a/include/openssl/bytestring.h
+++ b/include/openssl/bytestring.h
@@ -238,13 +238,13 @@
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 @@
* 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 f0b792e..c0ef97e 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -2185,7 +2185,6 @@
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 @@
}
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 @@
}
}
- /* 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 @@
}
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 @@
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);
}
