fix pkcs12 parse ordering. fixes pyca#5872 (pyca#5879)

* fix pkcs12 parse ordering. fixes pyca#5872

* remove an unneeded print

* simplify the test a bit more

* index

* black

* Update tests/hazmat/primitives/test_pkcs12.py

Co-authored-by: Alex Gaynor <[email protected]>

Co-authored-by: Alex Gaynor <[email protected]>

src/cryptography/hazmat/backends/openssl/backend.py

@@ -2546,7 +2546,9 @@ def load_key_and_certificates_from_pkcs12(self, data, password):
         if sk_x509_ptr[0] != self._ffi.NULL:
             sk_x509 = self._ffi.gc(sk_x509_ptr[0], self._lib.sk_X509_free)
             num = self._lib.sk_X509_num(sk_x509_ptr[0])
-            for i in range(num):
+            # In OpenSSL < 3.0.0 PKCS12 parsing reverses the order of the
+            # certificates.
+            for i in reversed(range(num)):
                 x509 = self._lib.sk_X509_value(sk_x509, i)
                 self.openssl_assert(x509 != self._ffi.NULL)
                 x509 = self._ffi.gc(x509, self._lib.X509_free)
@@ -2589,9 +2591,7 @@ def serialize_key_and_certificates_to_pkcs12(
             sk_x509 = self._lib.sk_X509_new_null()
             sk_x509 = self._ffi.gc(sk_x509, self._lib.sk_X509_free)
 
-            # reverse the list when building the stack so that they're encoded
-            # in the order they were originally provided. it is a mystery
-            for ca in reversed(cas):
+            for ca in cas:
                 res = self._lib.sk_X509_push(sk_x509, ca._x509)
                 backend.openssl_assert(res >= 1)
 

tests/hazmat/primitives/test_pkcs12.py

@@ -5,13 +5,15 @@
 from __future__ import absolute_import, division, print_function
 
 import os
+from datetime import datetime
 
 import pytest
 
 from cryptography import x509
 from cryptography.hazmat.backends.interfaces import DERSerializationBackend
 from cryptography.hazmat.backends.openssl.backend import _RC2
-from cryptography.hazmat.primitives import serialization
+from cryptography.hazmat.primitives import hashes, serialization
+from cryptography.hazmat.primitives.asymmetric import ec
 from cryptography.hazmat.primitives.serialization import load_pem_private_key
 from cryptography.hazmat.primitives.serialization.pkcs12 import (
     load_key_and_certificates,
@@ -269,3 +271,57 @@ def test_generate_unsupported_encryption_type(self, backend):
                 DummyKeySerializationEncryption(),
             )
         assert str(exc.value) == "Unsupported key encryption type"
+
+
+def test_pkcs12_ordering():
+    """
+    In OpenSSL < 3.0.0 PKCS12 parsing reverses the order. However, we
+    accidentally thought it was **encoding** that did it, leading to bug
+    https://github.com/pyca/cryptography/issues/5872
+    This test ensures our ordering is correct going forward.
+    """
+
+    def make_cert(name):
+        key = ec.generate_private_key(ec.SECP256R1())
+        subject = x509.Name(
+            [
+                x509.NameAttribute(x509.NameOID.COMMON_NAME, name),
+            ]
+        )
+        now = datetime.utcnow()
+        cert = (
+            x509.CertificateBuilder()
+            .subject_name(subject)
+            .issuer_name(subject)
+            .public_key(key.public_key())
+            .serial_number(x509.random_serial_number())
+            .not_valid_before(now)
+            .not_valid_after(now)
+            .sign(key, hashes.SHA256())
+        )
+        return (key, cert)
+
+    # Make some certificates with distinct names.
+    a_name = "A" * 20
+    b_name = "B" * 20
+    c_name = "C" * 20
+    a_key, a_cert = make_cert(a_name)
+    _, b_cert = make_cert(b_name)
+    _, c_cert = make_cert(c_name)
+
+    # Bundle them in a PKCS#12 file in order A, B, C.
+    p12 = serialize_key_and_certificates(
+        b"p12", a_key, a_cert, [b_cert, c_cert], serialization.NoEncryption()
+    )
+
+    # Parse them out. The API should report them in the same order.
+    (key, cert, certs) = load_key_and_certificates(p12, None)
+    assert cert == a_cert
+    assert certs == [b_cert, c_cert]
+
+    # The ordering in the PKCS#12 file itself should also match.
+    a_idx = p12.index(a_name.encode("utf-8"))
+    b_idx = p12.index(b_name.encode("utf-8"))
+    c_idx = p12.index(c_name.encode("utf-8"))
+
+    assert a_idx < b_idx < c_idx