* modules/ssl/ssl_engine_kernel.c (ssl_check_post_client_verify):

  Retrieve and set sslconn->client_cert here for both "modern" and
  classic access control.
  (ssl_hook_Access_classic, ssl_hook_Access_modern, ssl_hook_Access):
  Restore SSLRequire and FakeBasicAuth checks to ssl_hook_Access so tests
  are still applied for TLSv1.3.


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1841218 13f79535-47bb-0310-9956-ffa450edef68

1 files changed, 103 insertions, 95 deletions

diff --git a/modules/ssl/ssl_engine_kernel.c b/modules/ssl/ssl_engine_kernel.c
index ddf2a7b607..036794890d 100644
--- a/modules/ssl/ssl_engine_kernel.c
+++ b/modules/ssl/ssl_engine_kernel.c
@@ -431,8 +431,22 @@ static void ssl_configure_env(request_rec *r, SSLConnRec *sslconn)
 }
 
 static int ssl_check_post_client_verify(request_rec *r, SSLSrvConfigRec *sc, 
-                                        SSLDirConfigRec *dc, SSL *ssl)
+                                        SSLDirConfigRec *dc, SSLConnRec *sslconn,
+                                        SSL *ssl)
 {
+    X509 *cert;
+    
+    /*
+     * Remember the peer certificate's DN
+     */
+    if ((cert = SSL_get_peer_certificate(ssl))) {
+        if (sslconn->client_cert) {
+            X509_free(sslconn->client_cert);
+        }
+        sslconn->client_cert = cert;
+        sslconn->client_dn = NULL;
+    }
+    
     /*
      * Finally check for acceptable renegotiation results
      */
@@ -450,17 +464,13 @@ static int ssl_check_post_client_verify(request_rec *r, SSLSrvConfigRec *sc,
         }
 
         if (do_verify) {
-            X509 *peercert;
-
-            if ((peercert = SSL_get_peer_certificate(ssl)) == NULL) {
+            if (cert == NULL) {
                 ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02263)
                               "Re-negotiation handshake failed: "
                               "Client certificate missing");
 
                 return HTTP_FORBIDDEN;
             }
-
-            X509_free(peercert);
         }
     }
     return OK;
@@ -476,17 +486,13 @@ static int ssl_hook_Access_classic(request_rec *r, SSLSrvConfigRec *sc, SSLDirCo
     server_rec *handshakeserver = sslconn ? sslconn->server : NULL;
     SSLSrvConfigRec *hssc       = handshakeserver? mySrvConfig(handshakeserver) : NULL;
     SSL_CTX *ctx = ssl ? SSL_get_SSL_CTX(ssl) : NULL;
-    apr_array_header_t *requires;
-    ssl_require_t *ssl_requires;
-    int ok, i, rc;
     BOOL renegotiate = FALSE, renegotiate_quick = FALSE;
-    X509 *cert;
     X509 *peercert;
     X509_STORE *cert_store = NULL;
     X509_STORE_CTX *cert_store_ctx;
     STACK_OF(SSL_CIPHER) *cipher_list_old = NULL, *cipher_list = NULL;
     const SSL_CIPHER *cipher = NULL;
-    int depth, verify_old, verify, n;
+    int depth, verify_old, verify, n, rc;
     const char *ncipher_suite;
 
 #ifdef HAVE_SRP
@@ -866,6 +872,7 @@ static int ssl_hook_Access_classic(request_rec *r, SSLSrvConfigRec *sc, SSLDirCo
 
         if (renegotiate_quick) {
             STACK_OF(X509) *cert_stack;
+            X509 *cert;
 
             /* perform just a manual re-verification of the peer */
             ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02258)
@@ -1018,20 +1025,9 @@ static int ssl_hook_Access_classic(request_rec *r, SSLSrvConfigRec *sc, SSLDirCo
         }
 
         /*
-         * Remember the peer certificate's DN
-         */
-        if ((cert = SSL_get_peer_certificate(ssl))) {
-            if (sslconn->client_cert) {
-                X509_free(sslconn->client_cert);
-            }
-            sslconn->client_cert = cert;
-            sslconn->client_dn = NULL;
-        }
-
-        /*
          * Finally check for acceptable renegotiation results
          */
-        if (OK != (rc = ssl_check_post_client_verify(r, sc, dc, ssl))) {
+        if (OK != (rc = ssl_check_post_client_verify(r, sc, dc, sslconn, ssl))) {
             return rc;
         }
 
@@ -1055,74 +1051,6 @@ static int ssl_hook_Access_classic(request_rec *r, SSLSrvConfigRec *sc, SSLDirCo
         }
     }
 
-    /* If we're trying to have the user name set from a client
-     * certificate then we need to set it here. This should be safe as
-     * the user name probably isn't important from an auth checking point
-     * of view as the certificate supplied acts in that capacity.
-     * However, if FakeAuth is being used then this isn't the case so
-     * we need to postpone setting the username until later.
-     */
-    if ((dc->nOptions & SSL_OPT_FAKEBASICAUTH) == 0 && dc->szUserName) {
-        char *val = ssl_var_lookup(r->pool, r->server, r->connection,
-                                   r, (char *)dc->szUserName);
-        if (val && val[0])
-            r->user = val;
-        else
-            ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(02227)
-                          "Failed to set r->user to '%s'", dc->szUserName);
-    }
-
-    /*
-     * Check SSLRequire boolean expressions
-     */
-    requires = dc->aRequirement;
-    ssl_requires = (ssl_require_t *)requires->elts;
-
-    for (i = 0; i < requires->nelts; i++) {
-        ssl_require_t *req = &ssl_requires[i];
-        const char *errstring;
-        ok = ap_expr_exec(r, req->mpExpr, &errstring);
-
-        if (ok < 0) {
-            ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02265)
-                          "access to %s failed, reason: Failed to execute "
-                          "SSL requirement expression: %s",
-                          r->filename, errstring);
-
-            /* remember forbidden access for strict require option */
-            apr_table_setn(r->notes, "ssl-access-forbidden", "1");
-
-            return HTTP_FORBIDDEN;
-        }
-
-        if (ok != 1) {
-            ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02266)
-                          "Access to %s denied for %s "
-                          "(requirement expression not fulfilled)",
-                          r->filename, r->useragent_ip);
-
-            ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02228)
-                          "Failed expression: %s", req->cpExpr);
-
-            ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02229)
-                          "access to %s failed, reason: %s",
-                          r->filename,
-                          "SSL requirement expression not fulfilled");
-
-            /* remember forbidden access for strict require option */
-            apr_table_setn(r->notes, "ssl-access-forbidden", "1");
-
-            return HTTP_FORBIDDEN;
-        }
-    }
-
-    /*
-     * Else access is granted from our point of view (except vendor
-     * handlers override). But we have to return DECLINED here instead
-     * of OK, because mod_auth and other modules still might want to
-     * deny access.
-     */
-
     return DECLINED;
 }
 
@@ -1246,7 +1174,7 @@ static int ssl_hook_Access_modern(request_rec *r, SSLSrvConfigRec *sc, SSLDirCon
             /*
              * Finally check for acceptable renegotiation results
              */
-            if (OK != (rc = ssl_check_post_client_verify(r, sc, dc, ssl))) {
+            if (OK != (rc = ssl_check_post_client_verify(r, sc, dc, sslconn, ssl))) {
                 return rc;
             }
         }
@@ -1262,6 +1190,9 @@ int ssl_hook_Access(request_rec *r)
     SSLSrvConfigRec *sc         = mySrvConfig(r->server);
     SSLConnRec *sslconn         = myConnConfig(r->connection);
     SSL *ssl                    = sslconn ? sslconn->ssl : NULL;
+    apr_array_header_t *requires;
+    ssl_require_t *ssl_requires;
+    int ok, i, ret;
 
     /* On a slave connection, we do not expect to have an SSLConnRec, but
      * our master connection might have one. */
@@ -1317,10 +1248,87 @@ int ssl_hook_Access(request_rec *r)
     /* TLSv1.3+ is less complicated here. Branch off into a new codeline
      * and avoid messing with the past. */
     if (SSL_version(ssl) >= TLS1_3_VERSION) {
-        return ssl_hook_Access_modern(r, sc, dc, sslconn, ssl);
-    } 
+        ret = ssl_hook_Access_modern(r, sc, dc, sslconn, ssl);
+    }
+    else
 #endif
-    return ssl_hook_Access_classic(r, sc, dc, sslconn, ssl);
+    {
+        ret = ssl_hook_Access_classic(r, sc, dc, sslconn, ssl);
+    }
+
+    if (ret != DECLINED) {
+        return ret;
+    }
+
+    /* If we're trying to have the user name set from a client
+     * certificate then we need to set it here. This should be safe as
+     * the user name probably isn't important from an auth checking point
+     * of view as the certificate supplied acts in that capacity.
+     * However, if FakeAuth is being used then this isn't the case so
+     * we need to postpone setting the username until later.
+     */
+    if ((dc->nOptions & SSL_OPT_FAKEBASICAUTH) == 0 && dc->szUserName) {
+        char *val = ssl_var_lookup(r->pool, r->server, r->connection,
+                                   r, (char *)dc->szUserName);
+        if (val && val[0])
+            r->user = val;
+        else
+            ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(02227)
+                          "Failed to set r->user to '%s'", dc->szUserName);
+    }
+
+    /*
+     * Check SSLRequire boolean expressions
+     */
+    requires = dc->aRequirement;
+    ssl_requires = (ssl_require_t *)requires->elts;
+
+    for (i = 0; i < requires->nelts; i++) {
+        ssl_require_t *req = &ssl_requires[i];
+        const char *errstring;
+        ok = ap_expr_exec(r, req->mpExpr, &errstring);
+
+        if (ok < 0) {
+            ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02265)
+                          "access to %s failed, reason: Failed to execute "
+                          "SSL requirement expression: %s",
+                          r->filename, errstring);
+
+            /* remember forbidden access for strict require option */
+            apr_table_setn(r->notes, "ssl-access-forbidden", "1");
+
+            return HTTP_FORBIDDEN;
+        }
+
+        if (ok != 1) {
+            ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02266)
+                          "Access to %s denied for %s "
+                          "(requirement expression not fulfilled)",
+                          r->filename, r->useragent_ip);
+
+            ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02228)
+                          "Failed expression: %s", req->cpExpr);
+
+            ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02229)
+                          "access to %s failed, reason: %s",
+                          r->filename,
+                          "SSL requirement expression not fulfilled");
+
+            /* remember forbidden access for strict require option */
+            apr_table_setn(r->notes, "ssl-access-forbidden", "1");
+
+            return HTTP_FORBIDDEN;
+        }
+    }
+
+    /*
+     * Else access is granted from our point of view (except vendor
+     * handlers override). But we have to return DECLINED here instead
+     * of OK, because mod_auth and other modules still might want to
+     * deny access.
+     */
+
+    return DECLINED;
 }
 
 /*