Define CBS/CBB tags as uint32_t with a typedef.

We use unsigned, but we actually assume it is 32-bit for the bit-packing strategy. But also introduce a typedef to hint that callers shouldn't treat it as an arbitrary 32-bit integer. A typedef would also allow us to extend to uint64_t in the future, if we ever need to. Update-Note: Some APIs switch from unsigned * to uint32_t * out pointers. This is only source-compatible if unsigned and uint32_t are the exact same type. The CQ suggests this is indeed true. If they are not, replace unsigned with CBS_ASN1_TAG to fix the build. Bug: 525 Change-Id: I45cbe127c1aa252f5f6a169dca2e44d1e6e1d669 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54986 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]> (cherry picked from commit a1dffbfed9b78d77ab17c15fe2d189ca44704fd8)
pennyannn · Nov 28, 2022 · 964507d · 964507d
1 parent 740dd4c
commit 964507d
Show file tree

Hide file tree

Showing 13 changed files with 123 additions and 108 deletions.
diff --git a/crypto/asn1/asn1_lib.c b/crypto/asn1/asn1_lib.c
@@ -117,7 +117,7 @@ int ASN1_get_object(const unsigned char **inp, long *out_len, int *out_tag,
   // signature fields (see b/18228011). Make this only apply to that field,
   // while requiring DER elsewhere. Better yet, it should be limited to an
   // preprocessing step in that part of Android.
-  unsigned tag;
+  CBS_ASN1_TAG tag;
   size_t header_len;
   int indefinite;
   CBS cbs, body;

diff --git a/crypto/bytestring/ber.c b/crypto/bytestring/ber.c
@@ -24,11 +24,11 @@
 // kMaxDepth is a just a sanity limit. The code should be such that the length
 // of the input being processes always decreases. None the less, a very large
 // input could otherwise cause the stack to overflow.
-static const unsigned kMaxDepth = 2048;
+static const uint32_t kMaxDepth = 2048;
 
 // is_string_type returns one if |tag| is a string type and zero otherwise. It
 // ignores the constructed bit.
-static int is_string_type(unsigned tag) {
+static int is_string_type(CBS_ASN1_TAG tag) {
   // While BER supports constructed BIT STRINGS, OpenSSL misparses them. To
   // avoid acting on an ambiguous input, we do not support constructed BIT
   // STRINGS. See https://github.com/openssl/openssl/issues/12810.
@@ -55,7 +55,7 @@ static int is_string_type(unsigned tag) {
 // depending on whether an indefinite length element or constructed string was
 // found. The value of |orig_in| is not changed. It returns one on success (i.e.
 // |*ber_found| was set) and zero on error.
-static int cbs_find_ber(const CBS *orig_in, int *ber_found, unsigned depth) {
+static int cbs_find_ber(const CBS *orig_in, int *ber_found, uint32_t depth) {
   CBS in;
 
   if (depth > kMaxDepth) {
@@ -67,7 +67,7 @@ static int cbs_find_ber(const CBS *orig_in, int *ber_found, unsigned depth) {
 
   while (CBS_len(&in) > 0) {
     CBS contents;
-    unsigned tag;
+    CBS_ASN1_TAG tag;
     size_t header_len;
     int indefinite;
     if (!CBS_get_any_ber_asn1_element(&in, &contents, &tag, &header_len,
@@ -110,8 +110,8 @@ static int cbs_get_eoc(CBS *cbs) {
 // constructed string. If |looking_for_eoc| is set then any EOC elements found
 // will cause the function to return after consuming it. It returns one on
 // success and zero on error.
-static int cbs_convert_ber(CBS *in, CBB *out, unsigned string_tag,
-                           char looking_for_eoc, unsigned depth) {
+static int cbs_convert_ber(CBS *in, CBB *out, CBS_ASN1_TAG string_tag,
+                           int looking_for_eoc, uint32_t depth) {
   assert(!(string_tag & CBS_ASN1_CONSTRUCTED));
 
   if (depth > kMaxDepth) {
@@ -124,7 +124,7 @@ static int cbs_convert_ber(CBS *in, CBB *out, unsigned string_tag,
     }
 
     CBS contents;
-    unsigned tag, child_string_tag = string_tag;
+    CBS_ASN1_TAG tag, child_string_tag = string_tag;
     size_t header_len;
     int indefinite;
     CBB *out_contents, out_contents_storage;
@@ -142,7 +142,7 @@ static int cbs_convert_ber(CBS *in, CBB *out, unsigned string_tag,
       }
       out_contents = out;
     } else {
-      unsigned out_tag = tag;
+      CBS_ASN1_TAG out_tag = tag;
       if ((tag & CBS_ASN1_CONSTRUCTED) && is_string_type(tag)) {
         // If a constructed string, clear the constructed bit and inform
         // children to concatenate bodies.
@@ -221,7 +221,8 @@ int CBS_asn1_ber_to_der(CBS *in, CBS *out, uint8_t **out_storage) {
 }
 
 int CBS_get_asn1_implicit_string(CBS *in, CBS *out, uint8_t **out_storage,
-                                 unsigned outer_tag, unsigned inner_tag) {
+                                 CBS_ASN1_TAG outer_tag,
+                                 CBS_ASN1_TAG inner_tag) {
   assert(!(outer_tag & CBS_ASN1_CONSTRUCTED));
   assert(!(inner_tag & CBS_ASN1_CONSTRUCTED));
   assert(is_string_type(inner_tag));

diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc
@@ -249,7 +249,7 @@ TEST(CBSTest, GetASN1) {
   EXPECT_FALSE(CBS_get_optional_asn1_uint64(
       &data, &value, CBS_ASN1_CONTEXT_SPECIFIC | CBS_ASN1_CONSTRUCTED | 1, 42));
 
-  unsigned tag;
+  CBS_ASN1_TAG tag;
   CBS_init(&data, kData1, sizeof(kData1));
   ASSERT_TRUE(CBS_get_any_asn1(&data, &contents, &tag));
   EXPECT_EQ(CBS_ASN1_SEQUENCE, tag);
@@ -267,7 +267,7 @@ TEST(CBSTest, GetASN1) {
 TEST(CBSTest, ParseASN1Tag) {
   const struct {
     bool ok;
-    unsigned tag;
+    CBS_ASN1_TAG tag;
     std::vector<uint8_t> in;
   } kTests[] = {
       {true, CBS_ASN1_SEQUENCE, {0x30, 0}},
@@ -278,9 +278,9 @@ TEST(CBSTest, ParseASN1Tag) {
       {true,
        CBS_ASN1_PRIVATE | CBS_ASN1_CONSTRUCTED | 0x1fffffff,
        {0xff, 0x81, 0xff, 0xff, 0xff, 0x7f, 0}},
-      // Tag number fits in unsigned but not |CBS_ASN1_TAG_NUMBER_MASK|.
+      // Tag number fits in |uint32_t| but not |CBS_ASN1_TAG_NUMBER_MASK|.
       {false, 0, {0xff, 0x82, 0xff, 0xff, 0xff, 0x7f, 0}},
-      // Tag number does not fit in unsigned.
+      // Tag number does not fit in |uint32_t|.
       {false, 0, {0xff, 0x90, 0x80, 0x80, 0x80, 0, 0}},
       // Tag number is not minimally-encoded
       {false, 0, {0x5f, 0x80, 0x1f, 0}},
@@ -289,7 +289,7 @@ TEST(CBSTest, ParseASN1Tag) {
   };
   for (const auto &t : kTests) {
     SCOPED_TRACE(Bytes(t.in));
-    unsigned tag;
+    CBS_ASN1_TAG tag;
     CBS cbs, child;
     CBS_init(&cbs, t.in.data(), t.in.size());
     ASSERT_EQ(t.ok, !!CBS_get_any_asn1(&cbs, &child, &tag));
@@ -702,7 +702,7 @@ struct BERTest {
   bool ok;
   bool ber_found;
   bool indefinite;
-  unsigned tag;
+  CBS_ASN1_TAG tag;
 };
 
 static const BERTest kBERTests[] = {
@@ -748,7 +748,7 @@ TEST(CBSTest, BERElementTest) {
     ASSERT_TRUE(DecodeHex(&in_bytes, test.in_hex));
     CBS in(in_bytes);
     CBS out;
-    unsigned tag;
+    CBS_ASN1_TAG tag;
     size_t header_len;
     int ber_found;
     int indefinite;

diff --git a/crypto/bytestring/cbb.c b/crypto/bytestring/cbb.c
@@ -333,14 +333,14 @@ static int add_base128_integer(CBB *cbb, uint64_t v) {
   return 1;
 }
 
-int CBB_add_asn1(CBB *cbb, CBB *out_contents, unsigned tag) {
+int CBB_add_asn1(CBB *cbb, CBB *out_contents, CBS_ASN1_TAG tag) {
   if (!CBB_flush(cbb)) {
     return 0;
   }
 
   // Split the tag into leading bits and tag number.
   uint8_t tag_bits = (tag >> CBS_ASN1_TAG_SHIFT) & 0xe0;
-  unsigned tag_number = tag & CBS_ASN1_TAG_NUMBER_MASK;
+  CBS_ASN1_TAG tag_number = tag & CBS_ASN1_TAG_NUMBER_MASK;
   if (tag_number >= 0x1f) {
     // Set all the bits in the tag number to signal high tag number form.
     if (!CBB_add_u8(cbb, tag_bits | 0x1f) ||
@@ -470,7 +470,7 @@ int CBB_add_asn1_uint64(CBB *cbb, uint64_t value) {
   return CBB_add_asn1_uint64_with_tag(cbb, value, CBS_ASN1_INTEGER);
 }
 
-int CBB_add_asn1_uint64_with_tag(CBB *cbb, uint64_t value, unsigned tag) {
+int CBB_add_asn1_uint64_with_tag(CBB *cbb, uint64_t value, CBS_ASN1_TAG tag) {
   CBB child;
   if (!CBB_add_asn1(cbb, &child, tag)) {
     return 0;
@@ -508,7 +508,7 @@ int CBB_add_asn1_int64(CBB *cbb, int64_t value) {
   return CBB_add_asn1_int64_with_tag(cbb, value, CBS_ASN1_INTEGER);
 }
 
-int CBB_add_asn1_int64_with_tag(CBB *cbb, int64_t value, unsigned tag) {
+int CBB_add_asn1_int64_with_tag(CBB *cbb, int64_t value, CBS_ASN1_TAG tag) {
   if (value >= 0) {
     return CBB_add_asn1_uint64_with_tag(cbb, (uint64_t)value, tag);
   }

diff --git a/crypto/bytestring/cbs.c b/crypto/bytestring/cbs.c
@@ -259,7 +259,7 @@ static int parse_base128_integer(CBS *cbs, uint64_t *out) {
   return 1;
 }
 
-static int parse_asn1_tag(CBS *cbs, unsigned *out) {
+static int parse_asn1_tag(CBS *cbs, CBS_ASN1_TAG *out) {
   uint8_t tag_byte;
   if (!CBS_get_u8(cbs, &tag_byte)) {
     return 0;
@@ -271,8 +271,8 @@ static int parse_asn1_tag(CBS *cbs, unsigned *out) {
   // If the number portion is 31 (0x1f, the largest value that fits in the
   // allotted bits), then the tag is more than one byte long and the
   // continuation bytes contain the tag number.
-  unsigned tag = ((unsigned)tag_byte & 0xe0) << CBS_ASN1_TAG_SHIFT;
-  unsigned tag_number = tag_byte & 0x1f;
+  CBS_ASN1_TAG tag = ((CBS_ASN1_TAG)tag_byte & 0xe0) << CBS_ASN1_TAG_SHIFT;
+  CBS_ASN1_TAG tag_number = tag_byte & 0x1f;
   if (tag_number == 0x1f) {
     uint64_t v;
     if (!parse_base128_integer(cbs, &v) ||
@@ -282,7 +282,7 @@ static int parse_asn1_tag(CBS *cbs, unsigned *out) {
         v < 0x1f) {
       return 0;
     }
-    tag_number = (unsigned)v;
+    tag_number = (CBS_ASN1_TAG)v;
   }
 
   tag |= tag_number;
@@ -298,7 +298,7 @@ static int parse_asn1_tag(CBS *cbs, unsigned *out) {
   return 1;
 }
 
-static int cbs_get_any_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag,
+static int cbs_get_any_asn1_element(CBS *cbs, CBS *out, CBS_ASN1_TAG *out_tag,
                                     size_t *out_header_len, int *out_ber_found,
                                     int *out_indefinite, int ber_ok) {
   CBS header = *cbs;
@@ -315,7 +315,7 @@ static int cbs_get_any_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag,
     assert(out_indefinite == NULL);
   }
 
-  unsigned tag;
+  CBS_ASN1_TAG tag;
   if (!parse_asn1_tag(&header, &tag)) {
     return 0;
   }
@@ -399,7 +399,7 @@ static int cbs_get_any_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag,
   return CBS_get_bytes(cbs, out, len);
 }
 
-int CBS_get_any_asn1(CBS *cbs, CBS *out, unsigned *out_tag) {
+int CBS_get_any_asn1(CBS *cbs, CBS *out, CBS_ASN1_TAG *out_tag) {
   size_t header_len;
   if (!CBS_get_any_asn1_element(cbs, out, out_tag, &header_len)) {
     return 0;
@@ -413,13 +413,13 @@ int CBS_get_any_asn1(CBS *cbs, CBS *out, unsigned *out_tag) {
   return 1;
 }
 
-int CBS_get_any_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag,
+int CBS_get_any_asn1_element(CBS *cbs, CBS *out, CBS_ASN1_TAG *out_tag,
                                     size_t *out_header_len) {
   return cbs_get_any_asn1_element(cbs, out, out_tag, out_header_len, NULL, NULL,
                                   /*ber_ok=*/0);
 }
 
-int CBS_get_any_ber_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag,
+int CBS_get_any_ber_asn1_element(CBS *cbs, CBS *out, CBS_ASN1_TAG *out_tag,
                                  size_t *out_header_len, int *out_ber_found,
                                  int *out_indefinite) {
   int ber_found_temp;
@@ -429,10 +429,10 @@ int CBS_get_any_ber_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag,
       /*ber_ok=*/1);
 }
 
-static int cbs_get_asn1(CBS *cbs, CBS *out, unsigned tag_value,
+static int cbs_get_asn1(CBS *cbs, CBS *out, CBS_ASN1_TAG tag_value,
                         int skip_header) {
   size_t header_len;
-  unsigned tag;
+  CBS_ASN1_TAG tag;
   CBS throwaway;
 
   if (out == NULL) {
@@ -452,21 +452,21 @@ static int cbs_get_asn1(CBS *cbs, CBS *out, unsigned tag_value,
   return 1;
 }
 
-int CBS_get_asn1(CBS *cbs, CBS *out, unsigned tag_value) {
+int CBS_get_asn1(CBS *cbs, CBS *out, CBS_ASN1_TAG tag_value) {
   return cbs_get_asn1(cbs, out, tag_value, 1 /* skip header */);
 }
 
-int CBS_get_asn1_element(CBS *cbs, CBS *out, unsigned tag_value) {
+int CBS_get_asn1_element(CBS *cbs, CBS *out, CBS_ASN1_TAG tag_value) {
   return cbs_get_asn1(cbs, out, tag_value, 0 /* include header */);
 }
 
-int CBS_peek_asn1_tag(const CBS *cbs, unsigned tag_value) {
+int CBS_peek_asn1_tag(const CBS *cbs, CBS_ASN1_TAG tag_value) {
   if (CBS_len(cbs) < 1) {
     return 0;
   }
 
   CBS copy = *cbs;
-  unsigned actual_tag;
+  CBS_ASN1_TAG actual_tag;
   return parse_asn1_tag(&copy, &actual_tag) && tag_value == actual_tag;
 }
 
@@ -529,7 +529,7 @@ int CBS_get_asn1_bool(CBS *cbs, int *out) {
   return 1;
 }
 
-int CBS_get_optional_asn1(CBS *cbs, CBS *out, int *out_present, unsigned tag) {
+int CBS_get_optional_asn1(CBS *cbs, CBS *out, int *out_present, CBS_ASN1_TAG tag) {
   int present = 0;
 
   if (CBS_peek_asn1_tag(cbs, tag)) {
@@ -547,7 +547,7 @@ int CBS_get_optional_asn1(CBS *cbs, CBS *out, int *out_present, unsigned tag) {
 }
 
 int CBS_get_optional_asn1_octet_string(CBS *cbs, CBS *out, int *out_present,
-                                       unsigned tag) {
+                                       CBS_ASN1_TAG tag) {
   CBS child;
   int present;
   if (!CBS_get_optional_asn1(cbs, &child, &present, tag)) {
@@ -568,7 +568,7 @@ int CBS_get_optional_asn1_octet_string(CBS *cbs, CBS *out, int *out_present,
   return 1;
 }
 
-int CBS_get_optional_asn1_uint64(CBS *cbs, uint64_t *out, unsigned tag,
+int CBS_get_optional_asn1_uint64(CBS *cbs, uint64_t *out, CBS_ASN1_TAG tag,
                                  uint64_t default_value) {
   CBS child;
   int present;
@@ -586,7 +586,7 @@ int CBS_get_optional_asn1_uint64(CBS *cbs, uint64_t *out, unsigned tag,
   return 1;
 }
 
-int CBS_get_optional_asn1_bool(CBS *cbs, int *out, unsigned tag,
+int CBS_get_optional_asn1_bool(CBS *cbs, int *out, CBS_ASN1_TAG tag,
                                int default_value) {
   CBS child, child2;
   int present;

diff --git a/crypto/bytestring/internal.h b/crypto/bytestring/internal.h
@@ -54,8 +54,8 @@ OPENSSL_EXPORT int CBS_asn1_ber_to_der(CBS *in, CBS *out,
 // It returns one on success and zero otherwise.
 OPENSSL_EXPORT int CBS_get_asn1_implicit_string(CBS *in, CBS *out,
                                                 uint8_t **out_storage,
-                                                unsigned outer_tag,
-                                                unsigned inner_tag);
+                                                CBS_ASN1_TAG outer_tag,
+                                                CBS_ASN1_TAG inner_tag);
 
 // CBB_finish_i2d calls |CBB_finish| on |cbb| which must have been initialized
 // with |CBB_init|. If |outp| is not NULL then the result is written to |*outp|

diff --git a/crypto/ec_extra/ec_asn1.c b/crypto/ec_extra/ec_asn1.c
@@ -67,9 +67,9 @@
 #include "../internal.h"
 
 
-static const unsigned kParametersTag =
+static const CBS_ASN1_TAG kParametersTag =
     CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0;
-static const unsigned kPublicKeyTag =
+static const CBS_ASN1_TAG kPublicKeyTag =
     CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1;
 
 EC_KEY *EC_KEY_parse_private_key(CBS *cbs, const EC_GROUP *group) {

diff --git a/fuzz/der_roundtrip.cc b/fuzz/der_roundtrip.cc
@@ -22,7 +22,7 @@
 
 extern "C" int LLVMFuzzerTestOneInput(const uint8_t *buf, size_t len) {
   CBS cbs, body;
-  unsigned tag;
+  CBS_ASN1_TAG tag;
   CBS_init(&cbs, buf, len);
   if (CBS_get_any_asn1(&cbs, &body, &tag)) {
     // DER has a unique encoding, so any parsed input should round-trip

diff --git a/include/openssl/base.h b/include/openssl/base.h
@@ -337,6 +337,10 @@ extern "C" {
 // are sizes of or indices into C objects, can be converted without overflow.
 typedef ptrdiff_t ossl_ssize_t;
 
+// CBS_ASN1_TAG is the type used by |CBS| and |CBB| for ASN.1 tags. See that
+// header for details. This type is defined in base.h as a forward declaration.
+typedef uint32_t CBS_ASN1_TAG;
+
 // CRYPTO_THREADID is a dummy value.
 typedef int CRYPTO_THREADID;