PR comments; documentation and better checks

aws · Aug 21, 2024 · fd100a1 · fd100a1
1 parent 6e2ed43
commit fd100a1
Show file tree

Hide file tree

Showing 5 changed files with 518 additions and 486 deletions.
diff --git a/crypto/err/ocsp.errordata b/crypto/err/ocsp.errordata
@@ -8,6 +8,7 @@ OCSP,124,NEXTUPDATE_BEFORE_THISUPDATE
 OCSP,104,NOT_BASIC_RESPONSE
 OCSP,105,NO_CERTIFICATES_IN_CHAIN
 OCSP,108,NO_RESPONSE_DATA
+OCSP,109,NO_REVOKED_TIME
 OCSP,130,NO_SIGNER_KEY
 OCSP,131,OCSP_REQUEST_DUPLICATE_SIGNATURE
 OCSP,110,PRIVATE_KEY_DOES_NOT_MATCH_CERTIFICATE
@@ -19,5 +20,6 @@ OCSP,118,SIGNER_CERTIFICATE_NOT_FOUND
 OCSP,125,STATUS_EXPIRED
 OCSP,126,STATUS_NOT_YET_VALID
 OCSP,127,STATUS_TOO_OLD
+OCSP,132,UNKNOWN_FIELD_VALUE
 OCSP,119,UNKNOWN_MESSAGE_DIGEST
 OCSP,120,UNKNOWN_NID
diff --git a/crypto/ocsp/internal.h b/crypto/ocsp/internal.h
@@ -19,6 +19,11 @@ extern "C" {
 // See Reason Code RFC: https://www.rfc-editor.org/rfc/rfc5280#section-5.3.1.
 #define OCSP_UNASSIGNED_REVOKED_STATUS 7
 
+// OCSPResponseStatus does not have a status assigned to the value 4.
+//
+// See Reason Code RFC: https://datatracker.ietf.org/doc/html/rfc6960#section-4.2.1
+#define OCSP_UNASSIGNED_RESPONSE_STATUS 4
+
 // OCSP Request ASN.1 specification:
 // https://datatracker.ietf.org/doc/html/rfc6960#section-4.1.1
 //

diff --git a/crypto/ocsp/ocsp_server.c b/crypto/ocsp/ocsp_server.c
@@ -198,10 +198,12 @@ OCSP_SINGLERESP *OCSP_basic_add1_status(OCSP_BASICRESP *resp, OCSP_CERTID *cid,
                                         ASN1_TIME *revoked_time,
                                         ASN1_TIME *this_update,
                                         ASN1_TIME *next_update) {
-  GUARD_PTR(resp);
-  GUARD_PTR(resp->tbsResponseData);
-  GUARD_PTR(cid);
-  GUARD_PTR(this_update);
+  if (resp == NULL || resp->tbsResponseData == NULL || cid == NULL ||
+      this_update == NULL) {
+    OPENSSL_PUT_ERROR(OCSP, ERR_R_PASSED_NULL_PARAMETER);
+    return NULL;
+  }
+
   // Ambiguous status values are not allowed.
   if (status < V_OCSP_CERTSTATUS_GOOD || status > V_OCSP_CERTSTATUS_UNKNOWN) {
     OPENSSL_PUT_ERROR(OCSP, OCSP_R_UNKNOWN_FIELD_VALUE);
@@ -306,7 +308,7 @@ OCSP_RESPONSE *OCSP_response_create(int status, OCSP_BASICRESP *bs) {
   if (status < OCSP_RESPONSE_STATUS_SUCCESSFUL ||
       status > OCSP_RESPONSE_STATUS_UNAUTHORIZED ||
       // 4 is not a valid response status code.
-      status == 4) {
+      status == OCSP_UNASSIGNED_RESPONSE_STATUS) {
     OPENSSL_PUT_ERROR(OCSP, OCSP_R_UNKNOWN_FIELD_VALUE);
     return NULL;
   }
@@ -327,6 +329,10 @@ OCSP_RESPONSE *OCSP_response_create(int status, OCSP_BASICRESP *bs) {
   if (rsp->responseBytes == NULL) {
     goto err;
   }
+
+  // responseType will be id-pkix-ocsp-basic according to RFC6960.
+  //
+  // https://datatracker.ietf.org/doc/html/rfc6960#section-4.2.1
   rsp->responseBytes->responseType = OBJ_nid2obj(NID_id_pkix_OCSP_basic);
   if (!ASN1_item_pack(bs, ASN1_ITEM_rptr(OCSP_BASICRESP),
                       &rsp->responseBytes->response)) {

diff --git a/crypto/ocsp/ocsp_test.cc b/crypto/ocsp/ocsp_test.cc
@@ -134,7 +134,7 @@ static OCSP_CERTID *LoadTestOCSP_CERTID() {
       GetTestData(std::string("crypto/ocsp/test/aws/ca_cert.pem").c_str())
           .c_str()));
   bssl::UniquePtr<X509> server_cert(CertFromPEM(
-      GetTestData(std::string("crypto/ocsp/test/aws/server_cert.pem").c_str())
+      GetTestData(std::string("crypto/ocsp/test/aws/rsa_cert.pem").c_str())
           .c_str()));
   return OCSP_cert_to_id(nullptr, ca_cert.get(), server_cert.get());
 }
@@ -661,10 +661,24 @@ TEST(OCSPTest, BasicAddStatus) {
                                      V_OCSP_CERTSTATUS_GOOD, 0, nullptr,
                                      this_update.get(), next_update.get()));
 
+  // |revoked_reason| and |revoked_time| are ignored if |status| is
+  // |V_OCSP_CERTSTATUS_GOOD|, but will still succeed.
+  EXPECT_TRUE(OCSP_basic_add1_status(
+      basicResponse.get(), certId.get(), V_OCSP_CERTSTATUS_GOOD,
+      OCSP_REVOKED_STATUS_CACOMPROMISE, revoked_time.get(), this_update.get(),
+      next_update.get()));
+
   EXPECT_TRUE(OCSP_basic_add1_status(basicResponse.get(), certId.get(),
                                      V_OCSP_CERTSTATUS_UNKNOWN, 0, nullptr,
                                      this_update.get(), next_update.get()));
 
+  // |revoked_reason| and |revoked_time| are ignored if |status| is
+  // |V_OCSP_CERTSTATUS_UNKNOWN|, but will still succeed.
+  EXPECT_TRUE(OCSP_basic_add1_status(
+      basicResponse.get(), certId.get(), V_OCSP_CERTSTATUS_UNKNOWN,
+      OCSP_REVOKED_STATUS_SUPERSEDED, revoked_time.get(), this_update.get(),
+      next_update.get()));
+
   // Try setting a revoked response without an |ASN1_TIME|.
   EXPECT_FALSE(OCSP_basic_add1_status(basicResponse.get(), certId.get(),
                                       V_OCSP_CERTSTATUS_REVOKED, 0, nullptr,
@@ -685,11 +699,11 @@ TEST(OCSPTest, BasicAddStatus) {
   EXPECT_FALSE(OCSP_basic_add1_status(basicResponse.get(), certId.get(), 4, 0,
                                       nullptr, this_update.get(), nullptr));
 
-  // |OCSP_basic_add1_status| has succeeded 3 times at this point, so the
-  // |basicResponse| should have 3 |OCSP_SINGLERESP|s in the internal stack.
+  // |OCSP_basic_add1_status| has succeeded 5 times at this point, so the
+  // |basicResponse| should have 5 |OCSP_SINGLERESP|s in the internal stack.
   EXPECT_EQ((int)sk_OCSP_SINGLERESP_num(
                 basicResponse.get()->tbsResponseData->responses),
-            3);
+            5);
 }
 
 TEST(OCSPTest, OCSPResponseRecreate) {
@@ -719,7 +733,8 @@ TEST(OCSPTest, OCSPResponseRecreate) {
             Bytes(ocsp_response_data.data(), ocsp_response_data.size()));
 
   // Disallow creation of an |OCSP_RESPONSE| with an invalid status number.
-  EXPECT_FALSE(OCSP_response_create(4, basic_response.get()));
+  EXPECT_FALSE(OCSP_response_create(OCSP_UNASSIGNED_RESPONSE_STATUS,
+                                    basic_response.get()));
 }
 
 // === Translation of OpenSSL's OCSP tests ===