ocsp_vfy.c: Clean up code w.r.t. coding guidelines and reduce redundancies

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/12669)

2 files changed, 111 insertions, 142 deletions

diff --git a/crypto/ocsp/ocsp_local.h b/crypto/ocsp/ocsp_local.h
index d354197d4b..fa291d37eb 100644
--- a/crypto/ocsp/ocsp_local.h
+++ b/crypto/ocsp/ocsp_local.h
@@ -234,5 +234,5 @@ struct ocsp_service_locator_st {
         &(a)->optionalSignature->signatureAlgorithm,\
         (a)->optionalSignature->signature,&(a)->tbsRequest,r)
 
-#  define OCSP_BASICRESP_verify(a,r,d) ASN1_item_verify(ASN1_ITEM_rptr(OCSP_RESPDATA),\
+#  define OCSP_BASICRESP_verify(a,r) ASN1_item_verify(ASN1_ITEM_rptr(OCSP_RESPDATA),\
         &(a)->signatureAlgorithm,(a)->signature,&(a)->tbsResponseData,r)
diff --git a/crypto/ocsp/ocsp_vfy.c b/crypto/ocsp/ocsp_vfy.c
index 3138716a0a..92512829c9 100644
--- a/crypto/ocsp/ocsp_vfy.c
+++ b/crypto/ocsp/ocsp_vfy.c
@@ -25,79 +25,102 @@ static int ocsp_req_find_signer(X509 **psigner, OCSP_REQUEST *req,
                                 const X509_NAME *nm, STACK_OF(X509) *certs,
                                 unsigned long flags);
 
-/* Verify a basic response message */
+/* Returns 1 on success, 0 on failure, or -1 on fatal error */
+static int ocsp_verify_signer(X509 *signer, X509_STORE *st, unsigned long flags,
+                              STACK_OF(X509) *untrusted, STACK_OF(X509) **chain)
+{
+    X509_STORE_CTX *ctx = X509_STORE_CTX_new();
+    X509_VERIFY_PARAM *vp;
+    int ret = -1;
 
+    if (ctx == NULL) {
+        OCSPerr(0, ERR_R_MALLOC_FAILURE);
+        goto end;
+    }
+    if (!X509_STORE_CTX_init(ctx, st, signer, untrusted)) {
+        OCSPerr(0, ERR_R_X509_LIB);
+        goto end;
+    }
+    if ((flags & OCSP_PARTIAL_CHAIN) != 0
+            && (vp = X509_STORE_CTX_get0_param(ctx)) != NULL)
+        X509_VERIFY_PARAM_set_flags(vp, X509_V_FLAG_PARTIAL_CHAIN);
+    X509_STORE_CTX_set_purpose(ctx, X509_PURPOSE_OCSP_HELPER);
+    X509_STORE_CTX_set_trust(ctx, X509_TRUST_OCSP_REQUEST);
+    /* TODO: why is X509_TRUST_OCSP_REQUEST set? Seems to get ignored. */
+
+    ret = X509_verify_cert(ctx);
+    if (ret <= 0) {
+        ret = X509_STORE_CTX_get_error(ctx);
+        OCSPerr(0, OCSP_R_CERTIFICATE_VERIFY_ERROR);
+        ERR_add_error_data(2, "Verify error:",
+                           X509_verify_cert_error_string(ret));
+        goto end;
+    }
+    if (chain != NULL)
+        *chain = X509_STORE_CTX_get1_chain(ctx);
+
+ end:
+    X509_STORE_CTX_free(ctx);
+    return ret;
+}
+
+static int ocsp_verify(OCSP_REQUEST *req, OCSP_BASICRESP *bs,
+                       X509 *signer, unsigned long flags)
+{
+    EVP_PKEY *skey;
+    int ret = 1;
+
+    if ((flags & OCSP_NOSIGS) == 0) {
+        if ((skey = X509_get0_pubkey(signer)) == NULL) {
+            OCSPerr(0, OCSP_R_NO_SIGNER_KEY);
+            return -1;
+        }
+        if (req != NULL)
+            ret = OCSP_REQUEST_verify(req, skey);
+        else
+            ret = OCSP_BASICRESP_verify(bs, skey);
+        if (ret <= 0)
+            OCSPerr(0, OCSP_R_SIGNATURE_FAILURE);
+    }
+    return ret;
+}
+
+/* Verify a basic response message */
 int OCSP_basic_verify(OCSP_BASICRESP *bs, STACK_OF(X509) *certs,
                       X509_STORE *st, unsigned long flags)
 {
     X509 *signer, *x;
     STACK_OF(X509) *chain = NULL;
     STACK_OF(X509) *untrusted = NULL;
-    X509_STORE_CTX *ctx = NULL;
-    X509_VERIFY_PARAM *vp;
-    int i, ret = ocsp_find_signer(&signer, bs, certs, flags);
+    int ret = ocsp_find_signer(&signer, bs, certs, flags);
 
-    if (!ret) {
+    if (ret == 0) {
         OCSPerr(OCSP_F_OCSP_BASIC_VERIFY,
                 OCSP_R_SIGNER_CERTIFICATE_NOT_FOUND);
         goto end;
     }
-    ctx = X509_STORE_CTX_new();
-    if (ctx == NULL) {
-        OCSPerr(OCSP_F_OCSP_BASIC_VERIFY, ERR_R_MALLOC_FAILURE);
-        goto f_err;
-    }
-    if ((ret == 2) && (flags & OCSP_TRUSTOTHER))
+    if ((ret == 2) && (flags & OCSP_TRUSTOTHER) != 0)
         flags |= OCSP_NOVERIFY;
-    if (!(flags & OCSP_NOSIGS)) {
-        EVP_PKEY *skey;
-
-        skey = X509_get0_pubkey(signer);
-        if (skey == NULL) {
-            OCSPerr(OCSP_F_OCSP_BASIC_VERIFY, OCSP_R_NO_SIGNER_KEY);
-            goto err;
-        }
-        ret = OCSP_BASICRESP_verify(bs, skey, 0);
-        if (ret <= 0) {
-            OCSPerr(OCSP_F_OCSP_BASIC_VERIFY, OCSP_R_SIGNATURE_FAILURE);
-            goto end;
-        }
-    }
-    if (!(flags & OCSP_NOVERIFY)) {
-        int init_res;
 
-        if (flags & OCSP_NOCHAIN) {
+    if ((ret = ocsp_verify(NULL, bs, signer, flags)) <= 0)
+        goto end;
+    if ((flags & OCSP_NOVERIFY) == 0) {
+        ret = -1;
+        if ((flags & OCSP_NOCHAIN) != 0) {
             untrusted = NULL;
-        } else if (bs->certs && certs) {
+        } else if (bs->certs != NULL && certs != NULL) {
             untrusted = sk_X509_dup(bs->certs);
             if (!X509_add_certs(untrusted, certs, X509_ADD_FLAG_DEFAULT))
-                goto f_err;
+                goto end;
         } else if (certs != NULL) {
             untrusted = certs;
         } else {
             untrusted = bs->certs;
         }
-        init_res = X509_STORE_CTX_init(ctx, st, signer, untrusted);
-        if (!init_res) {
-            OCSPerr(OCSP_F_OCSP_BASIC_VERIFY, ERR_R_X509_LIB);
-            goto f_err;
-        }
-        if ((flags & OCSP_PARTIAL_CHAIN) != 0
-                && (vp = X509_STORE_CTX_get0_param(ctx)) != NULL)
-            X509_VERIFY_PARAM_set_flags(vp, X509_V_FLAG_PARTIAL_CHAIN);
-
-        X509_STORE_CTX_set_purpose(ctx, X509_PURPOSE_OCSP_HELPER);
-        ret = X509_verify_cert(ctx);
-        chain = X509_STORE_CTX_get1_chain(ctx);
-        if (ret <= 0) {
-            i = X509_STORE_CTX_get_error(ctx);
-            OCSPerr(OCSP_F_OCSP_BASIC_VERIFY,
-                    OCSP_R_CERTIFICATE_VERIFY_ERROR);
-            ERR_add_error_data(2, "Verify error:",
-                               X509_verify_cert_error_string(i));
+        ret = ocsp_verify_signer(signer, st, flags, untrusted, &chain);
+        if (ret <= 0)
             goto end;
-        }
-        if (flags & OCSP_NOCHECKS) {
+        if ((flags & OCSP_NOCHECKS) != 0) {
             ret = 1;
             goto end;
         }
@@ -115,38 +138,29 @@ int OCSP_basic_verify(OCSP_BASICRESP *bs, STACK_OF(X509) *certs,
          * Easy case: explicitly trusted. Get root CA and check for explicit
          * trust
          */
-        if (flags & OCSP_NOEXPLICIT)
+        if ((flags & OCSP_NOEXPLICIT) != 0)
             goto end;
 
         x = sk_X509_value(chain, sk_X509_num(chain) - 1);
         if (X509_check_trust(x, NID_OCSP_sign, 0) != X509_TRUST_TRUSTED) {
             OCSPerr(OCSP_F_OCSP_BASIC_VERIFY, OCSP_R_ROOT_CA_NOT_TRUSTED);
-            goto err;
+            ret = 0;
+            goto end;
         }
         ret = 1;
     }
+
  end:
-    X509_STORE_CTX_free(ctx);
     sk_X509_pop_free(chain, X509_free);
     if (bs->certs && certs)
         sk_X509_free(untrusted);
     return ret;
-
- err:
-    ret = 0;
-    goto end;
- f_err:
-    ret = -1;
-    goto end;
 }
 
 int OCSP_resp_get0_signer(OCSP_BASICRESP *bs, X509 **signer,
                           STACK_OF(X509) *extra_certs)
 {
-    int ret;
-
-    ret = ocsp_find_signer(signer, bs, extra_certs, 0);
-    return (ret > 0) ? 1 : 0;
+    return ocsp_find_signer(signer, bs, extra_certs, 0) > 0;
 }
 
 static int ocsp_find_signer(X509 **psigner, OCSP_BASICRESP *bs,
@@ -155,11 +169,11 @@ static int ocsp_find_signer(X509 **psigner, OCSP_BASICRESP *bs,
     X509 *signer;
     OCSP_RESPID *rid = &bs->tbsResponseData.responderId;
 
-    if ((signer = ocsp_find_signer_sk(certs, rid))) {
+    if ((signer = ocsp_find_signer_sk(certs, rid)) != NULL) {
         *psigner = signer;
         return 2;
     }
-    if (!(flags & OCSP_NOINTERN) &&
+    if ((flags & OCSP_NOINTERN) == 0 &&
         (signer = ocsp_find_signer_sk(bs->certs, rid))) {
         *psigner = signer;
         return 1;
@@ -199,32 +213,31 @@ static X509 *ocsp_find_signer_sk(STACK_OF(X509) *certs, OCSP_RESPID *id)
 
 static int ocsp_check_issuer(OCSP_BASICRESP *bs, STACK_OF(X509) *chain)
 {
-    STACK_OF(OCSP_SINGLERESP) *sresp;
+    STACK_OF(OCSP_SINGLERESP) *sresp = bs->tbsResponseData.responses;
     X509 *signer, *sca;
     OCSP_CERTID *caid = NULL;
-    int i;
+    int ret;
 
-    sresp = bs->tbsResponseData.responses;
     if (sk_X509_num(chain) <= 0) {
         OCSPerr(OCSP_F_OCSP_CHECK_ISSUER, OCSP_R_NO_CERTIFICATES_IN_CHAIN);
         return -1;
     }
 
     /* See if the issuer IDs match. */
-    i = ocsp_check_ids(sresp, &caid);
+    ret = ocsp_check_ids(sresp, &caid);
 
     /* If ID mismatch or other error then return */
-    if (i <= 0)
-        return i;
+    if (ret <= 0)
+        return ret;
 
     signer = sk_X509_value(chain, 0);
     /* Check to see if OCSP responder CA matches request CA */
     if (sk_X509_num(chain) > 1) {
         sca = sk_X509_value(chain, 1);
-        i = ocsp_match_issuerid(sca, caid, sresp);
-        if (i < 0)
-            return i;
-        if (i) {
+        ret = ocsp_match_issuerid(sca, caid, sresp);
+        if (ret < 0)
+            return ret;
+        if (ret != 0) {
             /* We have a match, if extensions OK then success */
             if (ocsp_check_delegated(signer))
                 return 1;
@@ -258,7 +271,6 @@ static int ocsp_check_ids(STACK_OF(OCSP_SINGLERESP) *sresp, OCSP_CERTID **ret)
     cid = sk_OCSP_SINGLERESP_value(sresp, 0)->certId;
 
     *ret = NULL;
-
     for (i = 1; i < idcount; i++) {
         tmpid = sk_OCSP_SINGLERESP_value(sresp, i)->certId;
         /* Check to see if IDs match */
@@ -279,19 +291,18 @@ static int ocsp_check_ids(STACK_OF(OCSP_SINGLERESP) *sresp, OCSP_CERTID **ret)
 
 /*
  * Match the certificate issuer ID.
- * Returns -1 on error, 0 if there is no match and 1 if there is a match.
+ * Returns -1 on fatal error, 0 if there is no match and 1 if there is a match.
  */
 static int ocsp_match_issuerid(X509 *cert, OCSP_CERTID *cid,
                                STACK_OF(OCSP_SINGLERESP) *sresp)
 {
     /* If only one ID to match then do it */
     if (cid != NULL) {
-        const EVP_MD *dgst;
+        const EVP_MD *dgst = EVP_get_digestbyobj(cid->hashAlgorithm.algorithm);
         const X509_NAME *iname;
         int mdlen;
         unsigned char md[EVP_MAX_MD_SIZE];
 
-        dgst = EVP_get_digestbyobj(cid->hashAlgorithm.algorithm);
         if (dgst == NULL) {
             OCSPerr(0, OCSP_R_UNKNOWN_MESSAGE_DIGEST);
             return -1;
@@ -343,87 +354,46 @@ static int ocsp_check_delegated(X509 *x)
 }
 
 /*
- * Verify an OCSP request. This is fortunately much easier than OCSP response
- * verify. Just find the signers certificate and verify it against a given
- * trust value.
+ * Verify an OCSP request. This is much easier than OCSP response verify.
+ * Just find the signer's certificate and verify it against a given trust value.
+ * Returns 1 on success, 0 on failure and on fatal error.
  */
-
 int OCSP_request_verify(OCSP_REQUEST *req, STACK_OF(X509) *certs,
                         X509_STORE *store, unsigned long flags)
 {
     X509 *signer;
     const X509_NAME *nm;
     GENERAL_NAME *gen;
-    int ret = 0;
-    X509_STORE_CTX *ctx = X509_STORE_CTX_new();
-
-    if (ctx == NULL) {
-        OCSPerr(OCSP_F_OCSP_REQUEST_VERIFY, ERR_R_MALLOC_FAILURE);
-        goto err;
-    }
+    int ret;
 
     if (!req->optionalSignature) {
         OCSPerr(OCSP_F_OCSP_REQUEST_VERIFY, OCSP_R_REQUEST_NOT_SIGNED);
-        goto err;
+        return 0;
     }
     gen = req->tbsRequest.requestorName;
     if (!gen || gen->type != GEN_DIRNAME) {
         OCSPerr(OCSP_F_OCSP_REQUEST_VERIFY,
                 OCSP_R_UNSUPPORTED_REQUESTORNAME_TYPE);
-        goto err;
+        return 0; /* not returning -1 here for backward compatibility*/
     }
     nm = gen->d.directoryName;
     ret = ocsp_req_find_signer(&signer, req, nm, certs, flags);
     if (ret <= 0) {
         OCSPerr(OCSP_F_OCSP_REQUEST_VERIFY,
                 OCSP_R_SIGNER_CERTIFICATE_NOT_FOUND);
-        goto err;
+        return 0; /* not returning -1 here for backward compatibility*/
     }
-    if ((ret == 2) && (flags & OCSP_TRUSTOTHER))
+    if ((ret == 2) && (flags & OCSP_TRUSTOTHER) != 0)
         flags |= OCSP_NOVERIFY;
-    if (!(flags & OCSP_NOSIGS)) {
-        EVP_PKEY *skey;
-        skey = X509_get0_pubkey(signer);
-        ret = OCSP_REQUEST_verify(req, skey);
-        if (ret <= 0) {
-            OCSPerr(OCSP_F_OCSP_REQUEST_VERIFY, OCSP_R_SIGNATURE_FAILURE);
-            goto err;
-        }
-    }
-    if (!(flags & OCSP_NOVERIFY)) {
-        int init_res;
-
-        if (flags & OCSP_NOCHAIN)
-            init_res = X509_STORE_CTX_init(ctx, store, signer, NULL);
-        else
-            init_res = X509_STORE_CTX_init(ctx, store, signer,
-                                           req->optionalSignature->certs);
-        if (!init_res) {
-            OCSPerr(OCSP_F_OCSP_REQUEST_VERIFY, ERR_R_X509_LIB);
-            goto err;
-        }
-
-        X509_STORE_CTX_set_purpose(ctx, X509_PURPOSE_OCSP_HELPER);
-        X509_STORE_CTX_set_trust(ctx, X509_TRUST_OCSP_REQUEST);
-        ret = X509_verify_cert(ctx);
-        if (ret <= 0) {
-            ret = X509_STORE_CTX_get_error(ctx);
-            OCSPerr(OCSP_F_OCSP_REQUEST_VERIFY,
-                    OCSP_R_CERTIFICATE_VERIFY_ERROR);
-            ERR_add_error_data(2, "Verify error:",
-                               X509_verify_cert_error_string(ret));
-            goto err;
-        }
-    }
-    ret = 1;
-    goto end;
-
-err:
-    ret = 0;
-end:
-    X509_STORE_CTX_free(ctx);
-    return ret;
 
+    if ((ret = ocsp_verify(req, NULL, signer, flags)) <= 0)
+        return 0; /* not returning 'ret' here for backward compatibility*/
+    if ((flags & OCSP_NOVERIFY) != 0)
+        return 1;
+    return ocsp_verify_signer(signer, store, flags,
+                              (flags & OCSP_NOCHAIN) != 0 ?
+                              NULL : req->optionalSignature->certs, NULL) > 0;
+    /* using '> 0' here to avoid breaking backward compatibility returning -1 */
 }
 
 static int ocsp_req_find_signer(X509 **psigner, OCSP_REQUEST *req,
@@ -432,16 +402,15 @@ static int ocsp_req_find_signer(X509 **psigner, OCSP_REQUEST *req,
 {
     X509 *signer;
 
-    if (!(flags & OCSP_NOINTERN)) {
+    if ((flags & OCSP_NOINTERN) == 0) {
         signer = X509_find_by_subject(req->optionalSignature->certs, nm);
-        if (signer) {
+        if (signer != NULL) {
             *psigner = signer;
             return 1;
         }
     }
 
-    signer = X509_find_by_subject(certs, nm);
-    if (signer) {
+    if ((signer = X509_find_by_subject(certs, nm)) != NULL) {
         *psigner = signer;
         return 2;
     }