Correctly order PKCS#7 certificates and CRLs.
PKCS#7 stores certificates and CRLs in (implicitly-tagged) SET OF
types. This means they're unordered and, in DER, must be sorted.
We currently sort neither. OpenSSL upstream sorts CRLs but doesn't sort
certificates. https://github.com/openssl/openssl/pull/13143 reports that
Microsoft has a stricter parser that checks this. This CL fixes both
fields in our serializer.
This does not change the parsing code, which still preserves whatever
order we happened to find, but I've updated the documentation to clarify
that callers should not rely on the ordering.
Based on [0] and the odd order in kPKCS7NSS, I believe this aligns with
NSS's behavior.
Update-Note: It is no longer the case that constructing a PKCS#7 file
and parsing them back out will keep the certificates and CRLs in the
same order.
[0] https://source.chromium.org/chromium/chromium/src/+/main:chrome/common/net/x509_certificate_model_nss_unittest.cc;drc=c91b0c37b5ddf31cffd732c661c0c5930b0740f4;l=286
Change-Id: If776bb78476557af2c4598f1b6dc10e189adab5d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47304
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/pkcs7/pkcs7_test.cc b/crypto/pkcs7/pkcs7_test.cc
index 948b44f..8e14603 100644
--- a/crypto/pkcs7/pkcs7_test.cc
+++ b/crypto/pkcs7/pkcs7_test.cc
@@ -17,6 +17,7 @@
#include <openssl/bytestring.h>
#include <openssl/crypto.h>
#include <openssl/mem.h>
+#include <openssl/pem.h>
#include <openssl/pkcs7.h>
#include <openssl/stack.h>
#include <openssl/x509.h>
@@ -492,6 +493,9 @@
ASSERT_TRUE(PKCS7_get_certificates(certs2.get(), &pkcs7));
EXPECT_EQ(0u, CBS_len(&pkcs7));
+ // PKCS#7 stores certificates in a SET OF, so |PKCS7_bundle_certificates| may
+ // not preserve the original order. All of our test inputs are already sorted,
+ // but this check should be relaxed if we add others.
ASSERT_EQ(sk_X509_num(certs.get()), sk_X509_num(certs2.get()));
for (size_t i = 0; i < sk_X509_num(certs.get()); i++) {
X509 *a = sk_X509_value(certs.get(), i);
@@ -574,6 +578,9 @@
ASSERT_TRUE(PKCS7_get_CRLs(crls2.get(), &pkcs7));
EXPECT_EQ(0u, CBS_len(&pkcs7));
+ // PKCS#7 stores CRLs in a SET OF, so |PKCS7_bundle_CRLs| may not preserve the
+ // original order. All of our test inputs are already sorted, but this check
+ // should be relaxed if we add others.
ASSERT_EQ(sk_X509_CRL_num(crls.get()), sk_X509_CRL_num(crls.get()));
for (size_t i = 0; i < sk_X509_CRL_num(crls.get()); i++) {
X509_CRL *a = sk_X509_CRL_value(crls.get(), i);
@@ -656,3 +663,115 @@
TEST(PKCS7Test, PEMCRLs) {
TestPEMCRLs(kPEMCRL);
}
+
+// Test that we output certificates in the canonical DER order.
+TEST(PKCS7Test, SortCerts) {
+ // kPKCS7NSS contains three certificates in the canonical DER order.
+ CBS pkcs7;
+ CBS_init(&pkcs7, kPKCS7NSS, sizeof(kPKCS7NSS));
+ bssl::UniquePtr<STACK_OF(X509)> certs(sk_X509_new_null());
+ ASSERT_TRUE(certs);
+ ASSERT_TRUE(PKCS7_get_certificates(certs.get(), &pkcs7));
+ ASSERT_EQ(3u, sk_X509_num(certs.get()));
+
+ X509 *cert1 = sk_X509_value(certs.get(), 0);
+ X509 *cert2 = sk_X509_value(certs.get(), 1);
+ X509 *cert3 = sk_X509_value(certs.get(), 2);
+
+ auto check_order = [&](X509 *new_cert1, X509 *new_cert2, X509 *new_cert3) {
+ // Bundle the certificates in the new order.
+ bssl::UniquePtr<STACK_OF(X509)> new_certs(sk_X509_new_null());
+ ASSERT_TRUE(new_certs);
+ ASSERT_TRUE(bssl::PushToStack(new_certs.get(), bssl::UpRef(new_cert1)));
+ ASSERT_TRUE(bssl::PushToStack(new_certs.get(), bssl::UpRef(new_cert2)));
+ ASSERT_TRUE(bssl::PushToStack(new_certs.get(), bssl::UpRef(new_cert3)));
+ bssl::ScopedCBB cbb;
+ ASSERT_TRUE(CBB_init(cbb.get(), sizeof(kPKCS7NSS)));
+ ASSERT_TRUE(PKCS7_bundle_certificates(cbb.get(), new_certs.get()));
+
+ // The bundle should be sorted back to the original order.
+ CBS cbs;
+ CBS_init(&cbs, CBB_data(cbb.get()), CBB_len(cbb.get()));
+ bssl::UniquePtr<STACK_OF(X509)> result(sk_X509_new_null());
+ ASSERT_TRUE(result);
+ ASSERT_TRUE(PKCS7_get_certificates(result.get(), &cbs));
+ ASSERT_EQ(sk_X509_num(certs.get()), sk_X509_num(result.get()));
+ for (size_t i = 0; i < sk_X509_num(certs.get()); i++) {
+ X509 *a = sk_X509_value(certs.get(), i);
+ X509 *b = sk_X509_value(result.get(), i);
+ EXPECT_EQ(0, X509_cmp(a, b));
+ }
+ };
+
+ check_order(cert1, cert2, cert3);
+ check_order(cert3, cert2, cert1);
+ check_order(cert2, cert3, cert1);
+}
+
+// Test that we output CRLs in the canonical DER order.
+TEST(PKCS7Test, SortCRLs) {
+ static const char kCRL1[] = R"(
+-----BEGIN X509 CRL-----
+MIIBpzCBkAIBATANBgkqhkiG9w0BAQsFADBOMQswCQYDVQQGEwJVUzETMBEGA1UE
+CAwKQ2FsaWZvcm5pYTEWMBQGA1UEBwwNTW91bnRhaW4gVmlldzESMBAGA1UECgwJ
+Qm9yaW5nU1NMFw0xNjA5MjYxNTEwNTVaFw0xNjEwMjYxNTEwNTVaoA4wDDAKBgNV
+HRQEAwIBATANBgkqhkiG9w0BAQsFAAOCAQEAnrBKKgvd9x9zwK9rtUvVeFeJ7+LN
+ZEAc+a5oxpPNEsJx6hXoApYEbzXMxuWBQoCs5iEBycSGudct21L+MVf27M38KrWo
+eOkq0a2siqViQZO2Fb/SUFR0k9zb8xl86Zf65lgPplALun0bV/HT7MJcl04Tc4os
+dsAReBs5nqTGNEd5AlC1iKHvQZkM//MD51DspKnDpsDiUVi54h9C1SpfZmX8H2Vv
+diyu0fZ/bPAM3VAGawatf/SyWfBMyKpoPXEG39oAzmjjOj8en82psn7m474IGaho
+/vBbhl1ms5qQiLYPjm4YELtnXQoFyC72tBjbdFd/ZE9k4CNKDbxFUXFbkw==
+-----END X509 CRL-----
+)";
+ static const char kCRL2[] = R"(
+-----BEGIN X509 CRL-----
+MIIBvjCBpwIBATANBgkqhkiG9w0BAQsFADBOMQswCQYDVQQGEwJVUzETMBEGA1UE
+CAwKQ2FsaWZvcm5pYTEWMBQGA1UEBwwNTW91bnRhaW4gVmlldzESMBAGA1UECgwJ
+Qm9yaW5nU1NMFw0xNjA5MjYxNTEyNDRaFw0xNjEwMjYxNTEyNDRaMBUwEwICEAAX
+DTE2MDkyNjE1MTIyNlqgDjAMMAoGA1UdFAQDAgECMA0GCSqGSIb3DQEBCwUAA4IB
+AQCUGaM4DcWzlQKrcZvI8TMeR8BpsvQeo5BoI/XZu2a8h//PyRyMwYeaOM+3zl0d
+sjgCT8b3C1FPgT+P2Lkowv7rJ+FHJRNQkogr+RuqCSPTq65ha4WKlRGWkMFybzVH
+NloxC+aU3lgp/NlX9yUtfqYmJek1CDrOOGPrAEAwj1l/BUeYKNGqfBWYJQtPJu+5
+OaSvIYGpETCZJscUWODmLEb/O3DM438vLvxonwGqXqS0KX37+CHpUlyhnSovxXxp
+Pz4aF+L7OtczxL0GYtD2fR9B7TDMqsNmHXgQrixvvOY7MUdLGbd4RfJL3yA53hyO
+xzfKY2TzxLiOmctG0hXFkH5J
+-----END X509 CRL-----
+)";
+
+ bssl::UniquePtr<BIO> bio(BIO_new_mem_buf(kCRL1, strlen(kCRL1)));
+ ASSERT_TRUE(bio);
+ bssl::UniquePtr<X509_CRL> crl1(
+ PEM_read_bio_X509_CRL(bio.get(), nullptr, nullptr, nullptr));
+ ASSERT_TRUE(crl1);
+ bio.reset(BIO_new_mem_buf(kCRL2, strlen(kCRL2)));
+ ASSERT_TRUE(bio);
+ bssl::UniquePtr<X509_CRL> crl2(
+ PEM_read_bio_X509_CRL(bio.get(), nullptr, nullptr, nullptr));
+ ASSERT_TRUE(crl2);
+
+ // DER's SET OF ordering sorts by tag, then length, so |crl1| comes before
+ // |crl2|.
+ auto check_order = [&](X509_CRL *new_crl1, X509_CRL *new_crl2) {
+ // Bundle the CRLs in the new order.
+ bssl::UniquePtr<STACK_OF(X509_CRL)> new_crls(sk_X509_CRL_new_null());
+ ASSERT_TRUE(new_crls);
+ ASSERT_TRUE(bssl::PushToStack(new_crls.get(), bssl::UpRef(new_crl1)));
+ ASSERT_TRUE(bssl::PushToStack(new_crls.get(), bssl::UpRef(new_crl2)));
+ bssl::ScopedCBB cbb;
+ ASSERT_TRUE(CBB_init(cbb.get(), 64));
+ ASSERT_TRUE(PKCS7_bundle_CRLs(cbb.get(), new_crls.get()));
+
+ // The bundle should be sorted back to the original order.
+ CBS cbs;
+ CBS_init(&cbs, CBB_data(cbb.get()), CBB_len(cbb.get()));
+ bssl::UniquePtr<STACK_OF(X509_CRL)> result(sk_X509_CRL_new_null());
+ ASSERT_TRUE(result);
+ ASSERT_TRUE(PKCS7_get_CRLs(result.get(), &cbs));
+ ASSERT_EQ(2u, sk_X509_CRL_num(result.get()));
+ EXPECT_EQ(0, X509_CRL_cmp(crl1.get(), sk_X509_CRL_value(result.get(), 0)));
+ EXPECT_EQ(0, X509_CRL_cmp(crl2.get(), sk_X509_CRL_value(result.get(), 1)));
+ };
+
+ check_order(crl1.get(), crl2.get());
+ check_order(crl2.get(), crl1.get());
+}
diff --git a/crypto/pkcs7/pkcs7_x509.c b/crypto/pkcs7/pkcs7_x509.c
index 5afd28b..3f1526c 100644
--- a/crypto/pkcs7/pkcs7_x509.c
+++ b/crypto/pkcs7/pkcs7_x509.c
@@ -192,7 +192,8 @@
}
}
- return CBB_flush(out);
+ // |certificates| is a implicitly-tagged SET OF.
+ return CBB_flush_asn1_set_of(&certificates) && CBB_flush(out);
}
int PKCS7_bundle_certificates(CBB *out, const STACK_OF(X509) *certs) {
@@ -222,7 +223,8 @@
}
}
- return CBB_flush(out);
+ // |crl_data| is a implicitly-tagged SET OF.
+ return CBB_flush_asn1_set_of(&crl_data) && CBB_flush(out);
}
int PKCS7_bundle_CRLs(CBB *out, const STACK_OF(X509_CRL) *crls) {
diff --git a/include/openssl/pkcs7.h b/include/openssl/pkcs7.h
index d2d642a..8f2a885 100644
--- a/include/openssl/pkcs7.h
+++ b/include/openssl/pkcs7.h
@@ -38,7 +38,10 @@
// success and zero on error. |cbs| is advanced passed the structure.
//
// Note that a SignedData structure may contain no certificates, in which case
-// this function succeeds but does not append any certificates.
+// this function succeeds but does not append any certificates. Additionally,
+// certificates in SignedData structures are unordered. Callers should not
+// assume a particular order in |*out_certs| and may need to search for matches
+// or run path-building algorithms.
OPENSSL_EXPORT int PKCS7_get_raw_certificates(
STACK_OF(CRYPTO_BUFFER) *out_certs, CBS *cbs, CRYPTO_BUFFER_POOL *pool);
@@ -47,7 +50,9 @@
OPENSSL_EXPORT int PKCS7_get_certificates(STACK_OF(X509) *out_certs, CBS *cbs);
// PKCS7_bundle_certificates appends a PKCS#7, SignedData structure containing
-// |certs| to |out|. It returns one on success and zero on error.
+// |certs| to |out|. It returns one on success and zero on error. Note that
+// certificates in SignedData structures are unordered. The order in |certs|
+// will not be preserved.
OPENSSL_EXPORT int PKCS7_bundle_certificates(
CBB *out, const STACK_OF(X509) *certs);
@@ -56,11 +61,15 @@
// |cbs| is advanced passed the structure.
//
// Note that a SignedData structure may contain no CRLs, in which case this
-// function succeeds but does not append any CRLs.
+// function succeeds but does not append any CRLs. Additionally, CRLs in
+// SignedData structures are unordered. Callers should not assume an order in
+// |*out_crls| and may need to search for matches.
OPENSSL_EXPORT int PKCS7_get_CRLs(STACK_OF(X509_CRL) *out_crls, CBS *cbs);
// PKCS7_bundle_CRLs appends a PKCS#7, SignedData structure containing
-// |crls| to |out|. It returns one on success and zero on error.
+// |crls| to |out|. It returns one on success and zero on error. Note that CRLs
+// in SignedData structures are unordered. The order in |crls| will not be
+// preserved.
OPENSSL_EXPORT int PKCS7_bundle_CRLs(CBB *out, const STACK_OF(X509_CRL) *crls);
// PKCS7_get_PEM_certificates reads a PEM-encoded, PKCS#7, SignedData structure
@@ -68,7 +77,10 @@
// returns one on success and zero on error.
//
// Note that a SignedData structure may contain no certificates, in which case
-// this function succeeds but does not append any certificates.
+// this function succeeds but does not append any certificates. Additionally,
+// certificates in SignedData structures are unordered. Callers should not
+// assume a particular order in |*out_certs| and may need to search for matches
+// or run path-building algorithms.
OPENSSL_EXPORT int PKCS7_get_PEM_certificates(STACK_OF(X509) *out_certs,
BIO *pem_bio);
@@ -77,7 +89,9 @@
// success and zero on error.
//
// Note that a SignedData structure may contain no CRLs, in which case this
-// function succeeds but does not append any CRLs.
+// function succeeds but does not append any CRLs. Additionally, CRLs in
+// SignedData structures are unordered. Callers should not assume an order in
+// |*out_crls| and may need to search for matches.
OPENSSL_EXPORT int PKCS7_get_PEM_CRLs(STACK_OF(X509_CRL) *out_crls,
BIO *pem_bio);
@@ -192,7 +206,9 @@
// ignored. |flags| must be equal to |PKCS7_DETACHED|.
//
// Note this function only implements a subset of the corresponding OpenSSL
-// function. It is provided for backwards compatibility only.
+// function. It is provided for backwards compatibility only. Additionally,
+// certificates in SignedData structures are unordered. The order of |certs|
+// will not be preserved.
OPENSSL_EXPORT PKCS7 *PKCS7_sign(X509 *sign_cert, EVP_PKEY *pkey,
STACK_OF(X509) *certs, BIO *data, int flags);
