diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc index 31ee51c4..5441da77 100644 --- a/crypto/bytestring/bytestring_test.cc +++ b/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) || - CBB_add_u8(&cbb, 1) || - !CBB_finish(&cbb, &out_buf, &out_size) || + if (!CBB_init_fixed(cbb.get(), NULL, 0) || + !CBB_finish(cbb.get(), &out_buf, &out_size) || out_buf != NULL || out_size != 0) { return false; } - if (!CBB_init_fixed(&cbb, buf, 1) || - !CBB_add_u8(&cbb, 1) || - CBB_add_u8(&cbb, 2) || - !CBB_finish(&cbb, &out_buf, &out_size) || + cbb.Reset(); + if (!CBB_init_fixed(cbb.get(), buf, 1) || + !CBB_add_u8(cbb.get(), 1) || + !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; } diff --git a/crypto/bytestring/cbb.c b/crypto/bytestring/cbb.c index 6cb7988c..9b38a6b1 100644 --- a/crypto/bytestring/cbb.c +++ b/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; } diff --git a/include/openssl/bytestring.h b/include/openssl/bytestring.h index c24281a5..68138bc2 100644 --- a/include/openssl/bytestring.h +++ b/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 {