# HG changeset patch # User Piotr Sikora <[email protected]> # Date 1470107238 25200 # Mon Aug 01 20:07:18 2016 -0700 # Node ID cd72e0a1164abd70aafdb391b3470869508532e5 # Parent d43ee392e825186545d81e683b88cc58ef8479bc SSL: fix order of checks during SSL certificate verification.
SSL_get_verify_result() should be called only if certificate was presented by the peer, otherwise returned value is the default one, which happens to be X509_V_OK, but it doesn't indicate success and it's considered a bug: https://www.openssl.org/docs/manmaster/ssl/SSL_get_verify_result.html While there, move common verification logic to ngx_ssl_verify_client() and ngx_ssl_check_host() in order to make the callers crypto-library-agnostic. Signed-off-by: Piotr Sikora <[email protected]> diff -r d43ee392e825 -r cd72e0a1164a src/event/ngx_event_openssl.c --- a/src/event/ngx_event_openssl.c +++ b/src/event/ngx_event_openssl.c @@ -3054,12 +3054,70 @@ ngx_ssl_cleanup_ctx(void *data) ngx_int_t +ngx_ssl_verify_client(ngx_connection_t *c, ngx_ssl_t *ssl, ngx_uint_t verify) +{ + long rc; + X509 *cert; + + cert = SSL_get_peer_certificate(c->ssl->connection); + if (cert == NULL) { + + if (verify != 1) { + return NGX_OK; + } + + ngx_log_error(NGX_LOG_INFO, c->log, 0, + "client sent no required SSL certificate"); + + ngx_ssl_remove_cached_session(ssl->ctx, + SSL_get0_session(c->ssl->connection)); + + return NGX_DECLINED; + } + + X509_free(cert); + + rc = SSL_get_verify_result(c->ssl->connection); + + if (rc != X509_V_OK + && (verify != 3 || !ngx_ssl_verify_error_optional(rc))) + { + ngx_log_error(NGX_LOG_INFO, c->log, 0, + "client SSL certificate verify error: (%l:%s)", + rc, X509_verify_cert_error_string(rc)); + + ngx_ssl_remove_cached_session(ssl->ctx, + SSL_get0_session(c->ssl->connection)); + + return NGX_ERROR; + } + + return NGX_OK; +} + + +ngx_int_t ngx_ssl_check_host(ngx_connection_t *c, ngx_str_t *name) { + long rc; X509 *cert; cert = SSL_get_peer_certificate(c->ssl->connection); if (cert == NULL) { + ngx_log_error(NGX_LOG_ERR, c->log, 0, + "upstream sent no required SSL certificate"); + + return NGX_ERROR; + } + + rc = SSL_get_verify_result(c->ssl->connection); + + if (rc != X509_V_OK) { + ngx_log_error(NGX_LOG_ERR, c->log, 0, + "upstream SSL certificate verify error: (%l:%s)", + rc, X509_verify_cert_error_string(rc)); + + X509_free(cert); return NGX_ERROR; } @@ -3170,6 +3228,10 @@ ngx_ssl_check_host(ngx_connection_t *c, failed: + ngx_log_error(NGX_LOG_ERR, c->log, 0, + "upstream SSL certificate does not match \"%V\"", + name); + X509_free(cert); return NGX_ERROR; @@ -3566,22 +3628,20 @@ ngx_ssl_get_client_verify(ngx_connection { X509 *cert; + cert = SSL_get_peer_certificate(c->ssl->connection); + if (cert == NULL) { + ngx_str_set(s, "NONE"); + return NGX_OK; + } + + X509_free(cert); + if (SSL_get_verify_result(c->ssl->connection) != X509_V_OK) { ngx_str_set(s, "FAILED"); return NGX_OK; } - cert = SSL_get_peer_certificate(c->ssl->connection); - - if (cert) { - ngx_str_set(s, "SUCCESS"); - - } else { - ngx_str_set(s, "NONE"); - } - - X509_free(cert); - + ngx_str_set(s, "SUCCESS"); return NGX_OK; } diff -r d43ee392e825 -r cd72e0a1164a src/event/ngx_event_openssl.h --- a/src/event/ngx_event_openssl.h +++ b/src/event/ngx_event_openssl.h @@ -184,6 +184,8 @@ ngx_int_t ngx_ssl_set_session(ngx_connec || n == X509_V_ERR_CERT_UNTRUSTED \ || n == X509_V_ERR_UNABLE_TO_VERIFY_LEAF_SIGNATURE) +ngx_int_t ngx_ssl_verify_client(ngx_connection_t *c, ngx_ssl_t *ssl, + ngx_uint_t verify); ngx_int_t ngx_ssl_check_host(ngx_connection_t *c, ngx_str_t *name); diff -r d43ee392e825 -r cd72e0a1164a src/http/ngx_http_request.c --- a/src/http/ngx_http_request.c +++ b/src/http/ngx_http_request.c @@ -1845,8 +1845,7 @@ ngx_http_process_request(ngx_http_reques #if (NGX_HTTP_SSL) if (r->http_connection->ssl) { - long rc; - X509 *cert; + ngx_int_t rc; ngx_http_ssl_srv_conf_t *sscf; if (c->ssl == NULL) { @@ -1859,38 +1858,13 @@ ngx_http_process_request(ngx_http_reques sscf = ngx_http_get_module_srv_conf(r, ngx_http_ssl_module); if (sscf->verify) { - rc = SSL_get_verify_result(c->ssl->connection); - - if (rc != X509_V_OK - && (sscf->verify != 3 || !ngx_ssl_verify_error_optional(rc))) - { - ngx_log_error(NGX_LOG_INFO, c->log, 0, - "client SSL certificate verify error: (%l:%s)", - rc, X509_verify_cert_error_string(rc)); - - ngx_ssl_remove_cached_session(sscf->ssl.ctx, - (SSL_get0_session(c->ssl->connection))); - - ngx_http_finalize_request(r, NGX_HTTPS_CERT_ERROR); + rc = ngx_ssl_verify_client(c, &sscf->ssl, sscf->verify); + if (rc != NGX_OK) { + ngx_http_finalize_request(r, (rc == NGX_ERROR) + ? NGX_HTTPS_CERT_ERROR + : NGX_HTTPS_NO_CERT); return; } - - if (sscf->verify == 1) { - cert = SSL_get_peer_certificate(c->ssl->connection); - - if (cert == NULL) { - ngx_log_error(NGX_LOG_INFO, c->log, 0, - "client sent no required SSL certificate"); - - ngx_ssl_remove_cached_session(sscf->ssl.ctx, - (SSL_get0_session(c->ssl->connection))); - - ngx_http_finalize_request(r, NGX_HTTPS_NO_CERT); - return; - } - - X509_free(cert); - } } } diff -r d43ee392e825 -r cd72e0a1164a src/http/ngx_http_upstream.c --- a/src/http/ngx_http_upstream.c +++ b/src/http/ngx_http_upstream.c @@ -1561,7 +1561,6 @@ ngx_http_upstream_ssl_init_connection(ng static void ngx_http_upstream_ssl_handshake(ngx_connection_t *c) { - long rc; ngx_http_request_t *r; ngx_http_upstream_t *u; @@ -1573,19 +1572,7 @@ ngx_http_upstream_ssl_handshake(ngx_conn if (c->ssl->handshaked) { if (u->conf->ssl_verify) { - rc = SSL_get_verify_result(c->ssl->connection); - - if (rc != X509_V_OK) { - ngx_log_error(NGX_LOG_ERR, c->log, 0, - "upstream SSL certificate verify error: (%l:%s)", - rc, X509_verify_cert_error_string(rc)); - goto failed; - } - if (ngx_ssl_check_host(c, &u->ssl_name) != NGX_OK) { - ngx_log_error(NGX_LOG_ERR, c->log, 0, - "upstream SSL certificate does not match \"%V\"", - &u->ssl_name); goto failed; } } diff -r d43ee392e825 -r cd72e0a1164a src/mail/ngx_mail_handler.c --- a/src/mail/ngx_mail_handler.c +++ b/src/mail/ngx_mail_handler.c @@ -16,8 +16,6 @@ static void ngx_mail_init_session(ngx_co #if (NGX_MAIL_SSL) static void ngx_mail_ssl_init_connection(ngx_ssl_t *ssl, ngx_connection_t *c); static void ngx_mail_ssl_handshake_handler(ngx_connection_t *c); -static ngx_int_t ngx_mail_verify_cert(ngx_mail_session_t *s, - ngx_connection_t *c); #endif @@ -247,15 +245,31 @@ ngx_mail_ssl_init_connection(ngx_ssl_t * static void ngx_mail_ssl_handshake_handler(ngx_connection_t *c) { + ngx_int_t rc; ngx_mail_session_t *s; + ngx_mail_ssl_conf_t *sslcf; ngx_mail_core_srv_conf_t *cscf; if (c->ssl->handshaked) { s = c->data; - if (ngx_mail_verify_cert(s, c) != NGX_OK) { - return; + sslcf = ngx_mail_get_module_srv_conf(s, ngx_mail_ssl_module); + + if (sslcf->verify) { + rc = ngx_ssl_verify_client(c, &sslcf->ssl, sslcf->verify); + if (rc != NGX_OK) { + cscf = ngx_mail_get_module_srv_conf(s, ngx_mail_core_module); + + s->out = (rc == NGX_ERROR) ? cscf->protocol->cert_error + : cscf->protocol->no_cert; + s->quit = 1; + + c->write->handler = ngx_mail_send; + + ngx_mail_send(s->connection->write); + return; + } } if (s->starttls) { @@ -278,71 +292,6 @@ ngx_mail_ssl_handshake_handler(ngx_conne ngx_mail_close_connection(c); } - -static ngx_int_t -ngx_mail_verify_cert(ngx_mail_session_t *s, ngx_connection_t *c) -{ - long rc; - X509 *cert; - ngx_mail_ssl_conf_t *sslcf; - ngx_mail_core_srv_conf_t *cscf; - - sslcf = ngx_mail_get_module_srv_conf(s, ngx_mail_ssl_module); - - if (!sslcf->verify) { - return NGX_OK; - } - - rc = SSL_get_verify_result(c->ssl->connection); - - if (rc != X509_V_OK - && (sslcf->verify != 3 || !ngx_ssl_verify_error_optional(rc))) - { - ngx_log_error(NGX_LOG_INFO, c->log, 0, - "client SSL certificate verify error: (%l:%s)", - rc, X509_verify_cert_error_string(rc)); - - ngx_ssl_remove_cached_session(sslcf->ssl.ctx, - (SSL_get0_session(c->ssl->connection))); - - cscf = ngx_mail_get_module_srv_conf(s, ngx_mail_core_module); - - s->out = cscf->protocol->cert_error; - s->quit = 1; - - c->write->handler = ngx_mail_send; - - ngx_mail_send(s->connection->write); - return NGX_ERROR; - } - - if (sslcf->verify == 1) { - cert = SSL_get_peer_certificate(c->ssl->connection); - - if (cert == NULL) { - ngx_log_error(NGX_LOG_INFO, c->log, 0, - "client sent no required SSL certificate"); - - ngx_ssl_remove_cached_session(sslcf->ssl.ctx, - (SSL_get0_session(c->ssl->connection))); - - cscf = ngx_mail_get_module_srv_conf(s, ngx_mail_core_module); - - s->out = cscf->protocol->no_cert; - s->quit = 1; - - c->write->handler = ngx_mail_send; - - ngx_mail_send(s->connection->write); - return NGX_ERROR; - } - - X509_free(cert); - } - - return NGX_OK; -} - #endif diff -r d43ee392e825 -r cd72e0a1164a src/stream/ngx_stream_proxy_module.c --- a/src/stream/ngx_stream_proxy_module.c +++ b/src/stream/ngx_stream_proxy_module.c @@ -986,7 +986,6 @@ ngx_stream_proxy_ssl_init_connection(ngx static void ngx_stream_proxy_ssl_handshake(ngx_connection_t *pc) { - long rc; ngx_stream_session_t *s; ngx_stream_upstream_t *u; ngx_stream_proxy_srv_conf_t *pscf; @@ -998,21 +997,7 @@ ngx_stream_proxy_ssl_handshake(ngx_conne if (pc->ssl->handshaked) { if (pscf->ssl_verify) { - rc = SSL_get_verify_result(pc->ssl->connection); - - if (rc != X509_V_OK) { - ngx_log_error(NGX_LOG_ERR, pc->log, 0, - "upstream SSL certificate verify error: (%l:%s)", - rc, X509_verify_cert_error_string(rc)); - goto failed; - } - - u = s->upstream; - - if (ngx_ssl_check_host(pc, &u->ssl_name) != NGX_OK) { - ngx_log_error(NGX_LOG_ERR, pc->log, 0, - "upstream SSL certificate does not match \"%V\"", - &u->ssl_name); + if (ngx_ssl_check_host(pc, &s->upstream->ssl_name) != NGX_OK) { goto failed; } } _______________________________________________ nginx-devel mailing list [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
