# HG changeset patch
# User Piotr Sikora <piotrsikora@google.com>
# 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 <piotrsikora@google.com>

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
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel