align OCSP_copy_nonce behavior with OpenSSL

crypto/ocsp/internal.h

@@ -287,11 +287,6 @@ OPENSSL_EXPORT X509_EXTENSION *OCSP_REQUEST_get_ext(OCSP_REQUEST *req, int loc);
 // out of bounds, the new extension is appended to the list.
 int OCSP_BASICRESP_add_ext(OCSP_BASICRESP *bs, X509_EXTENSION *ex, int loc);
 
-// OCSP_BASICRESP_delete_ext removes the extension in |x| at index |loc| and
-// returns the removed extension, or NULL if |loc| was out of bounds. If an
-// extension was returned, the caller must release it with
-// |X509_EXTENSION_free|.
-X509_EXTENSION *OCSP_BASICRESP_delete_ext(OCSP_BASICRESP *x, int loc);
 
 #define IS_OCSP_FLAG_SET(flags, query) (flags & query)
 #define OCSP_MAX_RESP_LENGTH (100 * 1024)

crypto/ocsp/ocsp_extension.c

@@ -165,15 +165,6 @@ int OCSP_copy_nonce(OCSP_BASICRESP *resp, OCSP_REQUEST *req) {
   // This shouldn't happen under normal circumstances.
   GUARD_PTR(req_ext);
 
-  // Delete the original nonce in |resp| if one exists.
-  int resp_idx =
-      OCSP_BASICRESP_get_ext_by_NID(resp, NID_id_pkix_OCSP_Nonce, -1);
-  if (resp_idx >= 0) {
-    X509_EXTENSION *old_resp_ext = OCSP_BASICRESP_delete_ext(resp, resp_idx);
-    GUARD_PTR(old_resp_ext);
-    X509_EXTENSION_free(old_resp_ext);
-  }
-
   // Append the nonce.
   return OCSP_BASICRESP_add_ext(resp, req_ext, -1);
 }

crypto/ocsp/ocsp_test.cc

@@ -1393,8 +1393,17 @@ TEST_P(OCSPNonceTest, OCSPNonce) {
       t.nonce_check_status == OCSP_NONCE_BOTH_ABSENT) {
     EXPECT_EQ(OCSP_copy_nonce(basicResponse.get(), ocspRequest.get()), 2);
     EXPECT_EQ(OCSP_check_nonce(ocspRequest.get(), basicResponse.get()),
-            t.nonce_check_status);
+              t.nonce_check_status);
   } else {
+    // OpenSSL's implementation of |OCSP_copy_nonce| keeps the original nonce in
+    // |resp| at the start of the list. We have to remove the old nonce prior to
+    // copying the new one over.
+    int resp_idx = OCSP_BASICRESP_get_ext_by_NID(basicResponse.get(),
+                                                 NID_id_pkix_OCSP_Nonce, -1);
+    if (resp_idx >= 0) {
+      bssl::UniquePtr<X509_EXTENSION> old_resp_ext(
+          OCSP_BASICRESP_delete_ext(basicResponse.get(), resp_idx));
+    }
     EXPECT_EQ(OCSP_copy_nonce(basicResponse.get(), ocspRequest.get()), 1);
     EXPECT_EQ(OCSP_check_nonce(ocspRequest.get(), basicResponse.get()),
               OCSP_NONCE_EQUAL);

include/openssl/ocsp.h

@@ -199,10 +199,10 @@ OPENSSL_EXPORT int OCSP_check_nonce(OCSP_REQUEST *req, OCSP_BASICRESP *bs);
 // 1 on success and 0 on failure. If the optional nonce value does not exist in
 // |req|, we return 2 instead.
 //
-// Note: Contrary to OpenSSL's |OCSP_copy_nonce| which allows for
-// multiple OCSP nonces to exist and appends the new nonce to the end of the
-// extension list, AWS-LC replaces the already existing OCSP nonce in |resp|, if
-// any.
+// Note: |OCSP_copy_nonce| allows for multiple OCSP nonces to exist and appends
+// the new nonce to the end of the extension list. This causes issues with
+// |OCSP_check_nonce|, since it looks for the first one in the list. The old
+// nonce extension should be deleted prior to calling |OCSP_copy_nonce|.
 OPENSSL_EXPORT int OCSP_copy_nonce(OCSP_BASICRESP *resp, OCSP_REQUEST *req);
 
 // OCSP_request_set1_name sets |requestorName| from an |X509_NAME| structure.
@@ -362,7 +362,8 @@ OPENSSL_EXPORT int OCSP_parse_url(const char *url, char **phost, char **pport,
 
 // OCSP_id_issuer_cmp compares the issuers' name and key hash of |a| and |b|. It
 // returns 0 on equal.
-OPENSSL_EXPORT int OCSP_id_issuer_cmp(const OCSP_CERTID *a, const OCSP_CERTID *b);
+OPENSSL_EXPORT int OCSP_id_issuer_cmp(const OCSP_CERTID *a,
+                                      const OCSP_CERTID *b);
 
 // OCSP_id_cmp calls |OCSP_id_issuer_cmp| and additionally compares the
 // |serialNumber| of |a| and |b|. It returns 0 on equal.
@@ -423,6 +424,13 @@ OPENSSL_EXPORT X509_EXTENSION *OCSP_BASICRESP_get_ext(OCSP_BASICRESP *bs,
 
 // OCSP |X509_EXTENSION| Functions
 
+// OCSP_BASICRESP_delete_ext removes the extension in |x| at index |loc| and
+// returns the removed extension, or NULL if |loc| was out of bounds. If an
+// extension was returned, the caller must release it with
+// |X509_EXTENSION_free|.
+OPENSSL_EXPORT X509_EXTENSION *OCSP_BASICRESP_delete_ext(OCSP_BASICRESP *x,
+                                                         int loc);
+
 // OCSP_SINGLERESP_add_ext adds a copy of |ex| to the extension list in
 // |*sresp|. It returns 1 on success and 0 on error. The new extension is
 // inserted at index |loc|, shifting extensions to the right. If |loc| is -1 or