From 686d05aaa57604b5d867f1d40e85f04759151ce1 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 17 Oct 2021 12:31:38 -0400 Subject: [PATCH] Fix error-handling for i2a_ASN1_OBJECT. Some BIO_write failures weren't handled. Otherwise would successfully write truncated results. The other i2a functions all report -1 on truncation, so match. While I'm here, write a test to make sure I didn't break this. Change-Id: If17d0209e75c15b3f37bceb1cdfb480fd2c62c4d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49931 Commit-Queue: David Benjamin Reviewed-by: Adam Langley --- crypto/asn1/a_object.c | 41 ++++++++++++-------- crypto/asn1/asn1_test.cc | 82 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 15 deletions(-) diff --git a/crypto/asn1/a_object.c b/crypto/asn1/a_object.c index b88f06b82..0de31415f 100644 --- a/crypto/asn1/a_object.c +++ b/crypto/asn1/a_object.c @@ -110,26 +110,37 @@ int i2t_ASN1_OBJECT(char *buf, int buf_len, const ASN1_OBJECT *a) return OBJ_obj2txt(buf, buf_len, a, 0); } +static int write_str(BIO *bp, const char *str) +{ + int len = strlen(str); + return BIO_write(bp, str, len) == len ? len : -1; +} + int i2a_ASN1_OBJECT(BIO *bp, const ASN1_OBJECT *a) { - char buf[80], *p = buf; - int i; + if (a == NULL || a->data == NULL) { + return write_str(bp, "NULL"); + } - if ((a == NULL) || (a->data == NULL)) - return (BIO_write(bp, "NULL", 4)); - i = i2t_ASN1_OBJECT(buf, sizeof buf, a); - if (i > (int)(sizeof(buf) - 1)) { - p = OPENSSL_malloc(i + 1); - if (!p) + char buf[80], *allocated = NULL; + const char *str = buf; + int len = i2t_ASN1_OBJECT(buf, sizeof(buf), a); + if (len > (int)sizeof(buf) - 1) { + /* The input was truncated. Allocate a buffer that fits. */ + allocated = OPENSSL_malloc(len + 1); + if (allocated == NULL) { return -1; - i2t_ASN1_OBJECT(p, i + 1, a); + } + len = i2t_ASN1_OBJECT(allocated, len + 1, a); + str = allocated; + } + if (len <= 0) { + str = ""; } - if (i <= 0) - return BIO_write(bp, "", 9); - BIO_write(bp, p, i); - if (p != buf) - OPENSSL_free(p); - return (i); + + int ret = write_str(bp, str); + OPENSSL_free(allocated); + return ret; } ASN1_OBJECT *d2i_ASN1_OBJECT(ASN1_OBJECT **a, const unsigned char **pp, diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc index bb2b0a892..1381169bd 100644 --- a/crypto/asn1/asn1_test.cc +++ b/crypto/asn1/asn1_test.cc @@ -1587,6 +1587,88 @@ TEST(ASN1Test, StringCmp) { } } +TEST(ASN1Test, PrintASN1Object) { + const struct { + std::vector in; + const char *expected; + } kDataTests[] = { + // Known OIDs print as the name. + {{0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01}, "rsaEncryption"}, + + // Unknown OIDs print in decimal. + {{0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04, 0x01, 0x84, 0xb7, 0x09, 0x00}, + "1.2.840.113554.4.1.72585.0"}, + + // Inputs which cannot be parsed as OIDs print as "". + {{0xff}, ""}, + + // The function has an internal 80-byte buffer. Test inputs at that + // boundary. First, 78 characters. + {{0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04, 0x01, 0x84, 0xb7, + 0x09, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01}, + "1.2.840.113554.4.1.72585.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0." + "0.0.0.1"}, + // 79 characters. + {{0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04, 0x01, 0x84, 0xb7, + 0x09, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0a}, + "1.2.840.113554.4.1.72585.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0." + "0.0.0.10"}, + // 80 characters. + {{0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04, 0x01, 0x84, 0xb7, + 0x09, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x64}, + "1.2.840.113554.4.1.72585.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0." + "0.0.0.100"}, + // 81 characters. + {{0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04, 0x01, 0x84, 0xb7, + 0x09, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x87, 0x68}, + "1.2.840.113554.4.1.72585.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0." + "0.0.0.1000"}, + // 82 characters. + {{0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04, 0x01, 0x84, 0xb7, + 0x09, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xce, 0x10}, + "1.2.840.113554.4.1.72585.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0." + "0.0.0.10000"}, + }; + for (const auto &t : kDataTests) { + SCOPED_TRACE(Bytes(t.in)); + bssl::UniquePtr obj(ASN1_OBJECT_create( + NID_undef, t.in.data(), t.in.size(), /*sn=*/nullptr, /*ln=*/nullptr)); + ASSERT_TRUE(obj); + bssl::UniquePtr bio(BIO_new(BIO_s_mem())); + ASSERT_TRUE(bio); + + int len = i2a_ASN1_OBJECT(bio.get(), obj.get()); + EXPECT_EQ(len, static_cast(strlen(t.expected))); + + const uint8_t *bio_data; + size_t bio_len; + BIO_mem_contents(bio.get(), &bio_data, &bio_len); + EXPECT_EQ(t.expected, + std::string(reinterpret_cast(bio_data), bio_len)); + } + + // Test writing NULL. + bssl::UniquePtr bio(BIO_new(BIO_s_mem())); + ASSERT_TRUE(bio); + int len = i2a_ASN1_OBJECT(bio.get(), nullptr); + EXPECT_EQ(len, 4); + const uint8_t *bio_data; + size_t bio_len; + BIO_mem_contents(bio.get(), &bio_data, &bio_len); + EXPECT_EQ("NULL", + std::string(reinterpret_cast(bio_data), bio_len)); +} + // The ASN.1 macros do not work on Windows shared library builds, where usage of // |OPENSSL_EXPORT| is a bit stricter. #if !defined(OPENSSL_WINDOWS) || !defined(BORINGSSL_SHARED_LIBRARY)