From 82cf625ab51dbed91bc38bdbc21ba192df2dd4d4 Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Wed, 16 Sep 2020 18:26:22 +0300 Subject: SSL: workaround for incorrect SSL_write() errors in OpenSSL 1.1.1. OpenSSL 1.1.1 fails to return SSL_ERROR_SYSCALL if an error happens during SSL_write() after close_notify alert from the peer, and returns SSL_ERROR_ZERO_RETURN instead. Broken by this commit, which removes the "i == 0" check around the SSL_RECEIVED_SHUTDOWN one: https://git.openssl.org/?p=openssl.git;a=commitdiff;h=8051ab2 In particular, if a client closed the connection without reading the response but with properly sent close_notify alert, this resulted in unexpected "SSL_write() failed while ..." critical log message instead of correct "SSL_write() failed (32: Broken pipe)" at the info level. Since SSL_ERROR_ZERO_RETURN cannot be legitimately returned after SSL_write(), the fix is to convert all SSL_ERROR_ZERO_RETURN errors after SSL_write() to SSL_ERROR_SYSCALL. --- src/event/ngx_event_openssl.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'src/event') diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c index f387c720d..62b475d77 100644 --- a/src/event/ngx_event_openssl.c +++ b/src/event/ngx_event_openssl.c @@ -2573,6 +2573,18 @@ ngx_ssl_write(ngx_connection_t *c, u_char *data, size_t size) sslerr = SSL_get_error(c->ssl->connection, n); + if (sslerr == SSL_ERROR_ZERO_RETURN) { + + /* + * OpenSSL 1.1.1 fails to return SSL_ERROR_SYSCALL if an error + * happens during SSL_write() after close_notify alert from the + * peer, and returns SSL_ERROR_ZERO_RETURN instead, + * https://git.openssl.org/?p=openssl.git;a=commitdiff;h=8051ab2 + */ + + sslerr = SSL_ERROR_SYSCALL; + } + err = (sslerr == SSL_ERROR_SYSCALL) ? ngx_errno : 0; ngx_log_debug1(NGX_LOG_DEBUG_EVENT, c->log, 0, "SSL_get_error: %d", sslerr); -- cgit From a1864c25861c8cb0b972815e4cbafcf227c4f8cb Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Wed, 16 Sep 2020 18:26:23 +0300 Subject: SSL: fixed event handling during shutdown. The c->read->ready and c->write->ready flags need to be cleared to ensure that appropriate read or write events will be reported by kernel. Without this, SSL shutdown might wait till the timeout after blocking on writing or reading even if there is a socket activity. --- src/event/ngx_event_openssl.c | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'src/event') diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c index 62b475d77..2f51b133a 100644 --- a/src/event/ngx_event_openssl.c +++ b/src/event/ngx_event_openssl.c @@ -2865,6 +2865,13 @@ ngx_ssl_shutdown(ngx_connection_t *c) c->read->handler = ngx_ssl_shutdown_handler; c->write->handler = ngx_ssl_shutdown_handler; + if (sslerr == SSL_ERROR_WANT_READ) { + c->read->ready = 0; + + } else { + c->write->ready = 0; + } + if (ngx_handle_read_event(c->read, 0) != NGX_OK) { return NGX_ERROR; } -- cgit From f6c28f93aff7caad3c178d4989b73a6ed3281bcd Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Wed, 16 Sep 2020 18:26:24 +0300 Subject: SSL: disabled shutdown after connection errors. This fixes "SSL_shutdown() failed (SSL: ... bad write retry)" errors as observed on the second SSL_shutdown() call after SSL shutdown fixes in 09fb2135a589 (1.19.2), notably when sending fails in ngx_http_test_expect(), similarly to ticket #1194. Note that there are some places where c->error is misused to prevent further output, such as ngx_http_v2_finalize_connection() if there are pending streams, or in filter finalization. These places seem to be extreme enough to don't care about missing shutdown though. For example, filter finalization currently prevents keepalive from being used. --- src/event/ngx_event_openssl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/event') diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c index 2f51b133a..16dc55382 100644 --- a/src/event/ngx_event_openssl.c +++ b/src/event/ngx_event_openssl.c @@ -2805,7 +2805,7 @@ ngx_ssl_shutdown(ngx_connection_t *c) return NGX_OK; } - if (c->timedout) { + if (c->timedout || c->error) { mode = SSL_RECEIVED_SHUTDOWN|SSL_SENT_SHUTDOWN; SSL_set_quiet_shutdown(c->ssl->connection, 1); -- cgit From e9a8612c13380beb7b313d3ce50b223abda3f90a Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Wed, 16 Sep 2020 18:26:25 +0300 Subject: SSL: disabled shutdown when there are buffered data. This fixes "SSL_shutdown() failed (SSL: ... bad write retry)" errors as observed on the second SSL_shutdown() call after SSL shutdown fixes in 09fb2135a589 (1.19.2), notably when HTTP/2 connections are closed due to read timeouts while there are incomplete writes. --- src/event/ngx_event_openssl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/event') diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c index 16dc55382..da37b71df 100644 --- a/src/event/ngx_event_openssl.c +++ b/src/event/ngx_event_openssl.c @@ -2805,7 +2805,7 @@ ngx_ssl_shutdown(ngx_connection_t *c) return NGX_OK; } - if (c->timedout || c->error) { + if (c->timedout || c->error || c->buffered) { mode = SSL_RECEIVED_SHUTDOWN|SSL_SENT_SHUTDOWN; SSL_set_quiet_shutdown(c->ssl->connection, 1); -- cgit