Chiebot-Mirror
/
boringssl
8
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Correctly order PKCS#7 certificates and CRLs.

Browse Source 
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>
grpc-202302
David Benjamin 4 years ago committed by Adam Langley
parent 94a63a5b6e
commit 1264f0ce35
3 changed files with 146 additions and 9 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Unified View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 119
      crypto/pkcs7/pkcs7_test.cc
  2. 6
      crypto/pkcs7/pkcs7_x509.c
  3. 30
      include/openssl/pkcs7.h

119
crypto/pkcs7/pkcs7_test.cc
Unescape View File

@ -17,6 +17,7 @@
#include <openssl/bytestring.h> #include <openssl/bytestring.h>
#include <openssl/crypto.h> #include <openssl/crypto.h>
#include <openssl/mem.h> #include <openssl/mem.h>
#include <openssl/pem.h>
#include <openssl/pkcs7.h> #include <openssl/pkcs7.h>
#include <openssl/stack.h> #include <openssl/stack.h>
#include <openssl/x509.h> #include <openssl/x509.h>
@ -492,6 +493,9 @@ static void TestCertReparse(const uint8_t *der_bytes, size_t der_len) {
ASSERT_TRUE(PKCS7_get_certificates(certs2.get(), &pkcs7)); ASSERT_TRUE(PKCS7_get_certificates(certs2.get(), &pkcs7));
EXPECT_EQ(0u, CBS_len(&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())); ASSERT_EQ(sk_X509_num(certs.get()), sk_X509_num(certs2.get()));
for (size_t i = 0; i < sk_X509_num(certs.get()); i++) { for (size_t i = 0; i < sk_X509_num(certs.get()); i++) {
X509 *a = sk_X509_value(certs.get(), i); X509 *a = sk_X509_value(certs.get(), i);
@ -574,6 +578,9 @@ static void TestCRLReparse(const uint8_t *der_bytes, size_t der_len) {
ASSERT_TRUE(PKCS7_get_CRLs(crls2.get(), &pkcs7)); ASSERT_TRUE(PKCS7_get_CRLs(crls2.get(), &pkcs7));
EXPECT_EQ(0u, CBS_len(&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())); 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++) { for (size_t i = 0; i < sk_X509_CRL_num(crls.get()); i++) {
X509_CRL *a = sk_X509_CRL_value(crls.get(), i); X509_CRL *a = sk_X509_CRL_value(crls.get(), i);
@ -656,3 +663,115 @@ TEST(PKCS7Test, PEMCerts) {
TEST(PKCS7Test, PEMCRLs) { TEST(PKCS7Test, PEMCRLs) {
TestPEMCRLs(kPEMCRL); 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());
}

6
crypto/pkcs7/pkcs7_x509.c
Unescape View File

@ -192,7 +192,8 @@ static int pkcs7_bundle_certificates_cb(CBB *out, const void *arg) {
} }
} }
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) { int PKCS7_bundle_certificates(CBB *out, const STACK_OF(X509) *certs) {
@ -222,7 +223,8 @@ static int pkcs7_bundle_crls_cb(CBB *out, const void *arg) {
} }
} }
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) { int PKCS7_bundle_CRLs(CBB *out, const STACK_OF(X509_CRL) *crls) {

30
include/openssl/pkcs7.h
Unescape View File

@ -38,7 +38,10 @@ DECLARE_STACK_OF(X509_CRL)
// success and zero on error. |cbs| is advanced passed the structure. // success and zero on error. |cbs| is advanced passed the structure.
// //
// Note that a SignedData structure may contain no certificates, in which case // 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( OPENSSL_EXPORT int PKCS7_get_raw_certificates(
STACK_OF(CRYPTO_BUFFER) *out_certs, CBS *cbs, CRYPTO_BUFFER_POOL *pool); STACK_OF(CRYPTO_BUFFER) *out_certs, CBS *cbs, CRYPTO_BUFFER_POOL *pool);
@ -47,7 +50,9 @@ OPENSSL_EXPORT int PKCS7_get_raw_certificates(
OPENSSL_EXPORT int PKCS7_get_certificates(STACK_OF(X509) *out_certs, CBS *cbs); OPENSSL_EXPORT int PKCS7_get_certificates(STACK_OF(X509) *out_certs, CBS *cbs);
// PKCS7_bundle_certificates appends a PKCS#7, SignedData structure containing // 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( OPENSSL_EXPORT int PKCS7_bundle_certificates(
CBB *out, const STACK_OF(X509) *certs); CBB *out, const STACK_OF(X509) *certs);
@ -56,11 +61,15 @@ OPENSSL_EXPORT int PKCS7_bundle_certificates(
// |cbs| is advanced passed the structure. // |cbs| is advanced passed the structure.
// //
// Note that a SignedData structure may contain no CRLs, in which case this // 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); OPENSSL_EXPORT int PKCS7_get_CRLs(STACK_OF(X509_CRL) *out_crls, CBS *cbs);
// PKCS7_bundle_CRLs appends a PKCS#7, SignedData structure containing // 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); 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 // PKCS7_get_PEM_certificates reads a PEM-encoded, PKCS#7, SignedData structure
@ -68,7 +77,10 @@ OPENSSL_EXPORT int PKCS7_bundle_CRLs(CBB *out, const STACK_OF(X509_CRL) *crls);
// returns one on success and zero on error. // returns one on success and zero on error.
// //
// Note that a SignedData structure may contain no certificates, in which case // 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, OPENSSL_EXPORT int PKCS7_get_PEM_certificates(STACK_OF(X509) *out_certs,
BIO *pem_bio); BIO *pem_bio);
@ -77,7 +89,9 @@ OPENSSL_EXPORT int PKCS7_get_PEM_certificates(STACK_OF(X509) *out_certs,
// success and zero on error. // success and zero on error.
// //
// Note that a SignedData structure may contain no CRLs, in which case this // 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, OPENSSL_EXPORT int PKCS7_get_PEM_CRLs(STACK_OF(X509_CRL) *out_crls,
BIO *pem_bio); BIO *pem_bio);
@ -192,7 +206,9 @@ OPENSSL_EXPORT int PKCS7_type_is_signedAndEnveloped(const PKCS7 *p7);
// ignored. |flags| must be equal to |PKCS7_DETACHED|. // ignored. |flags| must be equal to |PKCS7_DETACHED|.
// //
// Note this function only implements a subset of the corresponding OpenSSL // 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, OPENSSL_EXPORT PKCS7 *PKCS7_sign(X509 *sign_cert, EVP_PKEY *pkey,
STACK_OF(X509) *certs, BIO *data, int flags); STACK_OF(X509) *certs, BIO *data, int flags);

Write Preview
Loading…
Cancel
Save