CBBs are in an undefined state after an operation failed.

Our CBB patterns do not make it safe to use a CBB after any operation
failed. Suppose one does:

  int add_to_cbb(CBB *cbb) {
    CBB child;
    return CBB_add_u8(cbb, 1) &&
           CBB_add_u8_length_prefixed(cbb, &child) &&
           CBB_add_u8(&child, 2) &&
           /* Flush |cbb| before |child| goes out of scoped. */
           CBB_flush(cbb);
  }

If one of the earlier operations fails, any attempt to use |cbb| (except
CBB_cleanup) would hit a memory error. Doing this would be a bug anyway,
since the CBB would be in an undefined state anyway (wrote only half my
object), but the memory error is bad manners.

Officially document that using a CBB after failure is illegal and, to
avoid the memory error, set a poison bit on the cbb_buffer_st to prevent
all future operations. In theory we could make failure +
CBB_discard_child work, but this is not very useful and would require a
more complex CBB pattern.

Change-Id: I4303ee1c326785849ce12b5f7aa8bbde6b95d2ec
Reviewed-on: https://boringssl-review.googlesource.com/8840
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

crypto/bytestring/bytestring_test.cc

@@ -297,29 +297,35 @@ static bool TestCBBBasic() {
 }
 
 static bool TestCBBFixed() {
-  CBB cbb;
+  ScopedCBB cbb;
   uint8_t buf[1];
   uint8_t *out_buf;
   size_t out_size;
 
-  if (!CBB_init_fixed(&cbb, NULL, 0) ||
+  if (!CBB_init_fixed(cbb.get(), NULL, 0) ||
-      CBB_add_u8(&cbb, 1) ||
+      !CBB_finish(cbb.get(), &out_buf, &out_size) ||
-      !CBB_finish(&cbb, &out_buf, &out_size) ||
       out_buf != NULL ||
       out_size != 0) {
     return false;
   }
 
-  if (!CBB_init_fixed(&cbb, buf, 1) ||
+  cbb.Reset();
-      !CBB_add_u8(&cbb, 1) ||
+  if (!CBB_init_fixed(cbb.get(), buf, 1) ||
-      CBB_add_u8(&cbb, 2) ||
+      !CBB_add_u8(cbb.get(), 1) ||
-      !CBB_finish(&cbb, &out_buf, &out_size) ||
+      !CBB_finish(cbb.get(), &out_buf, &out_size) ||
       out_buf != buf ||
       out_size != 1 ||
       buf[0] != 1) {
     return false;
   }
 
+  cbb.Reset();
+  if (!CBB_init_fixed(cbb.get(), buf, 1) ||
+      !CBB_add_u8(cbb.get(), 1) ||
+      CBB_add_u8(cbb.get(), 2)) {
+    return false;
+  }
+
   return true;
 }
 
@@ -784,7 +790,12 @@ static bool TestCBBReserve() {
   ScopedCBB cbb;
   if (!CBB_init_fixed(cbb.get(), buf, sizeof(buf)) ||
       // Too large.
-      CBB_reserve(cbb.get(), &ptr, 11) ||
+      CBB_reserve(cbb.get(), &ptr, 11)) {
+    return false;
+  }
+
+  cbb.Reset();
+  if (!CBB_init_fixed(cbb.get(), buf, sizeof(buf)) ||
       // Successfully reserve the entire space.
       !CBB_reserve(cbb.get(), &ptr, 10) ||
       ptr != buf ||
@@ -797,6 +808,53 @@ static bool TestCBBReserve() {
   return true;
 }
 
+static bool TestStickyError() {
+  // Write an input that exceeds the limit for its length prefix.
+  ScopedCBB cbb;
+  CBB child;
+  static const uint8_t kZeros[256] = {0};
+  if (!CBB_init(cbb.get(), 0) ||
+      !CBB_add_u8_length_prefixed(cbb.get(), &child) ||
+      !CBB_add_bytes(&child, kZeros, sizeof(kZeros))) {
+    return false;
+  }
+
+  if (CBB_flush(cbb.get())) {
+    fprintf(stderr, "CBB_flush unexpectedly succeeded.\n");
+    return false;
+  }
+
+  // All future operations should fail.
+  uint8_t *ptr;
+  size_t len;
+  if (CBB_add_u8(cbb.get(), 0) ||
+      CBB_finish(cbb.get(), &ptr, &len)) {
+    fprintf(stderr, "Future operations unexpectedly succeeded.\n");
+    return false;
+  }
+
+  // Write an input that cannot fit in a fixed CBB.
+  cbb.Reset();
+  uint8_t buf;
+  if (!CBB_init_fixed(cbb.get(), &buf, 1)) {
+    return false;
+  }
+
+  if (CBB_add_bytes(cbb.get(), kZeros, sizeof(kZeros))) {
+    fprintf(stderr, "CBB_add_bytes unexpectedly succeeded.\n");
+    return false;
+  }
+
+  // All future operations should fail.
+  if (CBB_add_u8(cbb.get(), 0) ||
+      CBB_finish(cbb.get(), &ptr, &len)) {
+    fprintf(stderr, "Future operations unexpectedly succeeded.\n");
+    return false;
+  }
+
+  return true;
+}
+
 int main(void) {
   CRYPTO_library_init();
 
@@ -817,7 +875,8 @@ int main(void) {
       !TestASN1Uint64() ||
       !TestGetOptionalASN1Bool() ||
       !TestZero() ||
-      !TestCBBReserve()) {
+      !TestCBBReserve() ||
+      !TestStickyError()) {
     return 1;
   }
 

crypto/bytestring/cbb.c

@@ -37,6 +37,7 @@ static int cbb_init(CBB *cbb, uint8_t *buf, size_t cap) {
   base->len = 0;
   base->cap = cap;
   base->can_resize = 1;
+  base->error = 0;
 
   cbb->base = base;
   cbb->is_top_level = 1;
@@ -95,7 +96,7 @@ static int cbb_buffer_reserve(struct cbb_buffer_st *base, uint8_t **out,
   newlen = base->len + len;
   if (newlen < base->len) {
     /* Overflow */
-    return 0;
+    goto err;
   }
 
   if (newlen > base->cap) {
@@ -103,7 +104,7 @@ static int cbb_buffer_reserve(struct cbb_buffer_st *base, uint8_t **out,
     uint8_t *newbuf;
 
     if (!base->can_resize) {
-      return 0;
+      goto err;
     }
 
     if (newcap < base->cap || newcap < newlen) {
@@ -111,7 +112,7 @@ static int cbb_buffer_reserve(struct cbb_buffer_st *base, uint8_t **out,
     }
     newbuf = OPENSSL_realloc(base->buf, newcap);
     if (newbuf == NULL) {
-      return 0;
+      goto err;
     }
 
     base->buf = newbuf;
@@ -123,6 +124,10 @@ static int cbb_buffer_reserve(struct cbb_buffer_st *base, uint8_t **out,
   }
 
   return 1;
+
+err:
+  base->error = 1;
+  return 0;
 }
 
 static int cbb_buffer_add(struct cbb_buffer_st *base, uint8_t **out,
@@ -185,7 +190,10 @@ int CBB_finish(CBB *cbb, uint8_t **out_data, size_t *out_len) {
 int CBB_flush(CBB *cbb) {
   size_t child_start, i, len;
 
-  if (cbb->base == NULL) {
+  /* If |cbb->base| has hit an error, the buffer is in an undefined state, so
+   * fail all following calls. In particular, |cbb->child| may point to invalid
+   * memory. */
+  if (cbb->base == NULL || cbb->base->error) {
     return 0;
   }
 
@@ -198,7 +206,7 @@ int CBB_flush(CBB *cbb) {
   if (!CBB_flush(cbb->child) ||
       child_start < cbb->child->offset ||
       cbb->base->len < child_start) {
-    return 0;
+    goto err;
   }
 
   len = cbb->base->len - child_start;
@@ -214,7 +222,7 @@ int CBB_flush(CBB *cbb) {
 
     if (len > 0xfffffffe) {
       /* Too large. */
-      return 0;
+      goto err;
     } else if (len > 0xffffff) {
       len_len = 5;
       initial_length_byte = 0x80 | 4;
@@ -237,7 +245,7 @@ int CBB_flush(CBB *cbb) {
       /* We need to move the contents along in order to make space. */
       size_t extra_bytes = len_len - 1;
       if (!cbb_buffer_add(cbb->base, NULL, extra_bytes)) {
-        return 0;
+        goto err;
       }
       memmove(cbb->base->buf + child_start + extra_bytes,
               cbb->base->buf + child_start, len);
@@ -252,13 +260,17 @@ int CBB_flush(CBB *cbb) {
     len >>= 8;
   }
   if (len != 0) {
-    return 0;
+    goto err;
   }
 
   cbb->child->base = NULL;
   cbb->child = NULL;
 
   return 1;
+
+err:
+  cbb->base->error = 1;
+  return 0;
 }
 
 const uint8_t *CBB_data(const CBB *cbb) {
@@ -312,6 +324,7 @@ int CBB_add_u24_length_prefixed(CBB *cbb, CBB *out_contents) {
 int CBB_add_asn1(CBB *cbb, CBB *out_contents, uint8_t tag) {
   if ((tag & 0x1f) == 0x1f) {
     /* Long form identifier octets are not supported. */
+    cbb->base->error = 1;
     return 0;
   }
 

include/openssl/bytestring.h

@@ -242,7 +242,8 @@ OPENSSL_EXPORT int CBS_get_optional_asn1_bool(CBS *cbs, int *out, unsigned tag,
  * not be used again.
  *
  * If one needs to force a length prefix to be written out because a |CBB| is
- * going out of scope, use |CBB_flush|. */
+ * going out of scope, use |CBB_flush|. If an operation on a |CBB| fails, it is
+ * in an undefined state and must not be used except to call |CBB_cleanup|. */
 
 struct cbb_buffer_st {
   uint8_t *buf;
@@ -250,6 +251,8 @@ struct cbb_buffer_st {
   size_t cap;      /* The size of buf. */
   char can_resize; /* One iff |buf| is owned by this object. If not then |buf|
                       cannot be resized. */
+  char error;      /* One iff there was an error writing to this CBB. All future
+                      operations will fail. */
 };
 
 struct cbb_st {