Repository: randombit/botan (cpp)

Issue:
OCSP Response containing multiple certificates
### Discussed in https://github.com/randombit/botan/discussions/4497

<div type='discussions-op-text'>

<sup>Originally posted by **overocean** December 24, 2024</sup>
Hi guys, I found that when we construct OCSP Response, we call `decode_optional_list()` to construct the certificates. Inside the body of the while loop in this function, `certbits` will be the sequence of certificates in the response data, and the X509_Certificate constructor will construct an object with the first certificate data in `certbits`. Therefore, other certificates are discarded. This seems a bug to me.

```
   BER_Decoder list(obj);

   while(list.more_items()) {
      BER_Object certbits = list.get_next_object();
      X509_Certificate cert(certbits.bits(), certbits.length());
      output.push_back(std::move(cert));
   }
```
</div>

Pull request description:
FIX: loading pinned certificates from OCSP responses
Before, the OCSP response would be parsed but if it contained more than one pinned certificate, the list would be truncated to only the first one. This may prevent successful path-building for verifying the OCSP response, for instance.

Closes #4502

Patch:
diff --git a/src/lib/x509/ocsp.cpp b/src/lib/x509/ocsp.cpp
index effcd819e..7b03877c5 100644
--- a/src/lib/x509/ocsp.cpp
+++ b/src/lib/x509/ocsp.cpp
@@ -35,12 +35,15 @@ void decode_optional_list(BER_Decoder& ber, ASN1_Type tag, std::vector<X509_Cert
    }
 
    BER_Decoder list(obj);
-
-   while(list.more_items()) {
-      BER_Object certbits = list.get_next_object();
-      X509_Certificate cert(certbits.bits(), certbits.length());
-      output.push_back(std::move(cert));
+   auto seq = list.start_sequence();
+   while(seq.more_items()) {
+      output.push_back([&] {
+         X509_Certificate cert;
+         cert.decode_from(seq);
+         return cert;
+      }());
    }
+   seq.end_cons();
 }
 
 }  // namespace


Test patch:
diff --git a/src/tests/test_ocsp.cpp b/src/tests/test_ocsp.cpp
index 4d0612817..e19df1431 100644
--- a/src/tests/test_ocsp.cpp
+++ b/src/tests/test_ocsp.cpp
@@ -119,6 +119,16 @@ class OCSP_Tests final : public Test {
          auto bdr_responder = load_test_X509_cert("x509/ocsp/bdr-ocsp-responder.pem");
          auto bdr_ca = load_test_X509_cert("x509/ocsp/bdr-int.pem");
 
+         // The response in bdr_ocsp contains two certificates
+         if(result.test_eq("both certificates found", bdr_ocsp.certificates().size(), 2)) {
+            result.test_eq("first cert in response",
+                           bdr_ocsp.certificates()[0].subject_info("X520.CommonName").at(0),
+                           "D-TRUST OCSP 4 2-2 EV 2016");
+            result.test_eq("second cert in response",
+                           bdr_ocsp.certificates()[1].subject_info("X520.CommonName").at(0),
+                           "D-TRUST CA 2-2 EV 2016");
+         }
+
          // Dummy OCSP response is not signed at all
          auto dummy_ocsp = Botan::OCSP::Response(Botan::Certificate_Status_Code::OCSP_SERVER_NOT_AVAILABLE);