From 1ee71185a2322dc354bee5e5a0abfb1810a27dc6 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 6 Oct 2022 15:07:20 -0400 Subject: [PATCH] Add CBB_add_asn1_[u]int64_with_tag. CBB_add_asn1_uint64 doesn't work if you're encoding an implicitly-tagged INTEGER. Take a leaf from Go cryptobyte and add a "with tag" variant, rather than a "contents" variant, which is a little more convenient to use. It also avoids us having to decide how to name the contents field. Change-Id: I6072e55017230c513577c44c5a7ed86e778255b3 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54685 Reviewed-by: Bob Beck Commit-Queue: Bob Beck Commit-Queue: David Benjamin Auto-Submit: David Benjamin --- crypto/bytestring/bytestring_test.cc | 56 ++++++++++++++++++++++------ crypto/bytestring/cbb.c | 17 ++++++--- include/openssl/bytestring.h | 12 ++++++ 3 files changed, 68 insertions(+), 17 deletions(-) diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc index b6b716e56..27aae79ee 100644 --- a/crypto/bytestring/bytestring_test.cc +++ b/crypto/bytestring/bytestring_test.cc @@ -862,12 +862,28 @@ TEST(CBSTest, ASN1Uint64) { 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_finish(cbb.get(), &out, &len)); - bssl::UniquePtr scoper(out); - EXPECT_EQ(Bytes(test.encoding, test.encoding_len), Bytes(out, len)); + { + bssl::ScopedCBB cbb; + ASSERT_TRUE(CBB_init(cbb.get(), 0)); + 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)); + } + + { + // Overwrite the tag. + bssl::ScopedCBB cbb; + ASSERT_TRUE(CBB_init(cbb.get(), 0)); + ASSERT_TRUE(CBB_add_asn1_uint64_with_tag(cbb.get(), test.value, + CBS_ASN1_CONTEXT_SPECIFIC | 1)); + ASSERT_TRUE(CBB_finish(cbb.get(), &out, &len)); + bssl::UniquePtr scoper(out); + std::vector expected(test.encoding, + test.encoding + test.encoding_len); + expected[0] = 0x81; + EXPECT_EQ(Bytes(expected), Bytes(out, len)); + } } for (const ASN1InvalidUint64Test &test : kASN1InvalidUint64Tests) { @@ -952,12 +968,28 @@ TEST(CBSTest, ASN1Int64) { 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_finish(cbb.get(), &out, &len)); - bssl::UniquePtr scoper(out); - EXPECT_EQ(Bytes(test.encoding, test.encoding_len), Bytes(out, len)); + { + bssl::ScopedCBB cbb; + ASSERT_TRUE(CBB_init(cbb.get(), 0)); + 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)); + } + + { + // Overwrite the tag. + bssl::ScopedCBB cbb; + ASSERT_TRUE(CBB_init(cbb.get(), 0)); + ASSERT_TRUE(CBB_add_asn1_int64_with_tag(cbb.get(), test.value, + CBS_ASN1_CONTEXT_SPECIFIC | 1)); + ASSERT_TRUE(CBB_finish(cbb.get(), &out, &len)); + bssl::UniquePtr scoper(out); + std::vector expected(test.encoding, + test.encoding + test.encoding_len); + expected[0] = 0x81; + EXPECT_EQ(Bytes(expected), Bytes(out, len)); + } } for (const ASN1InvalidInt64Test &test : kASN1InvalidInt64Tests) { diff --git a/crypto/bytestring/cbb.c b/crypto/bytestring/cbb.c index 6ce20ad7a..100c7139c 100644 --- a/crypto/bytestring/cbb.c +++ b/crypto/bytestring/cbb.c @@ -503,13 +503,16 @@ void CBB_discard_child(CBB *cbb) { } int CBB_add_asn1_uint64(CBB *cbb, uint64_t value) { - CBB child; - int started = 0; + return CBB_add_asn1_uint64_with_tag(cbb, value, CBS_ASN1_INTEGER); +} - if (!CBB_add_asn1(cbb, &child, CBS_ASN1_INTEGER)) { +int CBB_add_asn1_uint64_with_tag(CBB *cbb, uint64_t value, unsigned tag) { + CBB child; + if (!CBB_add_asn1(cbb, &child, tag)) { return 0; } + int started = 0; for (size_t i = 0; i < 8; i++) { uint8_t byte = (value >> 8*(7-i)) & 0xff; if (!started) { @@ -538,8 +541,12 @@ int CBB_add_asn1_uint64(CBB *cbb, uint64_t value) { } 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) { if (value >= 0) { - return CBB_add_asn1_uint64(cbb, value); + return CBB_add_asn1_uint64_with_tag(cbb, (uint64_t)value, tag); } uint8_t bytes[sizeof(int64_t)]; @@ -551,7 +558,7 @@ int CBB_add_asn1_int64(CBB *cbb, int64_t value) { } CBB child; - if (!CBB_add_asn1(cbb, &child, CBS_ASN1_INTEGER)) { + if (!CBB_add_asn1(cbb, &child, tag)) { return 0; } for (int i = start; i >= 0; i--) { diff --git a/include/openssl/bytestring.h b/include/openssl/bytestring.h index 1d47f60ab..33a417c16 100644 --- a/include/openssl/bytestring.h +++ b/include/openssl/bytestring.h @@ -557,11 +557,23 @@ OPENSSL_EXPORT void CBB_discard_child(CBB *cbb); // error. OPENSSL_EXPORT int CBB_add_asn1_uint64(CBB *cbb, uint64_t value); +// CBB_add_asn1_uint64_with_tag behaves like |CBB_add_asn1_uint64| but uses +// |tag| as the tag instead of INTEGER. This is useful if the INTEGER type uses +// implicit tagging. +OPENSSL_EXPORT int CBB_add_asn1_uint64_with_tag(CBB *cbb, uint64_t value, + unsigned tag); + // CBB_add_asn1_int64 writes an ASN.1 INTEGER into |cbb| using |CBB_add_asn1| // and writes |value| in its contents. It returns one on success and zero on // error. OPENSSL_EXPORT int CBB_add_asn1_int64(CBB *cbb, int64_t value); +// CBB_add_asn1_int64_with_tag behaves like |CBB_add_asn1_int64| but uses |tag| +// as the tag instead of INTEGER. This is useful if the INTEGER type uses +// implicit tagging. +OPENSSL_EXPORT int CBB_add_asn1_int64_with_tag(CBB *cbb, int64_t value, + unsigned tag); + // CBB_add_asn1_octet_string writes an ASN.1 OCTET STRING into |cbb| with the // given contents. It returns one on success and zero on error. OPENSSL_EXPORT int CBB_add_asn1_octet_string(CBB *cbb, const uint8_t *data,