Compute ASN.1 BIT STRING sizes more consistently.

OpenSSL's BIT STRING representation has two modes, one where it
implicitly trims trailing zeros and the other where the number of unused
bits is explicitly set. This means logic in ASN1_item_verify, or
elsewhere in callers, that checks flags and ASN1_STRING_length is
inconsistent with i2c_ASN1_BIT_STRING.

Add ASN1_BIT_STRING_num_bytes for code that needs to deal with X.509
using BIT STRING for some fields instead of OCTET STRING. Switch
ASN1_item_verify to it. Some external code does this too, so export it
as public API.

This is mostly a theoretical issue. All parsed BIT STRINGS use explicit
byte strings, and there are no APIs (apart from not-yet-opaquified
structs) to specify the ASN1_STRING in X509, etc., structures. We
intentionally made X509_set1_signature_value, etc., internally construct
the ASN1_STRING. Still having an API is more consistent and helps nudge
callers towards rejecting excess bits when they want bytes.

It may also be worth a public API for consistently accessing the bit
count. I've left it alone for now because I've not seen callers that
need it, and it saves worrying about bytes-to-bits overflows.

This also fixes a bug in the original version of the truncating logic
when the entire string was all zeros, and const-corrects a few
parameters.

Change-Id: I9d29842a3d3264b0cde61ca8cfea07d02177dbc2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48225
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>

crypto/asn1/a_bitstr.c

@@ -65,64 +65,69 @@
 #include "../internal.h"
 
 
-int ASN1_BIT_STRING_set(ASN1_BIT_STRING *x, unsigned char *d, int len)
+int ASN1_BIT_STRING_set(ASN1_BIT_STRING *x, const unsigned char *d, int len)
 {
     return ASN1_STRING_set(x, d, len);
 }
 
-int i2c_ASN1_BIT_STRING(const ASN1_BIT_STRING *a, unsigned char **pp)
-{
-    int ret, j, bits, len;
-    unsigned char *p, *d;
-
-    if (a == NULL)
-        return (0);
-
-    len = a->length;
+static int asn1_bit_string_length(const ASN1_BIT_STRING *str,
+                                  uint8_t *out_padding_bits) {
+    int len = str->length;
+    if (str->flags & ASN1_STRING_FLAG_BITS_LEFT) {
+        // If the string is already empty, it cannot have padding bits.
+        *out_padding_bits = len == 0 ? 0 : str->flags & 0x07;
+        return len;
+    }
 
+    // TODO(davidben): If we move this logic to |ASN1_BIT_STRING_set_bit|, can
+    // we remove this representation?
+    while (len > 0 && str->data[len - 1] == 0) {
+        len--;
+    }
+    uint8_t padding_bits = 0;
     if (len > 0) {
-        if (a->flags & ASN1_STRING_FLAG_BITS_LEFT) {
-            bits = (int)a->flags & 0x07;
-        } else {
-            for (; len > 0; len--) {
-                if (a->data[len - 1])
-                    break;
+        uint8_t last = str->data[len - 1];
+        assert(last != 0);
+        for (; padding_bits < 7; padding_bits++) {
+            if (last & (1 << padding_bits)) {
+                break;
             }
-            j = a->data[len - 1];
-            if (j & 0x01)
-                bits = 0;
-            else if (j & 0x02)
-                bits = 1;
-            else if (j & 0x04)
-                bits = 2;
-            else if (j & 0x08)
-                bits = 3;
-            else if (j & 0x10)
-                bits = 4;
-            else if (j & 0x20)
-                bits = 5;
-            else if (j & 0x40)
-                bits = 6;
-            else if (j & 0x80)
-                bits = 7;
-            else
-                bits = 0;       /* should not happen */
         }
-    } else
-        bits = 0;
+    }
+    *out_padding_bits = padding_bits;
+    return len;
+}
 
-    ret = 1 + len;
-    if (pp == NULL)
-        return (ret);
+int ASN1_BIT_STRING_num_bytes(const ASN1_BIT_STRING *str, size_t *out) {
+    uint8_t padding_bits;
+    int len = asn1_bit_string_length(str, &padding_bits);
+    if (padding_bits != 0) {
+        return 0;
+    }
+    *out = len;
+    return 1;
+}
 
-    p = *pp;
+int i2c_ASN1_BIT_STRING(const ASN1_BIT_STRING *a, unsigned char **pp)
+{
+    if (a == NULL) {
+        return 0;
+    }
+
+    uint8_t bits;
+    int len = asn1_bit_string_length(a, &bits);
+    int ret = 1 + len;
+    if (pp == NULL) {
+        return ret;
+    }
 
-    *(p++) = (unsigned char)bits;
-    d = a->data;
-    OPENSSL_memcpy(p, d, len);
+    uint8_t *p = *pp;
+    *(p++) = bits;
+    OPENSSL_memcpy(p, a->data, len);
+    if (len > 0) {
+        p[len - 1] &= (0xff << bits);
+    }
     p += len;
-    if (len > 0)
-        p[-1] &= (0xff << bits);
     *pp = p;
     return (ret);
 }
@@ -251,7 +256,7 @@ int ASN1_BIT_STRING_get_bit(const ASN1_BIT_STRING *a, int n)
  * 'len' is the length of 'flags'.
  */
 int ASN1_BIT_STRING_check(const ASN1_BIT_STRING *a,
-                          unsigned char *flags, int flags_len)
+                          const unsigned char *flags, int flags_len)
 {
     int i, ok;
     /* Check if there is one bit set at all. */

crypto/asn1/asn1_test.cc

@@ -100,20 +100,23 @@ TEST(ASN1Test, IntegerSetting) {
 template <typename T>
 void TestSerialize(T obj, int (*i2d_func)(T a, uint8_t **pp),
                    bssl::Span<const uint8_t> expected) {
-  int len = static_cast<int>(expected.size());
-  ASSERT_EQ(i2d_func(obj, nullptr), len);
+  // Test the allocating version first. It is easiest to debug.
+  uint8_t *ptr = nullptr;
+  int len = i2d_func(obj, &ptr);
+  ASSERT_GT(len, 0);
+  EXPECT_EQ(Bytes(expected), Bytes(ptr, len));
+  OPENSSL_free(ptr);
+
+  len = i2d_func(obj, nullptr);
+  ASSERT_GT(len, 0);
+  EXPECT_EQ(len, static_cast<int>(expected.size()));
 
-  std::vector<uint8_t> buf(expected.size());
-  uint8_t *ptr = buf.data();
-  ASSERT_EQ(i2d_func(obj, &ptr), len);
+  std::vector<uint8_t> buf(len);
+  ptr = buf.data();
+  len = i2d_func(obj, &ptr);
+  ASSERT_EQ(len, static_cast<int>(expected.size()));
   EXPECT_EQ(ptr, buf.data() + buf.size());
   EXPECT_EQ(Bytes(expected), Bytes(buf));
-
-  // Test the allocating version.
-  ptr = nullptr;
-  ASSERT_EQ(i2d_func(obj, &ptr), len);
-  EXPECT_EQ(Bytes(expected), Bytes(ptr, expected.size()));
-  OPENSSL_free(ptr);
 }
 
 TEST(ASN1Test, SerializeObject) {
@@ -238,6 +241,163 @@ TEST(ASN1Test, ASN1ObjectReuse) {
   ASN1_OBJECT_free(obj);
 }
 
+TEST(ASN1Test, BitString) {
+  const size_t kNotWholeBytes = static_cast<size_t>(-1);
+  const struct {
+    std::vector<uint8_t> in;
+    size_t num_bytes;
+  } kValidInputs[] = {
+      // Empty bit string
+      {{0x03, 0x01, 0x00}, 0},
+      // 0b1
+      {{0x03, 0x02, 0x07, 0x80}, kNotWholeBytes},
+      // 0b1010
+      {{0x03, 0x02, 0x04, 0xa0}, kNotWholeBytes},
+      // 0b1010101
+      {{0x03, 0x02, 0x01, 0xaa}, kNotWholeBytes},
+      // 0b10101010
+      {{0x03, 0x02, 0x00, 0xaa}, 1},
+      // Bits 0 and 63 are set
+      {{0x03, 0x09, 0x00, 0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01}, 8},
+      // 64 zero bits
+      {{0x03, 0x09, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}, 8},
+  };
+  for (const auto &test : kValidInputs) {
+    SCOPED_TRACE(Bytes(test.in));
+    // The input should parse and round-trip correctly.
+    const uint8_t *ptr = test.in.data();
+    bssl::UniquePtr<ASN1_BIT_STRING> val(
+        d2i_ASN1_BIT_STRING(nullptr, &ptr, test.in.size()));
+    ASSERT_TRUE(val);
+    TestSerialize(val.get(), i2d_ASN1_BIT_STRING, test.in);
+
+    // Check the byte count.
+    size_t num_bytes;
+    if (test.num_bytes == kNotWholeBytes) {
+      EXPECT_FALSE(ASN1_BIT_STRING_num_bytes(val.get(), &num_bytes));
+    } else {
+      ASSERT_TRUE(ASN1_BIT_STRING_num_bytes(val.get(), &num_bytes));
+      EXPECT_EQ(num_bytes, test.num_bytes);
+    }
+  }
+
+  const std::vector<uint8_t> kInvalidInputs[] = {
+      // Wrong tag
+      {0x04, 0x01, 0x00},
+      // Missing leading byte
+      {0x03, 0x00},
+      // Leading byte too high
+      {0x03, 0x02, 0x08, 0x00},
+      {0x03, 0x02, 0xff, 0x00},
+      // TODO(https://crbug.com/boringssl/354): Reject these inputs.
+      // Empty bit strings must have a zero leading byte.
+      // {0x03, 0x01, 0x01},
+      // Unused bits must all be zero.
+      // {0x03, 0x02, 0x06, 0xc1 /* 0b11000001 */},
+  };
+  for (const auto &test : kInvalidInputs) {
+    SCOPED_TRACE(Bytes(test));
+    const uint8_t *ptr = test.data();
+    bssl::UniquePtr<ASN1_BIT_STRING> val(
+        d2i_ASN1_BIT_STRING(nullptr, &ptr, test.size()));
+    EXPECT_FALSE(val);
+  }
+}
+
+TEST(ASN1Test, SetBit) {
+  bssl::UniquePtr<ASN1_BIT_STRING> val(ASN1_BIT_STRING_new());
+  ASSERT_TRUE(val);
+  static const uint8_t kBitStringEmpty[] = {0x03, 0x01, 0x00};
+  TestSerialize(val.get(), i2d_ASN1_BIT_STRING, kBitStringEmpty);
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 0));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 100));
+
+  // Set a few bits via |ASN1_BIT_STRING_set_bit|.
+  ASSERT_TRUE(ASN1_BIT_STRING_set_bit(val.get(), 0, 1));
+  ASSERT_TRUE(ASN1_BIT_STRING_set_bit(val.get(), 1, 1));
+  ASSERT_TRUE(ASN1_BIT_STRING_set_bit(val.get(), 2, 0));
+  ASSERT_TRUE(ASN1_BIT_STRING_set_bit(val.get(), 3, 1));
+  static const uint8_t kBitString1101[] = {0x03, 0x02, 0x04, 0xd0};
+  TestSerialize(val.get(), i2d_ASN1_BIT_STRING, kBitString1101);
+  EXPECT_EQ(1, ASN1_BIT_STRING_get_bit(val.get(), 0));
+  EXPECT_EQ(1, ASN1_BIT_STRING_get_bit(val.get(), 1));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 2));
+  EXPECT_EQ(1, ASN1_BIT_STRING_get_bit(val.get(), 3));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 4));
+
+  // Bits that were set may be cleared.
+  ASSERT_TRUE(ASN1_BIT_STRING_set_bit(val.get(), 1, 0));
+  static const uint8_t kBitString1001[] = {0x03, 0x02, 0x04, 0x90};
+  TestSerialize(val.get(), i2d_ASN1_BIT_STRING, kBitString1001);
+  EXPECT_EQ(1, ASN1_BIT_STRING_get_bit(val.get(), 0));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 1));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 2));
+  EXPECT_EQ(1, ASN1_BIT_STRING_get_bit(val.get(), 3));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 4));
+
+  // Clearing trailing bits truncates the string.
+  ASSERT_TRUE(ASN1_BIT_STRING_set_bit(val.get(), 3, 0));
+  static const uint8_t kBitString1[] = {0x03, 0x02, 0x07, 0x80};
+  TestSerialize(val.get(), i2d_ASN1_BIT_STRING, kBitString1);
+  EXPECT_EQ(1, ASN1_BIT_STRING_get_bit(val.get(), 0));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 1));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 2));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 3));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 4));
+
+  // Bits may be set beyond the end of the string.
+  ASSERT_TRUE(ASN1_BIT_STRING_set_bit(val.get(), 63, 1));
+  static const uint8_t kBitStringLong[] = {0x03, 0x09, 0x00, 0x80, 0x00, 0x00,
+                                           0x00, 0x00, 0x00, 0x00, 0x01};
+  TestSerialize(val.get(), i2d_ASN1_BIT_STRING, kBitStringLong);
+  EXPECT_EQ(1, ASN1_BIT_STRING_get_bit(val.get(), 0));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 62));
+  EXPECT_EQ(1, ASN1_BIT_STRING_get_bit(val.get(), 63));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 64));
+
+  // The string can be truncated back down again.
+  ASSERT_TRUE(ASN1_BIT_STRING_set_bit(val.get(), 63, 0));
+  TestSerialize(val.get(), i2d_ASN1_BIT_STRING, kBitString1);
+  EXPECT_EQ(1, ASN1_BIT_STRING_get_bit(val.get(), 0));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 62));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 63));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 64));
+
+  // |ASN1_BIT_STRING_set_bit| also truncates when starting from a parsed
+  // string.
+  const uint8_t *ptr = kBitStringLong;
+  val.reset(d2i_ASN1_BIT_STRING(nullptr, &ptr, sizeof(kBitStringLong)));
+  ASSERT_TRUE(val);
+  TestSerialize(val.get(), i2d_ASN1_BIT_STRING, kBitStringLong);
+  ASSERT_TRUE(ASN1_BIT_STRING_set_bit(val.get(), 63, 0));
+  TestSerialize(val.get(), i2d_ASN1_BIT_STRING, kBitString1);
+  EXPECT_EQ(1, ASN1_BIT_STRING_get_bit(val.get(), 0));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 62));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 63));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 64));
+
+  // A parsed bit string preserves trailing zero bits.
+  static const uint8_t kBitString10010[] = {0x03, 0x02, 0x03, 0x90};
+  ptr = kBitString10010;
+  val.reset(d2i_ASN1_BIT_STRING(nullptr, &ptr, sizeof(kBitString10010)));
+  ASSERT_TRUE(val);
+  TestSerialize(val.get(), i2d_ASN1_BIT_STRING, kBitString10010);
+  // But |ASN1_BIT_STRING_set_bit| will truncate it even if otherwise a no-op.
+  ASSERT_TRUE(ASN1_BIT_STRING_set_bit(val.get(), 0, 1));
+  TestSerialize(val.get(), i2d_ASN1_BIT_STRING, kBitString1001);
+  EXPECT_EQ(1, ASN1_BIT_STRING_get_bit(val.get(), 0));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 62));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 63));
+  EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 64));
+
+  // By default, a BIT STRING implicitly truncates trailing zeros.
+  val.reset(ASN1_BIT_STRING_new());
+  ASSERT_TRUE(val);
+  static const uint8_t kZeros[64] = {0};
+  ASSERT_TRUE(ASN1_STRING_set(val.get(), kZeros, sizeof(kZeros)));
+  TestSerialize(val.get(), i2d_ASN1_BIT_STRING, kBitStringEmpty);
+}
+
 // The ASN.1 macros do not work on Windows shared library builds, where usage of
 // |OPENSSL_EXPORT| is a bit stricter.
 #if !defined(OPENSSL_WINDOWS) || !defined(BORINGSSL_SHARED_LIBRARY)

crypto/x509/a_verify.c

@@ -70,22 +70,26 @@
 #include "internal.h"
 
 int ASN1_item_verify(const ASN1_ITEM *it, X509_ALGOR *a,
-                     ASN1_BIT_STRING *signature, void *asn, EVP_PKEY *pkey)
-{
-    EVP_MD_CTX ctx;
-    uint8_t *buf_in = NULL;
-    int ret = 0, inl = 0;
-
+                     const ASN1_BIT_STRING *signature, void *asn,
+                     EVP_PKEY *pkey) {
     if (!pkey) {
         OPENSSL_PUT_ERROR(X509, ERR_R_PASSED_NULL_PARAMETER);
         return 0;
     }
 
-    if (signature->type == V_ASN1_BIT_STRING && signature->flags & 0x7) {
-        OPENSSL_PUT_ERROR(X509, X509_R_INVALID_BIT_STRING_BITS_LEFT);
-        return 0;
+    size_t sig_len;
+    if (signature->type == V_ASN1_BIT_STRING) {
+        if (!ASN1_BIT_STRING_num_bytes(signature, &sig_len)) {
+            OPENSSL_PUT_ERROR(X509, X509_R_INVALID_BIT_STRING_BITS_LEFT);
+            return 0;
+        }
+    } else {
+        sig_len = (size_t)ASN1_STRING_length(signature);
     }
 
+    EVP_MD_CTX ctx;
+    uint8_t *buf_in = NULL;
+    int ret = 0, inl = 0;
     EVP_MD_CTX_init(&ctx);
 
     if (!x509_digest_verify_init(&ctx, a, pkey)) {
@@ -99,7 +103,7 @@ int ASN1_item_verify(const ASN1_ITEM *it, X509_ALGOR *a,
         goto err;
     }
 
-    if (!EVP_DigestVerify(&ctx, signature->data, (size_t)signature->length,
+    if (!EVP_DigestVerify(&ctx, ASN1_STRING_get0_data(signature), sig_len,
                           buf_in, inl)) {
         OPENSSL_PUT_ERROR(X509, ERR_R_EVP_LIB);
         goto err;