commit a01deee96b1c9a28ff7d09ac6bcc75b91171b458
author David Benjamin <davidben@chromium.org> Tue Dec 08 18:56:31 2015 -0500
committer Adam Langley <agl@google.com> Wed Dec 16 21:16:12 2015 +0000
tree f4ae2b6183ecfe101cb758e666a1eeb15efb89a8
parent 77385bb43d02af71bf4e74890ba27b5bee349bd5

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>

ssl/t1_lib.c

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);
   }