add support for OCSP_copy_nonce (#1711)

Ruby consumes `OCSP_copy_nonce`, which copies the nonce from the request
to the OCSP response.
OpenSSL's `OCSP_copy_nonce` directly appends the new OCSP nonce to the
list of extensions in the response. This allows multiple OCSP nonces to
exist the same response since the existing nonces are not deleted. It's
a bit strange to allow multiple nonces to exist in a single response. To
make things stranger, `OCSP_check_nonce` directly takes the first OCSP
nonce in the list to compare, so any newer nonces are ignored.
We ultimately decided to follow OpenSSL's faulty implementation and
document/test the behavior, there doesn't seem to be much value in
diverging from the original and will only introduce more unanticipated
behavioral changes.

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.

crypto/ocsp/internal.h

@@ -281,6 +281,12 @@ OPENSSL_EXPORT int OCSP_REQUEST_get_ext_by_NID(OCSP_REQUEST *req, int nid,
 // by its position in the extension list.
 OPENSSL_EXPORT X509_EXTENSION *OCSP_REQUEST_get_ext(OCSP_REQUEST *req, int loc);
 
+// OCSP_BASICRESP_add_ext adds a copy of |ex| to the extension list in
+// |*bs|. 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
+// out of bounds, the new extension is appended to the list.
+int OCSP_BASICRESP_add_ext(OCSP_BASICRESP *bs, X509_EXTENSION *ex, int loc);
+
 
 #define IS_OCSP_FLAG_SET(flags, query) (flags & query)
 #define OCSP_MAX_RESP_LENGTH (100 * 1024)

crypto/ocsp/ocsp_extension.c

@@ -28,6 +28,11 @@ X509_EXTENSION *OCSP_REQUEST_get_ext(OCSP_REQUEST *req, int loc) {
   return X509v3_get_ext(req->tbsRequest->requestExtensions, loc);
 }
 
+int OCSP_BASICRESP_add_ext(OCSP_BASICRESP *bs, X509_EXTENSION *ex, int loc) {
+  return (X509v3_add_ext(&bs->tbsResponseData->responseExtensions, ex, loc) !=
+          NULL);
+}
+
 int OCSP_BASICRESP_get_ext_by_NID(OCSP_BASICRESP *bs, int nid, int lastpos) {
   return X509v3_get_ext_by_NID(bs->tbsResponseData->responseExtensions, nid,
                                lastpos);
@@ -37,6 +42,10 @@ X509_EXTENSION *OCSP_BASICRESP_get_ext(OCSP_BASICRESP *bs, int loc) {
   return X509v3_get_ext(bs->tbsResponseData->responseExtensions, loc);
 }
 
+X509_EXTENSION *OCSP_BASICRESP_delete_ext(OCSP_BASICRESP *x, int loc) {
+  return X509v3_delete_ext(x->tbsResponseData->responseExtensions, loc);
+}
+
 int OCSP_SINGLERESP_add_ext(OCSP_SINGLERESP *sresp, X509_EXTENSION *ex,
                             int loc) {
   GUARD_PTR(sresp);
@@ -140,3 +149,22 @@ int OCSP_check_nonce(OCSP_REQUEST *req, OCSP_BASICRESP *bs) {
   }
   return OCSP_NONCE_EQUAL;
 }
+
+int OCSP_copy_nonce(OCSP_BASICRESP *resp, OCSP_REQUEST *req) {
+  GUARD_PTR(resp);
+  GUARD_PTR(req);
+
+  // Check for nonce in request.
+  int req_idx = OCSP_REQUEST_get_ext_by_NID(req, NID_id_pkix_OCSP_Nonce, -1);
+  // If no nonce, that's OK. We return 2 in this case.
+  if (req_idx < 0) {
+    return 2;
+  }
+  X509_EXTENSION *req_ext = OCSP_REQUEST_get_ext(req, req_idx);
+  // Nonce found, but no entry at the index.
+  // This shouldn't happen under normal circumstances.
+  GUARD_PTR(req_ext);
+
+  // Append the nonce.
+  return OCSP_BASICRESP_add_ext(resp, req_ext, -1);
+}

crypto/ocsp/ocsp_test.cc

@@ -1387,6 +1387,27 @@ TEST_P(OCSPNonceTest, OCSPNonce) {
   }
   EXPECT_EQ(OCSP_check_nonce(ocspRequest.get(), basicResponse.get()),
             t.nonce_check_status);
+
+  // Check that nonce copying from |req| to |bs| also works as expected.
+  if (t.nonce_check_status == OCSP_NONCE_RESPONSE_ONLY ||
+      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);
+  } 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);
+  }
 }
 
 TEST(OCSPTest, OCSPNonce) {

include/openssl/ocsp.h

@@ -195,6 +195,16 @@ OPENSSL_EXPORT int OCSP_request_add1_nonce(OCSP_REQUEST *req,
 //  but aren't equal.
 OPENSSL_EXPORT int OCSP_check_nonce(OCSP_REQUEST *req, OCSP_BASICRESP *bs);
 
+// OCSP_copy_nonce copies the nonce value (if any) from |req| to |resp|. Returns
+// 1 on success and 0 on failure. If the optional nonce value does not exist in
+// |req|, we return 2 instead.
+//
+// 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.
 OPENSSL_EXPORT int OCSP_request_set1_name(OCSP_REQUEST *req, X509_NAME *nm);
 
@@ -352,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.
@@ -413,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