commit 3c6085b6ae982a80633bf5369c274036702c6848
author David Benjamin <davidben@google.com> Mon Feb 27 16:32:32 2023 -0500
committer Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Mon Feb 27 21:49:02 2023 +0000
tree 2e58a272d0b4a5ad7ae2046be8576c7ca3e22240
parent 92859cc7484ac178e77825ccb86f95edcf0b6c49

Workaround yet more NULL + 0 language bugs

No new tests because they're actually caught by our own tests. I just
forgot to put UBSan on CI! Will fix this shortly.

For BLAKE2, fix this by checking for zero. For c2i_ASN1_INTEGER,
rewrite it with CBS, which has the side effect of avoiding this. (It's
effectively maintaining in->data + start as a temporary, rather than
start itself.)

Bug: fuchsia:46910
Change-Id: I9366f1ba4fd0b0140d64c56e0534d7b060ab90e5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57687
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>

crypto/asn1/a_int.c

diff --git a/crypto/asn1/a_int.c b/crypto/asn1/a_int.c
index 78d1f09..27845c0 100644
--- a/crypto/asn1/a_int.c
+++ b/crypto/asn1/a_int.c
@@ -122,14 +122,17 @@
   // |ASN1_INTEGER|s should be represented minimally, but it is possible to
   // construct invalid ones. Skip leading zeros so this does not produce an
   // invalid encoding or break invariants.
-  int start = 0;
-  while (start < in->length && in->data[start] == 0) {
-    start++;
+  CBS cbs;
+  CBS_init(&cbs, in->data, in->length);
+  while (CBS_len(&cbs) > 0 && CBS_data(&cbs)[0] == 0) {
+    CBS_skip(&cbs, 1);
   }
 
   int is_negative = (in->type & V_ASN1_NEG) != 0;
-  int pad;
-  if (start >= in->length) {
+  size_t pad;
+  CBS copy = cbs;
+  uint8_t msb;
+  if (!CBS_get_u8(&copy, &msb)) {
     // Zero is represented as a single byte.
     is_negative = 0;
     pad = 1;
@@ -138,20 +141,19 @@
     // through 0x00...01 and need an extra byte to be negative.
     // 0x01...00 through 0x80...00 have a two's complement of 0xfe...ff
     // through 0x80...00 and can be negated as-is.
-    pad = in->data[start] > 0x80 ||
-          (in->data[start] == 0x80 &&
-           !is_all_zeros(in->data + start + 1, in->length - start - 1));
+    pad = msb > 0x80 ||
+          (msb == 0x80 && !is_all_zeros(CBS_data(&copy), CBS_len(&copy)));
   } else {
     // If the high bit is set, the signed representation needs an extra
     // byte to be positive.
-    pad = (in->data[start] & 0x80) != 0;
+    pad = (msb & 0x80) != 0;
   }
 
-  if (in->length - start > INT_MAX - pad) {
+  if (CBS_len(&cbs) > INT_MAX - pad) {
     OPENSSL_PUT_ERROR(ASN1, ERR_R_OVERFLOW);
     return 0;
   }
-  int len = pad + in->length - start;
+  int len = (int)(pad + CBS_len(&cbs));
   assert(len > 0);
   if (outp == NULL) {
     return len;
@@ -160,7 +162,7 @@
   if (pad) {
     (*outp)[0] = 0;
   }
-  OPENSSL_memcpy(*outp + pad, in->data + start, in->length - start);
+  OPENSSL_memcpy(*outp + pad, CBS_data(&cbs), CBS_len(&cbs));
   if (is_negative) {
     negate_twos_complement(*outp, len);
     assert((*outp)[0] >= 0x80);

crypto/blake2/blake2.c

diff --git a/crypto/blake2/blake2.c b/crypto/blake2/blake2.c
index 5c6b17e..848ed4f 100644
--- a/crypto/blake2/blake2.c
+++ b/crypto/blake2/blake2.c
@@ -105,8 +105,12 @@
 }
 
 void BLAKE2B256_Update(BLAKE2B_CTX *b2b, const void *in_data, size_t len) {
-  const uint8_t *data = (const uint8_t *)in_data;
+  if (len == 0) {
+    // Work around a C language bug. See https://crbug.com/1019588.
+    return;
+  }
 
+  const uint8_t *data = in_data;
   size_t todo = sizeof(b2b->block.bytes) - b2b->block_used;
   if (todo > len) {
     todo = len;