commit cfcb0060e8b8fba92d275fa4ac27d369890ea9bf
author Adam Langley <agl@google.com> Tue Jun 25 10:03:07 2019 -0700
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Tue Jun 25 18:48:43 2019 +0000
tree ed6a7599916f2e378d34108a614d672d2f31d0c5
parent 7198a233689e250eb36d3d8e96b48b4cafb1be0a

Emit empty signerInfos in PKCS#7 bundles.

This is our bug that we've had since the beginning of PKCS#7 writing
support in eeb9f491: the empty signerInfos SET wasn't emitted. Some
parsers, including OpenSSL, don't like this but it appears to have taken
five years for anyone to notice.

This change does not make parsing strict so that we continue to parse
old messages that we may have produced.

(As ever, PKCS#* should not be used expect where absolutely required for
interoperability.)

Bug: b:135982177

Change-Id: Ia7241de69f105657bdfb5ff75e909deae71748a0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/36564
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>

crypto/pkcs7/pkcs7.c

diff --git a/crypto/pkcs7/pkcs7.c b/crypto/pkcs7/pkcs7.c
index c04bffd..1d0b139 100644
--- a/crypto/pkcs7/pkcs7.c
+++ b/crypto/pkcs7/pkcs7.c
@@ -134,7 +134,7 @@
 int pkcs7_bundle(CBB *out, int (*cb)(CBB *out, const void *arg),
                  const void *arg) {
   CBB outer_seq, oid, wrapped_seq, seq, version_bytes, digest_algos_set,
-      content_info;
+      content_info, signer_infos;
 
   // See https://tools.ietf.org/html/rfc2315#section-7
   if (!CBB_add_asn1(out, &outer_seq, CBS_ASN1_SEQUENCE) ||
@@ -150,7 +150,8 @@
       !CBB_add_asn1(&seq, &content_info, CBS_ASN1_SEQUENCE) ||
       !CBB_add_asn1(&content_info, &oid, CBS_ASN1_OBJECT) ||
       !CBB_add_bytes(&oid, kPKCS7Data, sizeof(kPKCS7Data)) ||
-      !cb(&seq, arg)) {
+      !cb(&seq, arg) ||
+      !CBB_add_asn1(&seq, &signer_infos, CBS_ASN1_SET)) {
     return 0;
   }
 

crypto/pkcs7/pkcs7_test.cc

diff --git a/crypto/pkcs7/pkcs7_test.cc b/crypto/pkcs7/pkcs7_test.cc
index 1ac9af2..948b44f 100644
--- a/crypto/pkcs7/pkcs7_test.cc
+++ b/crypto/pkcs7/pkcs7_test.cc
@@ -469,7 +469,7 @@
     "fNQMQoI9So4Vdy88Kow6BBBV3Lu6sZHue+cjxXETrmshNdNk8ABUMQA=\n"
     "-----END PKCS7-----\n";
 
-static void TestCertRepase(const uint8_t *der_bytes, size_t der_len) {
+static void TestCertReparse(const uint8_t *der_bytes, size_t der_len) {
   bssl::UniquePtr<STACK_OF(X509)> certs(sk_X509_new_null());
   ASSERT_TRUE(certs);
   bssl::UniquePtr<STACK_OF(X509)> certs2(sk_X509_new_null());
@@ -638,11 +638,11 @@
 }
 
 TEST(PKCS7Test, CertReparseNSS) {
-  TestCertRepase(kPKCS7NSS, sizeof(kPKCS7NSS));
+  TestCertReparse(kPKCS7NSS, sizeof(kPKCS7NSS));
 }
 
 TEST(PKCS7Test, CertReparseWindows) {
-  TestCertRepase(kPKCS7Windows, sizeof(kPKCS7Windows));
+  TestCertReparse(kPKCS7Windows, sizeof(kPKCS7Windows));
 }
 
 TEST(PKCS7Test, CrlReparse) {