From 868cf157ddcb0f5154a96cad2cb1204829c667a0 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Tue, 18 Sep 2018 11:09:15 +0000 Subject: Merge r1841218 from trunk: * 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/branches/tlsv1.3-for-2.4.x@1841219 13f79535-47bb-0310-9956-ffa450edef68 --- modules/ssl/ssl_engine_kernel.c | 198 +++++++++++++++++++++------------------- 1 file changed, 103 insertions(+), 95 deletions(-) diff --git a/modules/ssl/ssl_engine_kernel.c b/modules/ssl/ssl_engine_kernel.c index 8be437c81d..9b28830389 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 = 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) @@ -1017,21 +1024,10 @@ static int ssl_hook_Access_classic(request_rec *r, SSLSrvConfigRec *sc, SSLDirCo sslconn->server = r->server; } - /* - * 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; } /* -- cgit v1.2.1