Properly advance the CBS when parsing BER structures.

CBS_asn1_ber_to_der was a little cumbersome to use. While it, in theory,
allowed callers to consistently advance past the element, no caller
actually did so consistently. Instead they would advance if conversion
happened, and not if it was already DER. For the PKCS7_* functions, this
was even caller-exposed.

Change-Id: I658d265df899bace9ba6616cb465f19c9e6c3534
Reviewed-on: https://boringssl-review.googlesource.com/29304
Reviewed-by: Adam Langley <agl@google.com>

crypto/bytestring/ber.c

@@ -189,7 +189,7 @@ static int cbs_convert_ber(CBS *in, CBB *out, unsigned string_tag,
   return looking_for_eoc == 0;
 }
 
-int CBS_asn1_ber_to_der(CBS *in, uint8_t **out, size_t *out_len) {
+int CBS_asn1_ber_to_der(CBS *in, CBS *out, uint8_t **out_storage) {
   CBB cbb;
 
   // First, do a quick walk to find any indefinite-length elements. Most of the
@@ -200,18 +200,22 @@ int CBS_asn1_ber_to_der(CBS *in, uint8_t **out, size_t *out_len) {
   }
 
   if (!conversion_needed) {
-    *out = NULL;
-    *out_len = 0;
+    if (!CBS_get_any_asn1_element(in, out, NULL, NULL)) {
+      return 0;
+    }
+    *out_storage = NULL;
     return 1;
   }
 
+  size_t len;
   if (!CBB_init(&cbb, CBS_len(in)) ||
       !cbs_convert_ber(in, &cbb, 0, 0, 0) ||
-      !CBB_finish(&cbb, out, out_len)) {
+      !CBB_finish(&cbb, out_storage, &len)) {
     CBB_cleanup(&cbb);
     return 0;
   }
 
+  CBS_init(out, *out_storage, len);
   return 1;
 }
 

crypto/bytestring/bytestring_test.cc

@@ -558,19 +558,18 @@ static void ExpectBerConvert(const char *name, const uint8_t *der_expected,
                              size_t der_len, const uint8_t *ber,
                              size_t ber_len) {
   SCOPED_TRACE(name);
-  CBS in;
-  uint8_t *out;
-  size_t out_len;
+  CBS in, out;
+  uint8_t *storage;
 
   CBS_init(&in, ber, ber_len);
-  ASSERT_TRUE(CBS_asn1_ber_to_der(&in, &out, &out_len));
-  bssl::UniquePtr<uint8_t> scoper(out);
+  ASSERT_TRUE(CBS_asn1_ber_to_der(&in, &out, &storage));
+  bssl::UniquePtr<uint8_t> scoper(storage);
 
-  if (out == NULL) {
-    EXPECT_EQ(Bytes(der_expected, der_len), Bytes(ber, ber_len));
-  } else {
+  EXPECT_EQ(Bytes(der_expected, der_len), Bytes(CBS_data(&out), CBS_len(&out)));
+  if (storage != nullptr) {
     EXPECT_NE(Bytes(der_expected, der_len), Bytes(ber, ber_len));
-    EXPECT_EQ(Bytes(der_expected, der_len), Bytes(out, out_len));
+  } else {
+    EXPECT_EQ(Bytes(der_expected, der_len), Bytes(ber, ber_len));
   }
 }
 

crypto/bytestring/internal.h

@@ -24,12 +24,10 @@ extern "C" {
 
 // CBS_asn1_ber_to_der reads a BER element from |in|. If it finds
 // indefinite-length elements or constructed strings then it converts the BER
-// data to DER and sets |*out| and |*out_length| to describe a malloced buffer
-// containing the DER data. Additionally, |*in| will be advanced over the BER
-// element.
-//
-// If it doesn't find any indefinite-length elements or constructed strings then
-// it sets |*out| to NULL and |*in| is unmodified.
+// data to DER, sets |out| to the converted contents and |*out_storage| to a
+// buffer which the caller must release with |OPENSSL_free|. Otherwise, it sets
+// |out| to the original BER element in |in| and |*out_storage| to NULL.
+// Additionally, |*in| will be advanced over the BER element.
 //
 // This function should successfully process any valid BER input, however it
 // will not convert all of BER's deviations from DER. BER is ambiguous between
@@ -39,7 +37,8 @@ extern "C" {
 // must also account for BER variations in the contents of a primitive.
 //
 // It returns one on success and zero otherwise.
-OPENSSL_EXPORT int CBS_asn1_ber_to_der(CBS *in, uint8_t **out, size_t *out_len);
+OPENSSL_EXPORT int CBS_asn1_ber_to_der(CBS *in, CBS *out,
+                                       uint8_t **out_storage);
 
 // CBS_get_asn1_implicit_string parses a BER string of primitive type
 // |inner_tag| implicitly-tagged with |outer_tag|. It sets |out| to the

crypto/pkcs7/pkcs7.c

@@ -41,23 +41,14 @@ static const uint8_t kPKCS7SignedData[] = {0x2a, 0x86, 0x48, 0x86, 0xf7,
 // It returns one on success or zero on error. On error, |*der_bytes| is
 // NULL.
 int pkcs7_parse_header(uint8_t **der_bytes, CBS *out, CBS *cbs) {
-  size_t der_len;
   CBS in, content_info, content_type, wrapped_signed_data, signed_data;
   uint64_t version;
 
   // The input may be in BER format.
   *der_bytes = NULL;
-  if (!CBS_asn1_ber_to_der(cbs, der_bytes, &der_len)) {
-    return 0;
-  }
-  if (*der_bytes != NULL) {
-    CBS_init(&in, *der_bytes, der_len);
-  } else {
-    CBS_init(&in, CBS_data(cbs), CBS_len(cbs));
-  }
-
-  // See https://tools.ietf.org/html/rfc2315#section-7
-  if (!CBS_get_asn1(&in, &content_info, CBS_ASN1_SEQUENCE) ||
+  if (!CBS_asn1_ber_to_der(cbs, &in, der_bytes) ||
+      // See https://tools.ietf.org/html/rfc2315#section-7
+      !CBS_get_asn1(&in, &content_info, CBS_ASN1_SEQUENCE) ||
       !CBS_get_asn1(&content_info, &content_type, CBS_ASN1_OBJECT)) {
     goto err;
   }

crypto/pkcs7/pkcs7_test.cc

@@ -480,6 +480,7 @@ static void TestCertRepase(const uint8_t *der_bytes, size_t der_len) {
   CBS pkcs7;
   CBS_init(&pkcs7, der_bytes, der_len);
   ASSERT_TRUE(PKCS7_get_certificates(certs.get(), &pkcs7));
+  EXPECT_EQ(0u, CBS_len(&pkcs7));
 
   bssl::ScopedCBB cbb;
   ASSERT_TRUE(CBB_init(cbb.get(), der_len));
@@ -489,6 +490,7 @@ static void TestCertRepase(const uint8_t *der_bytes, size_t der_len) {
 
   CBS_init(&pkcs7, result_data, result_len);
   ASSERT_TRUE(PKCS7_get_certificates(certs2.get(), &pkcs7));
+  EXPECT_EQ(0u, CBS_len(&pkcs7));
 
   ASSERT_EQ(sk_X509_num(certs.get()), sk_X509_num(certs2.get()));
 
@@ -517,6 +519,7 @@ static void TestCRLReparse(const uint8_t *der_bytes, size_t der_len) {
   CBS pkcs7;
   CBS_init(&pkcs7, der_bytes, der_len);
   ASSERT_TRUE(PKCS7_get_CRLs(crls.get(), &pkcs7));
+  EXPECT_EQ(0u, CBS_len(&pkcs7));
 
   bssl::ScopedCBB cbb;
   ASSERT_TRUE(CBB_init(cbb.get(), der_len));
@@ -526,6 +529,7 @@ static void TestCRLReparse(const uint8_t *der_bytes, size_t der_len) {
 
   CBS_init(&pkcs7, result_data, result_len);
   ASSERT_TRUE(PKCS7_get_CRLs(crls2.get(), &pkcs7));
+  EXPECT_EQ(0u, CBS_len(&pkcs7));
 
   ASSERT_EQ(sk_X509_CRL_num(crls.get()), sk_X509_CRL_num(crls.get()));
 

crypto/pkcs8/pkcs8_x509.c

@@ -239,8 +239,7 @@ struct pkcs12_context {
 static int PKCS12_handle_sequence(
     CBS *sequence, struct pkcs12_context *ctx,
     int (*handle_element)(CBS *cbs, struct pkcs12_context *ctx)) {
-  uint8_t *der_bytes = NULL;
-  size_t der_len;
+  uint8_t *storage = NULL;
   CBS in;
   int ret = 0;
 
@@ -248,17 +247,11 @@ static int PKCS12_handle_sequence(
   // the ASN.1 data gets wrapped in OCTETSTRINGs and/or encrypted and the
   // conversion cannot see through those wrappings. So each time we step
   // through one we need to convert to DER again.
-  if (!CBS_asn1_ber_to_der(sequence, &der_bytes, &der_len)) {
+  if (!CBS_asn1_ber_to_der(sequence, &in, &storage)) {
     OPENSSL_PUT_ERROR(PKCS8, PKCS8_R_BAD_PKCS12_DATA);
     return 0;
   }
 
-  if (der_bytes != NULL) {
-    CBS_init(&in, der_bytes, der_len);
-  } else {
-    CBS_init(&in, CBS_data(sequence), CBS_len(sequence));
-  }
-
   CBS child;
   if (!CBS_get_asn1(&in, &child, CBS_ASN1_SEQUENCE) ||
       CBS_len(&in) != 0) {
@@ -281,7 +274,7 @@ static int PKCS12_handle_sequence(
   ret = 1;
 
 err:
-  OPENSSL_free(der_bytes);
+  OPENSSL_free(storage);
   return ret;
 }
 
@@ -586,8 +579,7 @@ err:
 
 int PKCS12_get_key_and_certs(EVP_PKEY **out_key, STACK_OF(X509) *out_certs,
                              CBS *ber_in, const char *password) {
-  uint8_t *der_bytes = NULL;
-  size_t der_len;
+  uint8_t *storage = NULL;
   CBS in, pfx, mac_data, authsafe, content_type, wrapped_authsafes, authsafes;
   uint64_t version;
   int ret = 0;
@@ -595,15 +587,10 @@ int PKCS12_get_key_and_certs(EVP_PKEY **out_key, STACK_OF(X509) *out_certs,
   const size_t original_out_certs_len = sk_X509_num(out_certs);
 
   // The input may be in BER format.
-  if (!CBS_asn1_ber_to_der(ber_in, &der_bytes, &der_len)) {
+  if (!CBS_asn1_ber_to_der(ber_in, &in, &storage)) {
     OPENSSL_PUT_ERROR(PKCS8, PKCS8_R_BAD_PKCS12_DATA);
     return 0;
   }
-  if (der_bytes != NULL) {
-    CBS_init(&in, der_bytes, der_len);
-  } else {
-    CBS_init(&in, CBS_data(ber_in), CBS_len(ber_in));
-  }
 
   *out_key = NULL;
   OPENSSL_memset(&ctx, 0, sizeof(ctx));
@@ -723,7 +710,7 @@ int PKCS12_get_key_and_certs(EVP_PKEY **out_key, STACK_OF(X509) *out_certs,
   ret = 1;
 
 err:
-  OPENSSL_free(der_bytes);
+  OPENSSL_free(storage);
   if (!ret) {
     EVP_PKEY_free(*out_key);
     *out_key = NULL;

include/openssl/pkcs7.h

@@ -35,7 +35,7 @@ DECLARE_STACK_OF(X509_CRL)
 
 // PKCS7_get_raw_certificates parses a PKCS#7, SignedData structure from |cbs|
 // and appends the included certificates to |out_certs|. It returns one on
-// success and zero on error.
+// success and zero on error. |cbs| is advanced passed the structure.
 OPENSSL_EXPORT int PKCS7_get_raw_certificates(
     STACK_OF(CRYPTO_BUFFER) *out_certs, CBS *cbs, CRYPTO_BUFFER_POOL *pool);
 
@@ -49,8 +49,8 @@ OPENSSL_EXPORT int PKCS7_bundle_certificates(
     CBB *out, const STACK_OF(X509) *certs);
 
 // PKCS7_get_CRLs parses a PKCS#7, SignedData structure from |cbs| and appends
-// the included CRLs to |out_crls|. It returns one on success and zero on
-// error.
+// the included CRLs to |out_crls|. It returns one on success and zero on error.
+// |cbs| is advanced passed the structure.
 OPENSSL_EXPORT int PKCS7_get_CRLs(STACK_OF(X509_CRL) *out_crls, CBS *cbs);
 
 // PKCS7_bundle_CRLs appends a PKCS#7, SignedData structure containing