From 4298fce7d6178f49ec1a306cf84078cf3be313e4 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 16 Oct 2021 09:10:53 -0400 Subject: [PATCH] Rewrite ASN1_item_pack and ASN1_item_unpack. ASN1_item_unpack was missing checks for trailing data. ASN1_item_pack's error handling was all wrong. (Leaking the temporary on error, checking the the wrong return value for i2d, would-be redundant check for NULL, were the other check not wrong.) Update-Note: ASN1_item_unpack now checks for trailing data. Change-Id: Ibaa19ba2b264fca36dd21109e66f9558d373c58b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49927 Reviewed-by: Adam Langley Commit-Queue: Adam Langley --- crypto/asn1/asn1_test.cc | 60 ++++++++++++++++++++++++++++++++++++++++ crypto/asn1/asn_pack.c | 53 ++++++++++++++++------------------- 2 files changed, 84 insertions(+), 29 deletions(-) diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc index dee685a56..cd47d2bb6 100644 --- a/crypto/asn1/asn1_test.cc +++ b/crypto/asn1/asn1_test.cc @@ -1418,6 +1418,66 @@ TEST(ASN1Test, Null) { EXPECT_EQ(nullptr, null_type->value.ptr); } +TEST(ASN1Test, Pack) { + bssl::UniquePtr val(BASIC_CONSTRAINTS_new()); + ASSERT_TRUE(val); + val->ca = 0; + + // Test all three calling conventions. + static const uint8_t kExpected[] = {0x30, 0x00}; + bssl::UniquePtr str( + ASN1_item_pack(val.get(), ASN1_ITEM_rptr(BASIC_CONSTRAINTS), nullptr)); + ASSERT_TRUE(str); + EXPECT_EQ( + Bytes(ASN1_STRING_get0_data(str.get()), ASN1_STRING_length(str.get())), + Bytes(kExpected)); + + ASN1_STRING *raw = nullptr; + str.reset(ASN1_item_pack(val.get(), ASN1_ITEM_rptr(BASIC_CONSTRAINTS), &raw)); + ASSERT_TRUE(str); + EXPECT_EQ(raw, str.get()); + EXPECT_EQ( + Bytes(ASN1_STRING_get0_data(str.get()), ASN1_STRING_length(str.get())), + Bytes(kExpected)); + + str.reset(ASN1_STRING_new()); + ASSERT_TRUE(str); + raw = str.get(); + EXPECT_TRUE( + ASN1_item_pack(val.get(), ASN1_ITEM_rptr(BASIC_CONSTRAINTS), &raw)); + EXPECT_EQ(raw, str.get()); + EXPECT_EQ( + Bytes(ASN1_STRING_get0_data(str.get()), ASN1_STRING_length(str.get())), + Bytes(kExpected)); +} + +TEST(ASN1Test, Unpack) { + bssl::UniquePtr str(ASN1_STRING_new()); + ASSERT_TRUE(str); + + static const uint8_t kValid[] = {0x30, 0x00}; + ASSERT_TRUE( + ASN1_STRING_set(str.get(), kValid, sizeof(kValid))); + bssl::UniquePtr val(static_cast( + ASN1_item_unpack(str.get(), ASN1_ITEM_rptr(BASIC_CONSTRAINTS)))); + ASSERT_TRUE(val); + EXPECT_EQ(val->ca, 0); + EXPECT_EQ(val->pathlen, nullptr); + + static const uint8_t kInvalid[] = {0x31, 0x00}; + ASSERT_TRUE(ASN1_STRING_set(str.get(), kInvalid, sizeof(kInvalid))); + val.reset(static_cast( + ASN1_item_unpack(str.get(), ASN1_ITEM_rptr(BASIC_CONSTRAINTS)))); + EXPECT_FALSE(val); + + static const uint8_t kTraiilingData[] = {0x30, 0x00, 0x00}; + ASSERT_TRUE( + ASN1_STRING_set(str.get(), kTraiilingData, sizeof(kTraiilingData))); + val.reset(static_cast( + ASN1_item_unpack(str.get(), ASN1_ITEM_rptr(BASIC_CONSTRAINTS)))); + EXPECT_FALSE(val); +} + // 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) diff --git a/crypto/asn1/asn_pack.c b/crypto/asn1/asn_pack.c index fd7ab4b06..9124f23d5 100644 --- a/crypto/asn1/asn_pack.c +++ b/crypto/asn1/asn_pack.c @@ -59,48 +59,43 @@ #include #include -/* ASN1_ITEM versions of the above */ -ASN1_STRING *ASN1_item_pack(void *obj, const ASN1_ITEM *it, ASN1_STRING **oct) +ASN1_STRING *ASN1_item_pack(void *obj, const ASN1_ITEM *it, ASN1_STRING **out) { - ASN1_STRING *octmp; + uint8_t *new_data = NULL; + int len = ASN1_item_i2d(obj, &new_data, it); + if (len <= 0) { + OPENSSL_PUT_ERROR(ASN1, ASN1_R_ENCODE_ERROR); + return NULL; + } - if (!oct || !*oct) { - if (!(octmp = ASN1_STRING_new())) { + ASN1_STRING *ret = NULL; + if (out == NULL || *out == NULL) { + ret = ASN1_STRING_new(); + if (ret == NULL) { OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE); + OPENSSL_free(new_data); return NULL; } - if (oct) - *oct = octmp; - } else - octmp = *oct; - - if (octmp->data) { - OPENSSL_free(octmp->data); - octmp->data = NULL; + } else { + ret = *out; } - if (!(octmp->length = ASN1_item_i2d(obj, &octmp->data, it))) { - OPENSSL_PUT_ERROR(ASN1, ASN1_R_ENCODE_ERROR); - return NULL; + ASN1_STRING_set0(ret, new_data, len); + if (out != NULL) { + *out = ret; } - if (!octmp->data) { - OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE); - return NULL; - } - return octmp; + return ret; } -/* Extract an ASN1 object from an ASN1_STRING */ - void *ASN1_item_unpack(const ASN1_STRING *oct, const ASN1_ITEM *it) { - const unsigned char *p; - void *ret; - - p = oct->data; - if (!(ret = ASN1_item_d2i(NULL, &p, oct->length, it))) + const unsigned char *p = oct->data; + void *ret = ASN1_item_d2i(NULL, &p, oct->length, it); + if (ret == NULL || p != oct->data + oct->length) { OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR); - /* TODO(davidben): Check for trailing data. */ + ASN1_item_free(ret, it); + return NULL; + } return ret; }