Add APIs for checking ASN.1 INTEGERs.
We have several implementations of this internally, so consolidate them.
Chromium also has a copy in net/der/parse_values.cc which could call
into this.
(I'm also hoping we can make c2i_ASN1_INTEGER call this and
further tighten up crypto/asn1's parser, but I see Chromium still has an
allow_invalid_serial_numbers option, so perhaps not quite yet.)
Update-Note: This CL does not change behavior, but I'm leaving a note to
myself to make net/der/parse_values.cc call the new functions.
Change-Id: If2aae6574ba6a30e343e1308da6af543616156ec
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44051
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/bn_extra/bn_asn1.c b/crypto/bn_extra/bn_asn1.c
index 0d96573..a8333d4 100644
--- a/crypto/bn_extra/bn_asn1.c
+++ b/crypto/bn_extra/bn_asn1.c
@@ -20,25 +20,18 @@
int BN_parse_asn1_unsigned(CBS *cbs, BIGNUM *ret) {
CBS child;
+ int is_negative;
if (!CBS_get_asn1(cbs, &child, CBS_ASN1_INTEGER) ||
- CBS_len(&child) == 0) {
+ !CBS_is_valid_asn1_integer(&child, &is_negative)) {
OPENSSL_PUT_ERROR(BN, BN_R_BAD_ENCODING);
return 0;
}
- if (CBS_data(&child)[0] & 0x80) {
+ if (is_negative) {
OPENSSL_PUT_ERROR(BN, BN_R_NEGATIVE_NUMBER);
return 0;
}
- // INTEGERs must be minimal.
- if (CBS_data(&child)[0] == 0x00 &&
- CBS_len(&child) > 1 &&
- !(CBS_data(&child)[1] & 0x80)) {
- OPENSSL_PUT_ERROR(BN, BN_R_BAD_ENCODING);
- return 0;
- }
-
return BN_bin2bn(CBS_data(&child), CBS_len(&child), ret) != NULL;
}
diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc
index 93593e9..6445842 100644
--- a/crypto/bytestring/bytestring_test.cc
+++ b/crypto/bytestring/bytestring_test.cc
@@ -720,50 +720,65 @@
struct ASN1InvalidUint64Test {
const char *encoding;
size_t encoding_len;
+ bool overflow;
};
static const ASN1InvalidUint64Test kASN1InvalidUint64Tests[] = {
// Bad tag.
- {"\x03\x01\x00", 3},
+ {"\x03\x01\x00", 3, false},
// Empty contents.
- {"\x02\x00", 2},
+ {"\x02\x00", 2, false},
// Negative number.
- {"\x02\x01\x80", 3},
+ {"\x02\x01\x80", 3, false},
// Overflow.
- {"\x02\x09\x01\x00\x00\x00\x00\x00\x00\x00\x00", 11},
+ {"\x02\x09\x01\x00\x00\x00\x00\x00\x00\x00\x00", 11, true},
// Leading zeros.
- {"\x02\x02\x00\x01", 4},
+ {"\x02\x02\x00\x01", 4, false},
};
TEST(CBSTest, ASN1Uint64) {
- for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(kASN1Uint64Tests); i++) {
- SCOPED_TRACE(i);
- const ASN1Uint64Test *test = &kASN1Uint64Tests[i];
+ for (const ASN1Uint64Test &test : kASN1Uint64Tests) {
+ SCOPED_TRACE(Bytes(test.encoding, test.encoding_len));
+ SCOPED_TRACE(test.value);
CBS cbs;
uint64_t value;
uint8_t *out;
size_t len;
- CBS_init(&cbs, (const uint8_t *)test->encoding, test->encoding_len);
+ CBS_init(&cbs, (const uint8_t *)test.encoding, test.encoding_len);
ASSERT_TRUE(CBS_get_asn1_uint64(&cbs, &value));
EXPECT_EQ(0u, CBS_len(&cbs));
- EXPECT_EQ(test->value, value);
+ EXPECT_EQ(test.value, value);
+
+ CBS child;
+ int is_negative;
+ CBS_init(&cbs, (const uint8_t *)test.encoding, test.encoding_len);
+ ASSERT_TRUE(CBS_get_asn1(&cbs, &child, CBS_ASN1_INTEGER));
+ EXPECT_TRUE(CBS_is_valid_asn1_integer(&child, &is_negative));
+ EXPECT_EQ(0, is_negative);
+ EXPECT_TRUE(CBS_is_unsigned_asn1_integer(&child));
bssl::ScopedCBB cbb;
ASSERT_TRUE(CBB_init(cbb.get(), 0));
- ASSERT_TRUE(CBB_add_asn1_uint64(cbb.get(), test->value));
+ ASSERT_TRUE(CBB_add_asn1_uint64(cbb.get(), test.value));
ASSERT_TRUE(CBB_finish(cbb.get(), &out, &len));
bssl::UniquePtr<uint8_t> scoper(out);
- EXPECT_EQ(Bytes(test->encoding, test->encoding_len), Bytes(out, len));
+ EXPECT_EQ(Bytes(test.encoding, test.encoding_len), Bytes(out, len));
}
- for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(kASN1InvalidUint64Tests); i++) {
- const ASN1InvalidUint64Test *test = &kASN1InvalidUint64Tests[i];
+ for (const ASN1InvalidUint64Test &test : kASN1InvalidUint64Tests) {
+ SCOPED_TRACE(Bytes(test.encoding, test.encoding_len));
CBS cbs;
uint64_t value;
- CBS_init(&cbs, (const uint8_t *)test->encoding, test->encoding_len);
+ CBS_init(&cbs, (const uint8_t *)test.encoding, test.encoding_len);
EXPECT_FALSE(CBS_get_asn1_uint64(&cbs, &value));
+
+ CBS_init(&cbs, (const uint8_t *)test.encoding, test.encoding_len);
+ CBS child;
+ if (CBS_get_asn1(&cbs, &child, CBS_ASN1_INTEGER)) {
+ EXPECT_EQ(test.overflow, !!CBS_is_unsigned_asn1_integer(&child));
+ }
}
}
@@ -793,50 +808,67 @@
struct ASN1InvalidInt64Test {
const char *encoding;
size_t encoding_len;
+ bool overflow;
};
static const ASN1InvalidInt64Test kASN1InvalidInt64Tests[] = {
// Bad tag.
- {"\x03\x01\x00", 3},
+ {"\x03\x01\x00", 3, false},
// Empty contents.
- {"\x02\x00", 2},
+ {"\x02\x00", 2, false},
// Overflow.
- {"\x02\x09\x01\x00\x00\x00\x00\x00\x00\x00\x00", 11},
+ {"\x02\x09\x01\x00\x00\x00\x00\x00\x00\x00\x00", 11, true},
+ // Underflow.
+ {"\x02\x09\x08\xff\xff\xff\xff\xff\xff\xff\xff", 11, true},
// Leading zeros.
- {"\x02\x02\x00\x01", 4},
+ {"\x02\x02\x00\x01", 4, false},
// Leading 0xff.
- {"\x02\x02\xff\xff", 4},
+ {"\x02\x02\xff\xff", 4, false},
};
TEST(CBSTest, ASN1Int64) {
- for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(kASN1Int64Tests); i++) {
- SCOPED_TRACE(i);
- const ASN1Int64Test *test = &kASN1Int64Tests[i];
+ for (const ASN1Int64Test &test : kASN1Int64Tests) {
+ SCOPED_TRACE(Bytes(test.encoding, test.encoding_len));
+ SCOPED_TRACE(test.value);
CBS cbs;
int64_t value;
uint8_t *out;
size_t len;
- CBS_init(&cbs, (const uint8_t *)test->encoding, test->encoding_len);
+ CBS_init(&cbs, (const uint8_t *)test.encoding, test.encoding_len);
ASSERT_TRUE(CBS_get_asn1_int64(&cbs, &value));
EXPECT_EQ(0u, CBS_len(&cbs));
- EXPECT_EQ(test->value, value);
+ EXPECT_EQ(test.value, value);
+
+ CBS child;
+ int is_negative;
+ CBS_init(&cbs, (const uint8_t *)test.encoding, test.encoding_len);
+ ASSERT_TRUE(CBS_get_asn1(&cbs, &child, CBS_ASN1_INTEGER));
+ EXPECT_TRUE(CBS_is_valid_asn1_integer(&child, &is_negative));
+ EXPECT_EQ(test.value < 0, !!is_negative);
+ EXPECT_EQ(test.value >= 0, !!CBS_is_unsigned_asn1_integer(&child));
bssl::ScopedCBB cbb;
ASSERT_TRUE(CBB_init(cbb.get(), 0));
- ASSERT_TRUE(CBB_add_asn1_int64(cbb.get(), test->value));
+ ASSERT_TRUE(CBB_add_asn1_int64(cbb.get(), test.value));
ASSERT_TRUE(CBB_finish(cbb.get(), &out, &len));
bssl::UniquePtr<uint8_t> scoper(out);
- EXPECT_EQ(Bytes(test->encoding, test->encoding_len), Bytes(out, len));
+ EXPECT_EQ(Bytes(test.encoding, test.encoding_len), Bytes(out, len));
}
- for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(kASN1InvalidInt64Tests); i++) {
- const ASN1InvalidInt64Test *test = &kASN1InvalidInt64Tests[i];
+ for (const ASN1InvalidInt64Test &test : kASN1InvalidInt64Tests) {
+ SCOPED_TRACE(Bytes(test.encoding, test.encoding_len));
CBS cbs;
int64_t value;
- CBS_init(&cbs, (const uint8_t *)test->encoding, test->encoding_len);
+ CBS_init(&cbs, (const uint8_t *)test.encoding, test.encoding_len);
EXPECT_FALSE(CBS_get_asn1_int64(&cbs, &value));
+
+ CBS_init(&cbs, (const uint8_t *)test.encoding, test.encoding_len);
+ CBS child;
+ if (CBS_get_asn1(&cbs, &child, CBS_ASN1_INTEGER)) {
+ EXPECT_EQ(test.overflow, !!CBS_is_valid_asn1_integer(&child, NULL));
+ }
}
}
diff --git a/crypto/bytestring/cbs.c b/crypto/bytestring/cbs.c
index 49d7003..d7b2af7 100644
--- a/crypto/bytestring/cbs.c
+++ b/crypto/bytestring/cbs.c
@@ -426,29 +426,14 @@
int CBS_get_asn1_uint64(CBS *cbs, uint64_t *out) {
CBS bytes;
- if (!CBS_get_asn1(cbs, &bytes, CBS_ASN1_INTEGER)) {
+ if (!CBS_get_asn1(cbs, &bytes, CBS_ASN1_INTEGER) ||
+ !CBS_is_unsigned_asn1_integer(&bytes)) {
return 0;
}
*out = 0;
const uint8_t *data = CBS_data(&bytes);
size_t len = CBS_len(&bytes);
-
- if (len == 0) {
- // An INTEGER is encoded with at least one octet.
- return 0;
- }
-
- if ((data[0] & 0x80) != 0) {
- // Negative number.
- return 0;
- }
-
- if (data[0] == 0 && len > 1 && (data[1] & 0x80) == 0) {
- // Extra leading zeros.
- return 0;
- }
-
for (size_t i = 0; i < len; i++) {
if ((*out >> 56) != 0) {
// Too large to represent as a uint64_t.
@@ -462,31 +447,21 @@
}
int CBS_get_asn1_int64(CBS *cbs, int64_t *out) {
+ int is_negative;
CBS bytes;
- if (!CBS_get_asn1(cbs, &bytes, CBS_ASN1_INTEGER)) {
+ if (!CBS_get_asn1(cbs, &bytes, CBS_ASN1_INTEGER) ||
+ !CBS_is_valid_asn1_integer(&bytes, &is_negative)) {
return 0;
}
const uint8_t *data = CBS_data(&bytes);
const size_t len = CBS_len(&bytes);
-
- if (len == 0 || len > sizeof(int64_t)) {
- // An INTEGER is encoded with at least one octet.
+ if (len > sizeof(int64_t)) {
return 0;
}
- if (len > 1) {
- if (data[0] == 0 && (data[1] & 0x80) == 0) {
- return 0; // Extra leading zeros.
- }
- if (data[0] == 0xff && (data[1] & 0x80) != 0) {
- return 0; // Extra leading 0xff.
- }
- }
-
union {
int64_t i;
uint8_t bytes[sizeof(int64_t)];
} u;
- const int is_negative = (data[0] & 0x80);
memset(u.bytes, is_negative ? 0xff : 0, sizeof(u.bytes)); // Sign-extend.
for (size_t i = 0; i < len; i++) {
u.bytes[i] = data[len - i - 1];
@@ -635,6 +610,30 @@
(CBS_data(cbs)[byte_num] & (1 << bit_num)) != 0;
}
+int CBS_is_valid_asn1_integer(const CBS *cbs, int *out_is_negative) {
+ CBS copy = *cbs;
+ uint8_t first_byte, second_byte;
+ if (!CBS_get_u8(©, &first_byte)) {
+ return 0; // INTEGERs may not be empty.
+ }
+ if (out_is_negative != NULL) {
+ *out_is_negative = (first_byte & 0x80) != 0;
+ }
+ if (!CBS_get_u8(©, &second_byte)) {
+ return 1; // One byte INTEGERs are always minimal.
+ }
+ if ((first_byte == 0x00 && (second_byte & 0x80) == 0) ||
+ (first_byte == 0xff && (second_byte & 0x80) != 0)) {
+ return 0; // The value is minimal iff the first 9 bits are not all equal.
+ }
+ return 1;
+}
+
+int CBS_is_unsigned_asn1_integer(const CBS *cbs) {
+ int is_negative;
+ return CBS_is_valid_asn1_integer(cbs, &is_negative) && !is_negative;
+}
+
static int add_decimal(CBB *out, uint64_t v) {
char buf[DECIMAL_SIZE(uint64_t) + 1];
BIO_snprintf(buf, sizeof(buf), "%" PRIu64, v);
diff --git a/crypto/ec_extra/ec_asn1.c b/crypto/ec_extra/ec_asn1.c
index 9769d01..56cbbed 100644
--- a/crypto/ec_extra/ec_asn1.c
+++ b/crypto/ec_extra/ec_asn1.c
@@ -241,21 +241,6 @@
return 1;
}
-// is_unsigned_integer returns one if |cbs| is a valid unsigned DER INTEGER and
-// zero otherwise.
-static int is_unsigned_integer(const CBS *cbs) {
- if (CBS_len(cbs) == 0) {
- return 0;
- }
- uint8_t byte = CBS_data(cbs)[0];
- if ((byte & 0x80) ||
- (byte == 0 && CBS_len(cbs) > 1 && (CBS_data(cbs)[1] & 0x80) == 0)) {
- // Negative or not minimally-encoded.
- return 0;
- }
- return 1;
-}
-
// kPrimeFieldOID is the encoding of 1.2.840.10045.1.1.
static const uint8_t kPrimeField[] = {0x2a, 0x86, 0x48, 0xce, 0x3d, 0x01, 0x01};
@@ -276,7 +261,7 @@
OPENSSL_memcmp(CBS_data(&field_type), kPrimeField, sizeof(kPrimeField)) !=
0 ||
!CBS_get_asn1(&field_id, out_prime, CBS_ASN1_INTEGER) ||
- !is_unsigned_integer(out_prime) ||
+ !CBS_is_unsigned_asn1_integer(out_prime) ||
CBS_len(&field_id) != 0 ||
!CBS_get_asn1(¶ms, &curve, CBS_ASN1_SEQUENCE) ||
!CBS_get_asn1(&curve, out_a, CBS_ASN1_OCTETSTRING) ||
@@ -286,7 +271,7 @@
CBS_len(&curve) != 0 ||
!CBS_get_asn1(¶ms, &base, CBS_ASN1_OCTETSTRING) ||
!CBS_get_asn1(¶ms, out_order, CBS_ASN1_INTEGER) ||
- !is_unsigned_integer(out_order) ||
+ !CBS_is_unsigned_asn1_integer(out_order) ||
!CBS_get_optional_asn1(¶ms, &cofactor, &has_cofactor,
CBS_ASN1_INTEGER) ||
CBS_len(¶ms) != 0) {
diff --git a/crypto/x509/x509_v3.c b/crypto/x509/x509_v3.c
index 7cfd6e9..91bf024 100644
--- a/crypto/x509/x509_v3.c
+++ b/crypto/x509/x509_v3.c
@@ -248,7 +248,7 @@
if (ex == NULL)
return (0);
- i = M_ASN1_OCTET_STRING_set(ex->value, data->data, data->length);
+ i = ASN1_OCTET_STRING_set(ex->value, data->data, data->length);
if (!i)
return (0);
return (1);
diff --git a/include/openssl/bytestring.h b/include/openssl/bytestring.h
index 1f9c879..a4c4724 100644
--- a/include/openssl/bytestring.h
+++ b/include/openssl/bytestring.h
@@ -310,14 +310,25 @@
int default_value);
// CBS_is_valid_asn1_bitstring returns one if |cbs| is a valid ASN.1 BIT STRING
-// and zero otherwise.
+// body and zero otherwise.
OPENSSL_EXPORT int CBS_is_valid_asn1_bitstring(const CBS *cbs);
// CBS_asn1_bitstring_has_bit returns one if |cbs| is a valid ASN.1 BIT STRING
-// and the specified bit is present and set. Otherwise, it returns zero. |bit|
-// is indexed starting from zero.
+// body and the specified bit is present and set. Otherwise, it returns zero.
+// |bit| is indexed starting from zero.
OPENSSL_EXPORT int CBS_asn1_bitstring_has_bit(const CBS *cbs, unsigned bit);
+// CBS_is_valid_asn1_integer returns one if |cbs| is a valid ASN.1 INTEGER,
+// body and zero otherwise. On success, if |out_is_negative| is non-NULL,
+// |*out_is_negative| will be set to one if |cbs| is negative and zero
+// otherwise.
+OPENSSL_EXPORT int CBS_is_valid_asn1_integer(const CBS *cbs,
+ int *out_is_negative);
+
+// CBS_is_unsigned_asn1_integer returns one if |cbs| is a valid non-negative
+// ASN.1 INTEGER body and zero otherwise.
+OPENSSL_EXPORT int CBS_is_unsigned_asn1_integer(const CBS *cbs);
+
// CBS_asn1_oid_to_text interprets |cbs| as DER-encoded ASN.1 OBJECT IDENTIFIER
// contents (not including the element framing) and returns the ASCII
// representation (e.g., "1.2.840.113554.4.1.72585") in a newly-allocated
