From 095b6c9baa93fecf5e0d8591f3586a666b60e00a Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 29 Nov 2017 17:17:41 -0500 Subject: [PATCH] Also add a decoupled OBJ_obj2txt. We need it in both directions. Also I missed that in OBJ_obj2txt we allowed uint64_t components, but in my new OBJ_txt2obj we only allowed uint32_t. For consistency, upgrade that to uint64_t. Bug: chromium:706445 Change-Id: I38cfeea8ff64b9acf7998e552727c6c3b2cc600f Reviewed-on: https://boringssl-review.googlesource.com/23544 Commit-Queue: Steven Valdez Reviewed-by: Steven Valdez CQ-Verified: CQ bot account: commit-bot@chromium.org --- crypto/bytestring/bytestring_test.cc | 125 ++++++++++++++++++--------- crypto/bytestring/cbb.c | 16 ++-- crypto/bytestring/cbs.c | 108 +++++++++++++++++++---- crypto/obj/obj.c | 79 ++--------------- include/openssl/bytestring.h | 9 +- 5 files changed, 195 insertions(+), 142 deletions(-) diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc index 6ce703581..1969e73e9 100644 --- a/crypto/bytestring/bytestring_test.cc +++ b/crypto/bytestring/bytestring_test.cc @@ -889,54 +889,95 @@ TEST(CBSTest, BitString) { TEST(CBBTest, AddOIDFromText) { const struct { - const char *in; - bool ok; - std::vector out; - } kTests[] = { + const char *text; + std::vector der; + } kValidOIDs[] = { // Some valid values. - {"1.2.3.4", true, {0x2a, 0x3, 0x4}}, + {"0.0", {0x00}}, + {"0.2.3.4", {0x2, 0x3, 0x4}}, + {"1.2.3.4", {0x2a, 0x3, 0x4}}, + {"2.2.3.4", {0x52, 0x3, 0x4}}, {"1.2.840.113554.4.1.72585", - true, {0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04, 0x01, 0x84, 0xb7, 0x09}}, // Test edge cases around the first component. - {"0.39", true, {0x27}}, - {"0.40", false, {}}, - {"1.0", true, {0x28}}, - {"1.39", true, {0x4f}}, - {"1.40", false, {}}, - {"2.0", true, {0x50}}, - {"2.1", true, {0x51}}, - {"2.40", true, {0x78}}, - // The empty string is not an OID. - {"", false, {}}, - // No empty components. - {".1.2.3.4.5", false, {}}, - {"1..2.3.4.5", false, {}}, - {"1.2.3.4.5.", false, {}}, - // There must be at least two components. - {"1", false, {}}, - // No extra leading zeros. - {"00.1.2.3.4", false, {}}, - {"01.1.2.3.4", false, {}}, - // Check for overflow. - {"1.2.4294967295", true, {0x2a, 0x8f, 0xff, 0xff, 0xff, 0x7f}}, - {"1.2.4294967296", false, {}}, - // 40*A + B overflows. - {"2.4294967215", true, {0x8f, 0xff, 0xff, 0xff, 0x7f}}, - {"2.4294967216", false, {}}, + {"0.39", {0x27}}, + {"1.0", {0x28}}, + {"1.39", {0x4f}}, + {"2.0", {0x50}}, + {"2.1", {0x51}}, + {"2.40", {0x78}}, + // Edge cases near an overflow. + {"1.2.18446744073709551615", + {0x2a, 0x81, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f}}, + {"2.18446744073709551535", + {0x81, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f}}, }; - for (const auto &t : kTests) { - SCOPED_TRACE(t.in); + + const char *kInvalidTexts[] = { + // Invalid second component. + "0.40", + "1.40", + // Invalid first component. + "3.1", + // The empty string is not an OID. + "", + // No empty components. + ".1.2.3.4.5", + "1..2.3.4.5", + "1.2.3.4.5.", + // There must be at least two components. + "1", + // No extra leading zeros. + "00.1.2.3.4", + "01.1.2.3.4", + // Overflow for both components or 40*A + B. + "1.2.18446744073709551616", + "2.18446744073709551536", + }; + + const std::vector kInvalidDER[] = { + // The empty string is not an OID. + {}, + // Non-minimal representation. + {0x80, 0x01}, + // Overflow. This is the DER representation of + // 1.2.840.113554.4.1.72585.18446744073709551616. (The final value is + // 2^64.) + {0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04, 0x01, 0x84, 0xb7, 0x09, + 0x82, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x00}, + }; + + for (const auto &t : kValidOIDs) { + SCOPED_TRACE(t.text); + bssl::ScopedCBB cbb; ASSERT_TRUE(CBB_init(cbb.get(), 0)); - int ok = CBB_add_asn1_oid_from_text(cbb.get(), t.in, strlen(t.in)); - EXPECT_EQ(t.ok, static_cast(ok)); - if (ok) { - uint8_t *out; - size_t len; - ASSERT_TRUE(CBB_finish(cbb.get(), &out, &len)); - bssl::UniquePtr free_out(out); - EXPECT_EQ(Bytes(t.out), Bytes(out, len)); - } + ASSERT_TRUE(CBB_add_asn1_oid_from_text(cbb.get(), t.text, strlen(t.text))); + uint8_t *out; + size_t len; + ASSERT_TRUE(CBB_finish(cbb.get(), &out, &len)); + bssl::UniquePtr free_out(out); + EXPECT_EQ(Bytes(t.der), Bytes(out, len)); + + CBS cbs; + CBS_init(&cbs, t.der.data(), t.der.size()); + bssl::UniquePtr text(CBS_asn1_oid_to_text(&cbs)); + ASSERT_TRUE(text.get()); + EXPECT_STREQ(t.text, text.get()); + } + + for (const char *t : kInvalidTexts) { + SCOPED_TRACE(t); + bssl::ScopedCBB cbb; + ASSERT_TRUE(CBB_init(cbb.get(), 0)); + EXPECT_FALSE(CBB_add_asn1_oid_from_text(cbb.get(), t, strlen(t))); + } + + for (const auto &t : kInvalidDER) { + SCOPED_TRACE(Bytes(t)); + CBS cbs; + CBS_init(&cbs, t.data(), t.size()); + bssl::UniquePtr text(CBS_asn1_oid_to_text(&cbs)); + EXPECT_FALSE(text); } } diff --git a/crypto/bytestring/cbb.c b/crypto/bytestring/cbb.c index 5a1c45b38..b12a66c24 100644 --- a/crypto/bytestring/cbb.c +++ b/crypto/bytestring/cbb.c @@ -332,9 +332,9 @@ int CBB_add_u24_length_prefixed(CBB *cbb, CBB *out_contents) { // add_base128_integer encodes |v| as a big-endian base-128 integer where the // high bit of each byte indicates where there is more data. This is the // encoding used in DER for both high tag number form and OID components. -static int add_base128_integer(CBB *cbb, uint32_t v) { +static int add_base128_integer(CBB *cbb, uint64_t v) { unsigned len_len = 0; - unsigned copy = v; + uint64_t copy = v; while (copy > 0) { len_len++; copy >>= 7; @@ -508,7 +508,7 @@ int CBB_add_asn1_uint64(CBB *cbb, uint64_t value) { // an OID literal, e.g., "1.2.840.113554.4.1.72585". It consumes both the // component and the dot, so |cbs| may be passed into the function again for the // next value. -static int parse_dotted_decimal(CBS *cbs, uint32_t *out) { +static int parse_dotted_decimal(CBS *cbs, uint64_t *out) { *out = 0; int seen_digit = 0; for (;;) { @@ -524,8 +524,8 @@ static int parse_dotted_decimal(CBS *cbs, uint32_t *out) { // Forbid stray leading zeros. (seen_digit && *out == 0) || // Check for overflow. - *out > UINT32_MAX / 10 || - *out * 10 > UINT32_MAX - (u - '0')) { + *out > UINT64_MAX / 10 || + *out * 10 > UINT64_MAX - (u - '0')) { return 0; } *out = *out * 10 + (u - '0'); @@ -544,7 +544,7 @@ int CBB_add_asn1_oid_from_text(CBB *cbb, const char *text, size_t len) { CBS_init(&cbs, (const uint8_t *)text, len); // OIDs must have at least two components. - uint32_t a, b; + uint64_t a, b; if (!parse_dotted_decimal(&cbs, &a) || !parse_dotted_decimal(&cbs, &b)) { return 0; @@ -554,8 +554,8 @@ int CBB_add_asn1_oid_from_text(CBB *cbb, const char *text, size_t len) { // 0, 1, or 2 and that, when it is 0 or 1, |b| is at most 39. if (a > 2 || (a < 2 && b > 39) || - b > UINT32_MAX - 80 || - !add_base128_integer(cbb, 40 * a + b)) { + b > UINT64_MAX - 80 || + !add_base128_integer(cbb, 40u * a + b)) { return 0; } diff --git a/crypto/bytestring/cbs.c b/crypto/bytestring/cbs.c index d96371ce5..e456330f8 100644 --- a/crypto/bytestring/cbs.c +++ b/crypto/bytestring/cbs.c @@ -12,11 +12,16 @@ * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ +#if !defined(__STDC_FORMAT_MACROS) +#define __STDC_FORMAT_MACROS +#endif + #include #include #include #include +#include #include #include "internal.h" @@ -175,6 +180,33 @@ int CBS_get_u24_length_prefixed(CBS *cbs, CBS *out) { return cbs_get_length_prefixed(cbs, out, 3); } +// parse_base128_integer reads a big-endian base-128 integer from |cbs| and sets +// |*out| to the result. This is the encoding used in DER for both high tag +// number form and OID components. +static int parse_base128_integer(CBS *cbs, uint64_t *out) { + uint64_t v = 0; + uint8_t b; + do { + if (!CBS_get_u8(cbs, &b)) { + return 0; + } + if ((v >> (64 - 7)) != 0) { + // The value is too large. + return 0; + } + if (v == 0 && b == 0x80) { + // The value must be minimally encoded. + return 0; + } + v = (v << 7) | (b & 0x7f); + + // Values end at an octet with the high bit cleared. + } while (b & 0x80); + + *out = v; + return 1; +} + static int parse_asn1_tag(CBS *cbs, unsigned *out) { uint8_t tag_byte; if (!CBS_get_u8(cbs, &tag_byte)) { @@ -191,27 +223,15 @@ static int parse_asn1_tag(CBS *cbs, unsigned *out) { unsigned tag = ((unsigned)tag_byte & 0xe0) << CBS_ASN1_TAG_SHIFT; unsigned tag_number = tag_byte & 0x1f; if (tag_number == 0x1f) { - tag_number = 0; - for (;;) { - if (!CBS_get_u8(cbs, &tag_byte) || - ((tag_number << 7) >> 7) != tag_number) { - return 0; - } - tag_number = (tag_number << 7) | (tag_byte & 0x7f); - // The tag must be represented in the minimal number of bytes. - if (tag_number == 0) { - return 0; - } - if ((tag_byte & 0x80) == 0) { - break; - } - } - if (// Check the tag number is within our supported bounds. - tag_number > CBS_ASN1_TAG_NUMBER_MASK || + uint64_t v; + if (!parse_base128_integer(cbs, &v) || + // Check the tag number is within our supported bounds. + v > CBS_ASN1_TAG_NUMBER_MASK || // Small tag numbers should have used low tag number form. - tag_number < 0x1f) { + v < 0x1f) { return 0; } + tag_number = (unsigned)v; } tag |= tag_number; @@ -527,3 +547,55 @@ int CBS_asn1_bitstring_has_bit(const CBS *cbs, unsigned bit) { return byte_num < CBS_len(cbs) && (CBS_data(cbs)[byte_num] & (1 << bit_num)) != 0; } + +static int add_decimal(CBB *out, uint64_t v) { + char buf[DECIMAL_SIZE(uint64_t) + 1]; + BIO_snprintf(buf, sizeof(buf), "%" PRIu64, v); + return CBB_add_bytes(out, (const uint8_t *)buf, strlen(buf)); +} + +char *CBS_asn1_oid_to_text(const CBS *cbs) { + CBB cbb; + if (!CBB_init(&cbb, 32)) { + goto err; + } + + CBS copy = *cbs; + // The first component is 40 * value1 + value2, where value1 is 0, 1, or 2. + uint64_t v; + if (!parse_base128_integer(©, &v)) { + goto err; + } + + if (v >= 80) { + if (!CBB_add_bytes(&cbb, (const uint8_t *)"2.", 2) || + !add_decimal(&cbb, v - 80)) { + goto err; + } + } else if (!add_decimal(&cbb, v / 40) || + !CBB_add_u8(&cbb, '.') || + !add_decimal(&cbb, v % 40)) { + goto err; + } + + while (CBS_len(©) != 0) { + if (!parse_base128_integer(©, &v) || + !CBB_add_u8(&cbb, '.') || + !add_decimal(&cbb, v)) { + goto err; + } + } + + uint8_t *txt; + size_t txt_len; + if (!CBB_add_u8(&cbb, '\0') || + !CBB_finish(&cbb, &txt, &txt_len)) { + goto err; + } + + return (char *)txt; + +err: + CBB_cleanup(&cbb); + return NULL; +} diff --git a/crypto/obj/obj.c b/crypto/obj/obj.c index 8e1e6b14c..a34d6dc03 100644 --- a/crypto/obj/obj.c +++ b/crypto/obj/obj.c @@ -434,36 +434,6 @@ static int strlcpy_int(char *dst, const char *src, int dst_size) { return (int)ret; } -static int parse_oid_component(CBS *cbs, uint64_t *out) { - uint64_t v = 0; - uint8_t b; - do { - if (!CBS_get_u8(cbs, &b)) { - return 0; - } - if ((v >> (64 - 7)) != 0) { - // The component is too large. - return 0; - } - if (v == 0 && b == 0x80) { - // The component must be minimally encoded. - return 0; - } - v = (v << 7) | (b & 0x7f); - - // Components end at an octet with the high bit cleared. - } while (b & 0x80); - - *out = v; - return 1; -} - -static int add_decimal(CBB *out, uint64_t v) { - char buf[DECIMAL_SIZE(uint64_t) + 1]; - BIO_snprintf(buf, sizeof(buf), "%" PRIu64, v); - return CBB_add_bytes(out, (const uint8_t *)buf, strlen(buf)); -} - int OBJ_obj2txt(char *out, int out_len, const ASN1_OBJECT *obj, int always_return_oid) { // Python depends on the empty OID successfully encoding as the empty @@ -485,56 +455,19 @@ int OBJ_obj2txt(char *out, int out_len, const ASN1_OBJECT *obj, } } - CBB cbb; - if (!CBB_init(&cbb, 32)) { - goto err; - } - CBS cbs; CBS_init(&cbs, obj->data, obj->length); - - // The first component is 40 * value1 + value2, where value1 is 0, 1, or 2. - uint64_t v; - if (!parse_oid_component(&cbs, &v)) { - goto err; - } - - if (v >= 80) { - if (!CBB_add_bytes(&cbb, (const uint8_t *)"2.", 2) || - !add_decimal(&cbb, v - 80)) { - goto err; + char *txt = CBS_asn1_oid_to_text(&cbs); + if (txt == NULL) { + if (out_len > 0) { + out[0] = '\0'; } - } else if (!add_decimal(&cbb, v / 40) || - !CBB_add_u8(&cbb, '.') || - !add_decimal(&cbb, v % 40)) { - goto err; + return -1; } - while (CBS_len(&cbs) != 0) { - if (!parse_oid_component(&cbs, &v) || - !CBB_add_u8(&cbb, '.') || - !add_decimal(&cbb, v)) { - goto err; - } - } - - uint8_t *txt; - size_t txt_len; - if (!CBB_add_u8(&cbb, '\0') || - !CBB_finish(&cbb, &txt, &txt_len)) { - goto err; - } - - int ret = strlcpy_int(out, (const char *)txt, out_len); + int ret = strlcpy_int(out, txt, out_len); OPENSSL_free(txt); return ret; - -err: - CBB_cleanup(&cbb); - if (out_len > 0) { - out[0] = '\0'; - } - return -1; } static uint32_t hash_nid(const ASN1_OBJECT *obj) { diff --git a/include/openssl/bytestring.h b/include/openssl/bytestring.h index abf888571..43349e9ac 100644 --- a/include/openssl/bytestring.h +++ b/include/openssl/bytestring.h @@ -279,6 +279,13 @@ OPENSSL_EXPORT int CBS_is_valid_asn1_bitstring(const CBS *cbs); // is indexed starting from zero. OPENSSL_EXPORT int CBS_asn1_bitstring_has_bit(const CBS *cbs, unsigned bit); +// 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 +// string, or NULL on failure. The caller must release the result with +// |OPENSSL_free|. +OPENSSL_EXPORT char *CBS_asn1_oid_to_text(const CBS *cbs); + // CRYPTO ByteBuilder. // @@ -450,7 +457,7 @@ OPENSSL_EXPORT int CBB_add_asn1_uint64(CBB *cbb, uint64_t value); // the element's contents. // // This function considers OID strings with components which do not fit in a -// |uint32_t| to be invalid. +// |uint64_t| to be invalid. OPENSSL_EXPORT int CBB_add_asn1_oid_from_text(CBB *cbb, const char *text, size_t len);