diff --git a/crypto/bn_extra/bn_asn1.c b/crypto/bn_extra/bn_asn1.c index 0d96573a3..a8333d419 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 93593e96d..644584233 100644 --- a/crypto/bytestring/bytestring_test.cc +++ b/crypto/bytestring/bytestring_test.cc @@ -720,50 +720,65 @@ static const ASN1Uint64Test kASN1Uint64Tests[] = { 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 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 @@ static const ASN1Int64Test kASN1Int64Tests[] = { 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 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 49d700366..d7b2af797 100644 --- a/crypto/bytestring/cbs.c +++ b/crypto/bytestring/cbs.c @@ -426,29 +426,14 @@ int CBS_peek_asn1_tag(const CBS *cbs, unsigned tag_value) { 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_uint64(CBS *cbs, uint64_t *out) { } 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 @@ int CBS_asn1_bitstring_has_bit(const CBS *cbs, unsigned bit) { (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 9769d014e..56cbbed16 100644 --- a/crypto/ec_extra/ec_asn1.c +++ b/crypto/ec_extra/ec_asn1.c @@ -241,21 +241,6 @@ int EC_KEY_marshal_private_key(CBB *cbb, const EC_KEY *key, 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 @@ static int parse_explicit_prime_curve(CBS *in, CBS *out_prime, CBS *out_a, 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 @@ static int parse_explicit_prime_curve(CBS *in, CBS *out_prime, CBS *out_a, 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 7cfd6e935..91bf024ca 100644 --- a/crypto/x509/x509_v3.c +++ b/crypto/x509/x509_v3.c @@ -248,7 +248,7 @@ int X509_EXTENSION_set_data(X509_EXTENSION *ex, const ASN1_OCTET_STRING *data) 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 1f9c87908..a4c4724d6 100644 --- a/include/openssl/bytestring.h +++ b/include/openssl/bytestring.h @@ -310,14 +310,25 @@ OPENSSL_EXPORT int CBS_get_optional_asn1_bool(CBS *cbs, int *out, unsigned tag, 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