diff --git a/.github/workflows/linux.yml b/.github/workflows/linux.yml index fc4e73b893e..8410fc78ea8 100644 --- a/.github/workflows/linux.yml +++ b/.github/workflows/linux.yml @@ -333,6 +333,9 @@ jobs: APR_VERSION=1.7.6 APU_VERSION=1.6.5 APU_CONFIG="--without-crypto" + TEST_PYTEST=1 + PYHTTPD_TARGETS=modules/ssl + PYTEST_ARGS=--only=pyhttpd # ------------------------------------------------------------------------- - name: OpenSSL 3.4 -Werror config: --enable-mods-shared=most --enable-maintainer-mode --disable-md --disable-http2 --disable-ldap --disable-crypto @@ -342,6 +345,9 @@ jobs: APR_VERSION=1.7.6 APU_VERSION=1.6.5 APU_CONFIG="--without-crypto" + TEST_PYTEST=1 + PYHTTPD_TARGETS=modules/ssl + PYTEST_ARGS=--only=pyhttpd # ------------------------------------------------------------------------- - name: OpenSSL 3.4 no-engine config: --enable-mods-shared=most --enable-maintainer-mode --disable-md --disable-http2 --disable-ldap --disable-crypto @@ -351,6 +357,9 @@ jobs: APR_VERSION=1.7.6 APU_VERSION=1.6.5 APU_CONFIG="--without-crypto" + TEST_PYTEST=1 + PYHTTPD_TARGETS=modules/ssl + PYTEST_ARGS=--only=pyhttpd # ------------------------------------------------------------------------- - name: OpenSSL 3.5 no-engine -Werror config: --enable-mods-shared=most --enable-maintainer-mode --disable-md --disable-http2 --disable-ldap --disable-crypto @@ -361,6 +370,9 @@ jobs: APR_VERSION=1.7.6 APU_VERSION=1.6.5 APU_CONFIG="--without-crypto" + TEST_PYTEST=1 + PYHTTPD_TARGETS=modules/ssl + PYTEST_ARGS=--only=pyhttpd # ------------------------------------------------------------------------- - name: OpenSSL 4.0 config: --enable-mods-shared=most --enable-maintainer-mode --disable-md --disable-http2 --disable-ldap --disable-crypto @@ -371,6 +383,9 @@ jobs: APR_VERSION=1.7.6 APU_VERSION=1.6.5 APU_CONFIG="--without-crypto" + TEST_PYTEST=1 + PYHTTPD_TARGETS=modules/ssl + PYTEST_ARGS=--only=pyhttpd # ------------------------------------------------------------------------- runs-on: ${{ matrix.os == '' && 'ubuntu-latest' || matrix.os }} timeout-minutes: 30 diff --git a/changes-entries/ssl-reneg-timeout-shutdown.txt b/changes-entries/ssl-reneg-timeout-shutdown.txt new file mode 100644 index 00000000000..52307173bd7 --- /dev/null +++ b/changes-entries/ssl-reneg-timeout-shutdown.txt @@ -0,0 +1,3 @@ + *) mod_ssl: Send a close_notify alert when a client times out during a + renegotiation or TLSv1.3 Post-Handshake Authentication, rather than + dropping the connection without one. [Joe Orton] diff --git a/modules/ssl/ssl_engine_io.c b/modules/ssl/ssl_engine_io.c index 2156ab40a49..76eff3ca418 100644 --- a/modules/ssl/ssl_engine_io.c +++ b/modules/ssl/ssl_engine_io.c @@ -516,7 +516,19 @@ static int bio_filter_in_read(BIO *bio, char *in, int inlen) if (block == APR_BLOCK_READ && APR_STATUS_IS_TIMEUP(inctx->rc) && APR_BRIGADE_EMPTY(inctx->bb)) { - /* don't give up, just return the timeout */ + SSLConnRec *sslconn = myConnConfig(inctx->f->c); + + /* A timeout is a retryable read: the connection is still up, it + * is only this wait which is over. Flagging it retryable keeps + * the SSL session usable - reporting an I/O error here would put + * the state machine into an error state, after which OpenSSL + * refuses to send a close_notify alert. The timeout itself is + * reported out of band, as OpenSSL's own BIOs do for a datagram + * receive timeout. */ + BIO_set_retry_read(bio); + if (sslconn) { + sslconn->read_timedout = 1; + } return -1; } if (inctx->rc != APR_SUCCESS) { @@ -730,6 +742,8 @@ static apr_status_t ssl_io_input_read(bio_filter_in_ctx_t *inctx, * (This is usually the case when the client forces an SSL * renegotiation which is handled implicitly by OpenSSL.) */ + int timedout = APR_STATUS_IS_TIMEUP(inctx->rc); + inctx->rc = APR_EAGAIN; if (*len > 0) { @@ -739,6 +753,11 @@ static apr_status_t ssl_io_input_read(bio_filter_in_ctx_t *inctx, if (inctx->block == APR_NONBLOCK_READ) { break; } + if (timedout) { + /* The wait is over, so do not keep retrying it. */ + inctx->rc = APR_TIMEUP; + break; + } continue; /* Blocking and nothing yet? Try again. */ } else if (ssl_err == SSL_ERROR_SYSCALL) { @@ -1431,13 +1450,20 @@ static apr_status_t ssl_io_filter_handshake(ssl_filter_ctx_t *filter_ctx) "SSL handshake stopped: connection was closed"); } else if (ssl_err == SSL_ERROR_WANT_READ) { - /* - * This is in addition to what was present earlier. It is - * borrowed from openssl_state_machine.c [mod_tls]. - * TBD. - */ - outctx->rc = APR_EAGAIN; - return APR_EAGAIN; + if (!APR_STATUS_IS_TIMEUP(inctx->rc)) { + /* + * This is in addition to what was present earlier. It is + * borrowed from openssl_state_machine.c [mod_tls]. + * TBD. + */ + outctx->rc = APR_EAGAIN; + return APR_EAGAIN; + } + /* A read timeout is reported as a retryable read to keep the + * SSL session usable, but the wait is over, so fail the + * handshake here rather than retrying it. */ + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, rc, c, + "SSL handshake timed out"); } else if (ERR_GET_LIB(ERR_peek_error()) == ERR_LIB_SSL && ERR_GET_REASON(ERR_peek_error()) == SSL_R_HTTP_REQUEST) { diff --git a/modules/ssl/ssl_engine_kernel.c b/modules/ssl/ssl_engine_kernel.c index 6e554b0d2b1..ddc9ab18b45 100644 --- a/modules/ssl/ssl_engine_kernel.c +++ b/modules/ssl/ssl_engine_kernel.c @@ -1057,6 +1057,26 @@ static int ssl_hook_Access_modern(request_rec *r, SSLSrvConfigRec *sc, SSLDirCon } #endif +/* Shut the TLS layer of connection C down cleanly and suppress any further + * output. Passing an EOC bucket makes the I/O filter send a close_notify + * alert; this is used where a request has failed because the connection + * itself has failed, so that the client sees the TLS session being closed + * rather than a truncated connection. */ +static void ssl_shutdown_connection(conn_rec *c) +{ + apr_bucket_brigade *bb = apr_brigade_create(c->pool, c->bucket_alloc); + + APR_BRIGADE_INSERT_TAIL(bb, apr_bucket_flush_create(c->bucket_alloc)); + APR_BRIGADE_INSERT_TAIL(bb, ap_bucket_eoc_create(c->bucket_alloc)); + ap_pass_brigade(c->output_filters, bb); + apr_brigade_destroy(bb); + + /* The TLS session is gone, so anything written from here on would be + * sent in the clear. */ + c->keepalive = AP_CONN_CLOSE; + c->aborted = 1; +} + int ssl_hook_Access(request_rec *r) { SSLDirConfigRec *dc = myDirConfig(r); @@ -1120,6 +1140,14 @@ int ssl_hook_Access(request_rec *r) } if (ret != DECLINED) { + /* A read timeout during the renegotiation or post-handshake + * authentication above leaves the connection unusable for the error + * response, so no 403 can be sent. Shut the TLS session down here, + * while that is still possible, rather than leaving the client to + * see the connection simply disappear. */ + if (sslconn->read_timedout && !r->connection->master) { + ssl_shutdown_connection(r->connection); + } return ret; } diff --git a/modules/ssl/ssl_private.h b/modules/ssl/ssl_private.h index 16014186e87..463b642b186 100644 --- a/modules/ssl/ssl_private.h +++ b/modules/ssl/ssl_private.h @@ -649,6 +649,7 @@ typedef struct { const char *verify_error; int verify_depth; int disabled; + int read_timedout; /* a read from the client hit the timeout */ enum { NON_SSL_OK = 0, /* is SSL request, or error handling completed */ NON_SSL_SEND_REQLINE, /* Need to send the fake request line */ diff --git a/test/modules/ssl/test_004_reqtimeout.py b/test/modules/ssl/test_004_reqtimeout.py new file mode 100644 index 00000000000..bd02eb6fba5 --- /dev/null +++ b/test/modules/ssl/test_004_reqtimeout.py @@ -0,0 +1,252 @@ +import os +import select +import socket +import time + +import pytest +from cryptography.hazmat.bindings.openssl.binding import Binding +from OpenSSL import SSL + +from pyhttpd.certs import CertificateSpec +from pyhttpd.conf import HttpdConf + +# mod_reqtimeout's "handshake" stage bounds how long a client may take over a +# TLS handshake. A client certificate requested for a arrives in a +# second handshake - a renegotiation below TLSv1.3, Post-Handshake +# Authentication at TLSv1.3 - where a client can stall just as easily. +# +# The two timeouts are set far apart so that which one fired is never in +# doubt: the stage is much shorter than the core Timeout backstopping it. +HANDSHAKE_TIMEOUT = 2 +CORE_TIMEOUT = 6 +CAP = CORE_TIMEOUT + 6 + +VERSIONS = {"TLSv1.2": SSL.TLS1_2_VERSION, "TLSv1.3": SSL.TLS1_3_VERSION} + +# What the virtual host asks for, which decides whether a certificate has +# already been collected by the time the asks for one. +VHOSTS = {"unset": "", "optional": "SSLVerifyClient optional"} + +# pyOpenSSL exposes no post-handshake auth setting, so reach the one call +# needed through the same OpenSSL bindings pyhttpd already uses for its CA. +_LIB = Binding().lib + +RRT = f"""RequestReadTimeout handshake={HANDSHAKE_TIMEOUT} header=8 body=8 + Timeout {CORE_TIMEOUT}""" + + +class TestReqTimeout: + + @pytest.fixture(autouse=True, scope='class') + def _class_scope(self, env): + doc_dir = os.path.join(env.server_dir, "htdocs", "test1", "secure") + os.makedirs(doc_dir, exist_ok=True) + with open(os.path.join(doc_dir, "index.html"), "w") as f: + f.write("secret\n") + env.httpd_error_log.add_ignored_lognos( + ["AH01991", "AH01992", "AH02261", "AH02262", "AH02263", + "AH10158", "AH10373"]) + env.httpd_error_log.add_ignored_matches([ + r'.*SSL Library Error.*', + r'.*certificate verify failed.*', + ]) + + def install(self, env, proto, vhost_verify=""): + conf = HttpdConf(env, extras={ + "base": RRT, + f"test1.{env.http_tld}": f""" + SSLProtocol -all +{proto} + SSLCACertificateFile "{env.ca.cert_file}" + {vhost_verify} + + SSLVerifyClient require + + """, + }) + conf.add_vhost_test1() + conf.install() + assert env.apache_restart() == 0 + + def connect(self, env): + sock = socket.create_connection(("127.0.0.1", env.https_port), + timeout=CAP) + sock.settimeout(None) # pyOpenSSL wants a blocking socket + return sock + + def context(self, env, proto, creds=None): + ctx = SSL.Context(SSL.TLS_CLIENT_METHOD) + ctx.set_min_proto_version(VERSIONS[proto]) + ctx.set_max_proto_version(VERSIONS[proto]) + ctx.set_verify(SSL.VERIFY_NONE) + if creds: + ctx.use_certificate_file(creds.cert_file) + ctx.use_privatekey_file(creds.pkey_file) + # offer RFC 8446 post_handshake_auth, or the server cannot ask for a + # certificate at all on TLSv1.3 and there is nothing to stall over + _LIB.SSL_CTX_set_post_handshake_auth(ctx._context, 1) + return ctx + + def stalled_request(self, env, proto, vhost_verify=""): + """Handshake, ask for a resource needing a client certificate, then + stop reading - so the server's request for one is never answered.""" + self.install(env, proto, vhost_verify) + creds = env.ca.issue_cert( + CertificateSpec(name="reqtimeout-client", client=True)) + sock = self.connect(env) + conn = SSL.Connection(self.context(env, proto, creds), sock) + conn.set_tlsext_host_name(f"test1.{env.http_tld}".encode()) + conn.set_connect_state() + conn.do_handshake() + conn.sendall(f"GET /secure/index.html HTTP/1.1\r\n" + f"Host: test1.{env.http_tld}\r\n\r\n".encode()) + return conn, sock + + def close_delay(self, sock, cap=CAP): + """Seconds until the server closes, read at the socket so that the + TLS layer never answers anything and ends the stall by accident. + + Read with the socket rather than os.read(), which cannot take a + socket handle on Windows and fails there at once.""" + start = time.monotonic() + while time.monotonic() - start < cap: + if not select.select([sock], [], [], 0.25)[0]: + continue + try: + if sock.recv(65536) == b"": + return time.monotonic() - start + except OSError: + return time.monotonic() - start + return None + + def sent_then_closed(self, sock, cap=CAP): + """(bytes the server sent, whether it then closed the connection). + + Read at the socket, so nothing here can answer the server and end the + stall by accident.""" + total = 0 + deadline = time.monotonic() + cap + while time.monotonic() < deadline: + if not select.select([sock], [], [], 0.25)[0]: + continue + try: + data = sock.recv(65536) + except OSError: + return total, True + if data == b"": + return total, True + total += len(data) + return total, False + + def close_notify_then_eof(self, conn, sock, cap=CAP): + """Drain at the TLS layer after a timeout has fired, reporting + whether the server sent close_notify - which pyOpenSSL surfaces as + ZeroReturnError - and whether the TCP connection then went away.""" + saw_close_notify = False + ending = None + deadline = time.monotonic() + cap + while time.monotonic() < deadline: + if not select.select([sock], [], [], 0.5)[0]: + continue + try: + if conn.recv(16384) == b"": + ending = "eof" + break + except SSL.ZeroReturnError: + saw_close_notify = True + break + except Exception as exc: + ending = f"{type(exc).__name__}: {exc}" + break + # the TLS shutdown should be followed by the connection closing + tcp_closed = False + deadline = time.monotonic() + 5 + while time.monotonic() < deadline: + if not select.select([sock], [], [], 0.25)[0]: + continue + try: + if sock.recv(65536) == b"": + tcp_closed = True + except OSError: + tcp_closed = True + break + return saw_close_notify, tcp_closed, ending + + # -- timing ----------------------------------------------------------- + + # A client stalling in the first handshake is bounded by the stage. + def test_ssl_004_01(self, env): + self.install(env, "TLSv1.3") + sock = self.connect(env) + # Something must be sent or TCP_DEFER_ACCEPT leaves the connection + # unaccepted, and the kernel rather than httpd decides when it ends. + sock.sendall(bytes([0x16, 0x03, 0x01, 0x00, 0x50])) + delay = self.close_delay(sock) + sock.close() + assert delay is not None, "connection was never closed" + assert delay < HANDSHAKE_TIMEOUT + 2, \ + f"closed after {delay:.1f}s, expected the handshake stage " \ + f"({HANDSHAKE_TIMEOUT}s) not Timeout ({CORE_TIMEOUT}s)" + + # Stalling over a certificate requested for a is bounded by no + # RequestReadTimeout stage at all, only by the core Timeout. + @pytest.mark.xfail(strict=True, reason="no RequestReadTimeout stage " + "applies to a renegotiation or post-handshake auth") + @pytest.mark.parametrize("vhost", list(VHOSTS), ids=list(VHOSTS)) + @pytest.mark.parametrize("proto", list(VERSIONS)) + def test_ssl_004_02(self, env, proto, vhost): + conn, sock = self.stalled_request(env, proto, VHOSTS[vhost]) + delay = self.close_delay(sock) + sock.close() + assert delay is not None, "connection was never closed" + assert delay < HANDSHAKE_TIMEOUT + 2, \ + f"closed after {delay:.1f}s, expected the handshake stage " \ + f"({HANDSHAKE_TIMEOUT}s) not Timeout ({CORE_TIMEOUT}s)" + + # -- what reaches the client ----------------------------------------- + + # What matters on a timeout is that the connection is closed, so that a + # client is not left waiting on a server which has already given up. + # Whether anything precedes the close - an HTTP error, a TLS alert, or + # nothing at all - is left to the server; the counts below are reported + # only to make a change in that behaviour visible. + def test_ssl_004_04(self, env): + self.install(env, "TLSv1.3") + sock = self.connect(env) + sock.sendall(bytes([0x16, 0x03, 0x01, 0x00, 0x50])) + sent, closed = self.sent_then_closed(sock) + sock.close() + assert closed, \ + f"connection still open after {CAP}s ({sent} bytes received)" + + @pytest.mark.parametrize("vhost", list(VHOSTS), ids=list(VHOSTS)) + @pytest.mark.parametrize("proto", list(VERSIONS)) + def test_ssl_004_05(self, env, proto, vhost): + conn, sock = self.stalled_request(env, proto, VHOSTS[vhost]) + start = time.monotonic() + sent, closed = self.sent_then_closed(sock) + elapsed = time.monotonic() - start + try: + sock.close() + except OSError: + pass + assert closed, \ + f"connection still open after {CAP}s ({sent} bytes received)" + assert elapsed < CORE_TIMEOUT + 3, \ + f"closed after {elapsed:.1f}s, later than Timeout ({CORE_TIMEOUT}s)" + + # A timeout in the second handshake should be an orderly TLS shutdown: + # close_notify, and then the connection goes away. Without it a client + # cannot distinguish the server giving up from the connection being cut. + @pytest.mark.parametrize("vhost", list(VHOSTS), ids=list(VHOSTS)) + @pytest.mark.parametrize("proto", list(VERSIONS)) + def test_ssl_004_06(self, env, proto, vhost): + conn, sock = self.stalled_request(env, proto, VHOSTS[vhost]) + time.sleep(CORE_TIMEOUT + 1) # let the timeout fire + notified, tcp_closed, ending = self.close_notify_then_eof(conn, sock) + try: + sock.close() + except OSError: + pass + assert notified, \ + f"no close_notify from the server, stream ended with: {ending}" + assert tcp_closed, "close_notify sent but the connection stayed open"