From 2d4f04bba0613292d8b51bf0de959e88afc72c54 Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Wed, 3 Jun 2020 19:11:32 +0300 Subject: SSL: added verify callback to ngx_ssl_trusted_certificate(). This ensures that certificate verification is properly logged to debug log during upstream server certificate verification. This should help with debugging various certificate issues. --- src/event/ngx_event_openssl.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'src/event') diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c index 264d4e7a4..c1d5d6a43 100644 --- a/src/event/ngx_event_openssl.c +++ b/src/event/ngx_event_openssl.c @@ -920,6 +920,8 @@ ngx_int_t ngx_ssl_trusted_certificate(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *cert, ngx_int_t depth) { + SSL_CTX_set_verify(ssl->ctx, SSL_VERIFY_PEER, ngx_ssl_verify_callback); + SSL_CTX_set_verify_depth(ssl->ctx, depth); if (cert->len == 0) { -- cgit From 7547581bbcb7b737710f9141260d822a08685b83 Mon Sep 17 00:00:00 2001 From: Roman Arutyunyan Date: Mon, 15 Jun 2020 20:17:16 +0300 Subject: OCSP: fixed use-after-free on error. When validating second and further certificates, ssl callback could be called twice to report the error. After the first call client connection is terminated and its memory is released. Prior to the second call and in it released connection memory is accessed. Errors triggering this behavior: - failure to create the request - failure to start resolving OCSP responder name - failure to start connecting to the OCSP responder The fix is to rearrange the code to eliminate the second call. --- src/event/ngx_event_openssl_stapling.c | 41 +++++++++++++++++----------------- 1 file changed, 20 insertions(+), 21 deletions(-) (limited to 'src/event') diff --git a/src/event/ngx_event_openssl_stapling.c b/src/event/ngx_event_openssl_stapling.c index a0a63c165..0e79d6cc4 100644 --- a/src/event/ngx_event_openssl_stapling.c +++ b/src/event/ngx_event_openssl_stapling.c @@ -980,6 +980,7 @@ ngx_ssl_ocsp_validate_next(ngx_connection_t *c) if (ocsp->ncert == n - 1 || (ocf->depth == 2 && ocsp->ncert == 1)) { ngx_log_debug1(NGX_LOG_DEBUG_EVENT, c->log, 0, "ssl ocsp validated, certs:%ui", ocsp->ncert); + rc = NGX_OK; goto done; } @@ -988,7 +989,8 @@ ngx_ssl_ocsp_validate_next(ngx_connection_t *c) ctx = ngx_ssl_ocsp_start(c->log); if (ctx == NULL) { - goto failed; + rc = NGX_ERROR; + goto done; } ocsp->ctx = ctx; @@ -1012,8 +1014,9 @@ ngx_ssl_ocsp_validate_next(ngx_connection_t *c) ctx->uri = ocf->uri; ctx->port = ocf->port; - if (ngx_ssl_ocsp_responder(c, ctx) != NGX_OK) { - goto failed; + rc = ngx_ssl_ocsp_responder(c, ctx); + if (rc != NGX_OK) { + goto done; } if (ctx->uri.len == 0) { @@ -1025,7 +1028,7 @@ ngx_ssl_ocsp_validate_next(ngx_connection_t *c) rc = ngx_ssl_ocsp_cache_lookup(ctx); if (rc == NGX_ERROR) { - goto failed; + goto done; } if (rc == NGX_DECLINED) { @@ -1051,12 +1054,12 @@ ngx_ssl_ocsp_validate_next(ngx_connection_t *c) done: - ocsp->status = NGX_OK; - return; - -failed: + ocsp->status = rc; - ocsp->status = NGX_ERROR; + if (c->ssl->in_ocsp) { + c->ssl->handshaked = 1; + c->ssl->handler(c); + } } @@ -1073,22 +1076,16 @@ ngx_ssl_ocsp_handler(ngx_ssl_ocsp_ctx_t *ctx) rc = ngx_ssl_ocsp_verify(ctx); if (rc != NGX_OK) { - ocsp->status = rc; - ngx_ssl_ocsp_done(ctx); goto done; } rc = ngx_ssl_ocsp_cache_store(ctx); if (rc != NGX_OK) { - ocsp->status = rc; - ngx_ssl_ocsp_done(ctx); goto done; } if (ctx->status != V_OCSP_CERTSTATUS_GOOD) { ocsp->cert_status = ctx->status; - ocsp->status = NGX_OK; - ngx_ssl_ocsp_done(ctx); goto done; } @@ -1096,15 +1093,17 @@ ngx_ssl_ocsp_handler(ngx_ssl_ocsp_ctx_t *ctx) ngx_ssl_ocsp_validate_next(c); -done: + return; - if (ocsp->status == NGX_AGAIN || !c->ssl->in_ocsp) { - return; - } +done: - c->ssl->handshaked = 1; + ocsp->status = rc; + ngx_ssl_ocsp_done(ctx); - c->ssl->handler(c); + if (c->ssl->in_ocsp) { + c->ssl->handshaked = 1; + c->ssl->handler(c); + } } -- cgit From fa2f2e35082ba01a8aed026b34fc5246637f104e Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Mon, 29 Jun 2020 17:15:51 +0300 Subject: SSL: fixed unexpected certificate requests (ticket #2008). Using SSL_CTX_set_verify(SSL_VERIFY_PEER) implies that OpenSSL will send a certificate request during an SSL handshake, leading to unexpected certificate requests from browsers as long as there are any client certificates installed. Given that ngx_ssl_trusted_certificate() is called unconditionally by the ngx_http_ssl_module, this affected all HTTPS servers. Broken by 699f6e55bbb4 (not released yet). Fix is to set verify callback in the ngx_ssl_trusted_certificate() function without changing the verify mode. --- src/event/ngx_event_openssl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'src/event') diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c index c1d5d6a43..f589b9812 100644 --- a/src/event/ngx_event_openssl.c +++ b/src/event/ngx_event_openssl.c @@ -920,7 +920,8 @@ ngx_int_t ngx_ssl_trusted_certificate(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *cert, ngx_int_t depth) { - SSL_CTX_set_verify(ssl->ctx, SSL_VERIFY_PEER, ngx_ssl_verify_callback); + SSL_CTX_set_verify(ssl->ctx, SSL_CTX_get_verify_mode(ssl->ctx), + ngx_ssl_verify_callback); SSL_CTX_set_verify_depth(ssl->ctx, depth); -- cgit From dfcfcc5a881bf4b349f74c9a0a04da2d861f02bf Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Mon, 6 Jul 2020 18:36:22 +0300 Subject: Upstream: drop extra data sent by upstream. Previous behaviour was to pass everything to the client, but this seems to be suboptimal and causes issues (ticket #1695). Fix is to drop extra data instead, as it naturally happens in most clients. This change covers generic buffered and unbuffered filters as used in the scgi and uwsgi modules. Appropriate input filter init handlers are provided by the scgi and uwsgi modules to set corresponding lengths. Note that for responses to HEAD requests there is an exception: we do allow any response length. This is because responses to HEAD requests might be actual full responses, and it is up to nginx to remove the response body. If caching is enabled, only full responses matching the Content-Length header will be cached (see b779728b180c). --- src/event/ngx_event_pipe.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) (limited to 'src/event') diff --git a/src/event/ngx_event_pipe.c b/src/event/ngx_event_pipe.c index 531b13aad..54412e130 100644 --- a/src/event/ngx_event_pipe.c +++ b/src/event/ngx_event_pipe.c @@ -960,6 +960,22 @@ ngx_event_pipe_copy_input_filter(ngx_event_pipe_t *p, ngx_buf_t *buf) return NGX_OK; } + if (p->upstream_done) { + ngx_log_debug0(NGX_LOG_DEBUG_EVENT, p->log, 0, + "input data after close"); + return NGX_OK; + } + + if (p->length == 0) { + p->upstream_done = 1; + + ngx_log_error(NGX_LOG_WARN, p->log, 0, + "upstream sent more data than specified in " + "\"Content-Length\" header"); + + return NGX_OK; + } + cl = ngx_chain_get_free_buf(p->pool, &p->free); if (cl == NULL) { return NGX_ERROR; @@ -987,6 +1003,18 @@ ngx_event_pipe_copy_input_filter(ngx_event_pipe_t *p, ngx_buf_t *buf) return NGX_OK; } + if (b->last - b->pos > p->length) { + + ngx_log_error(NGX_LOG_WARN, p->log, 0, + "upstream sent more data than specified in " + "\"Content-Length\" header"); + + b->last = b->pos + p->length; + p->upstream_done = 1; + + return NGX_OK; + } + p->length -= b->last - b->pos; return NGX_OK; -- cgit