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>
diff --git a/crypto/bytestring/ber.c b/crypto/bytestring/ber.c
index bb5e17c..7437239 100644
--- a/crypto/bytestring/ber.c
+++ b/crypto/bytestring/ber.c
@@ -189,7 +189,7 @@
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 @@
}
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;
}
diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc
index 5d1b1da..639ddc7 100644
--- a/crypto/bytestring/bytestring_test.cc
+++ b/crypto/bytestring/bytestring_test.cc
@@ -558,19 +558,18 @@
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));
}
}
diff --git a/crypto/bytestring/internal.h b/crypto/bytestring/internal.h
index b731aad..7ef0e21 100644
--- a/crypto/bytestring/internal.h
+++ b/crypto/bytestring/internal.h
@@ -24,12 +24,10 @@
// 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 @@
// 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
diff --git a/crypto/pkcs7/pkcs7.c b/crypto/pkcs7/pkcs7.c
index fc175a9..393249b 100644
--- a/crypto/pkcs7/pkcs7.c
+++ b/crypto/pkcs7/pkcs7.c
@@ -41,23 +41,14 @@
// 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;
}
diff --git a/crypto/pkcs7/pkcs7_test.cc b/crypto/pkcs7/pkcs7_test.cc
index 54f7e8a..e146ede 100644
--- a/crypto/pkcs7/pkcs7_test.cc
+++ b/crypto/pkcs7/pkcs7_test.cc
@@ -480,6 +480,7 @@
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 @@
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 @@
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 @@
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()));
diff --git a/crypto/pkcs8/pkcs8_x509.c b/crypto/pkcs8/pkcs8_x509.c
index 23aad09..5d7178a 100644
--- a/crypto/pkcs8/pkcs8_x509.c
+++ b/crypto/pkcs8/pkcs8_x509.c
@@ -239,8 +239,7 @@
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 @@
// 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 @@
ret = 1;
err:
- OPENSSL_free(der_bytes);
+ OPENSSL_free(storage);
return ret;
}
@@ -586,8 +579,7 @@
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 @@
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 @@
ret = 1;
err:
- OPENSSL_free(der_bytes);
+ OPENSSL_free(storage);
if (!ret) {
EVP_PKEY_free(*out_key);
*out_key = NULL;
diff --git a/include/openssl/pkcs7.h b/include/openssl/pkcs7.h
index d708141..d9eb670 100644
--- a/include/openssl/pkcs7.h
+++ b/include/openssl/pkcs7.h
@@ -35,7 +35,7 @@
// 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 @@
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
