diff options
author | David Benjamin <davidben@google.com> | 2016-07-19 08:08:24 +0300 |
---|---|---|
committer | CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> | 2016-07-19 23:51:51 +0300 |
commit | 93a034a7d796a49ec8376829dcfb2531cc6df54a (patch) | |
tree | a40b9b94d700e7a7e962e42d3e2c418951e34011 /crypto | |
parent | 0af8240d4eaeddc18a3b83c70b7b59137392d17e (diff) |
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>
Diffstat (limited to 'crypto')
-rw-r--r-- | crypto/bytestring/bytestring_test.cc | 79 | ||||
-rw-r--r-- | crypto/bytestring/cbb.c | 29 |
2 files changed, 90 insertions, 18 deletions
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; } |