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 <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index dee685a..cd47d2b 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -1418,6 +1418,66 @@
EXPECT_EQ(nullptr, null_type->value.ptr);
}
+TEST(ASN1Test, Pack) {
+ bssl::UniquePtr<BASIC_CONSTRAINTS> val(BASIC_CONSTRAINTS_new());
+ ASSERT_TRUE(val);
+ val->ca = 0;
+
+ // Test all three calling conventions.
+ static const uint8_t kExpected[] = {0x30, 0x00};
+ bssl::UniquePtr<ASN1_STRING> 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<ASN1_STRING> 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<BASIC_CONSTRAINTS> val(static_cast<BASIC_CONSTRAINTS *>(
+ 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<BASIC_CONSTRAINTS *>(
+ 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<BASIC_CONSTRAINTS *>(
+ 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 fd7ab4b..9124f23 100644
--- a/crypto/asn1/asn_pack.c
+++ b/crypto/asn1/asn_pack.c
@@ -59,48 +59,43 @@
#include <openssl/err.h>
#include <openssl/mem.h>
-/* 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;
-
- if (!oct || !*oct) {
- if (!(octmp = ASN1_STRING_new())) {
- OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE);
- return NULL;
- }
- if (oct)
- *oct = octmp;
- } else
- octmp = *oct;
-
- if (octmp->data) {
- OPENSSL_free(octmp->data);
- octmp->data = NULL;
- }
-
- if (!(octmp->length = ASN1_item_i2d(obj, &octmp->data, it))) {
+ 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 (!octmp->data) {
- OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE);
- return NULL;
- }
- return octmp;
-}
-/* Extract an ASN1 object from an ASN1_STRING */
+ 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;
+ }
+ } else {
+ ret = *out;
+ }
+
+ ASN1_STRING_set0(ret, new_data, len);
+ if (out != NULL) {
+ *out = ret;
+ }
+ return ret;
+}
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;
}
