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>

crypto/asn1/asn1_test.cc

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)

crypto/asn1/asn_pack.c

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;
 }