diff --git a/NEWS b/NEWS index ca879ccf..e2ba69ef 100644 --- a/NEWS +++ b/NEWS @@ -12,6 +12,8 @@ option (enabled by default) - Limited HTTP header size to 16 KB to complement the existing 256 KB body limit +- Fixed HTTP request desynchronization: pipelined requests in the same + TLS record are now rejected instead of being processed out of order (#716) - Fix build issue on FreeBSD (#704) - Build against Nettle 4. - Removed unnecessary for openconnect client handlers to reduce attack surface: diff --git a/src/worker-http.c b/src/worker-http.c index ed08760e..e8c9280d 100644 --- a/src/worker-http.c +++ b/src/worker-http.c @@ -915,6 +915,19 @@ int http_message_complete_cb(llhttp_t *parser) return 0; } +int http_message_begin_cb(llhttp_t *parser) +{ + struct worker_st *ws = parser->data; + struct http_req_st *req = &ws->req; + + /* headers_complete is 0 at the start of each keep-alive cycle + * (http_req_reset() clears it). If it is already 1 here a second + * message has started before the first was dispatched — reject. */ + if (req->headers_complete) + return HPE_PAUSED; + return 0; +} + int http_body_cb(llhttp_t *parser, const char *at, size_t length) { struct worker_st *ws = parser->data; diff --git a/src/worker-vpn.c b/src/worker-vpn.c index 04796a21..5b22fbe2 100644 --- a/src/worker-vpn.c +++ b/src/worker-vpn.c @@ -964,6 +964,7 @@ void vpn_server(struct worker_st *ws) if (ws->cert_auth_ok) ws_switch_auth_to(ws, AUTH_TYPE_CERTIFICATE); + settings.on_message_begin = http_message_begin_cb; settings.on_url = http_url_cb; settings.on_header_field = http_header_field_cb; settings.on_header_value = http_header_value_cb; @@ -1005,6 +1006,10 @@ restart: parser.method == HTTP_CONNECT) { llhttp_resume_after_upgrade(&parser); break; + } else if (lerr == HPE_PAUSED) { + oclog(ws, LOG_INFO, + "closing connection: unexpected pipelined data"); + exit_worker(ws); } else if (lerr != HPE_OK) { oclog(ws, LOG_INFO, "error parsing HTTP request: %s", llhttp_errno_name(lerr)); @@ -1056,7 +1061,11 @@ restart: lerr = llhttp_execute(&parser, (void *)ws->buffer, nrecvd); - if (lerr != HPE_OK) { + if (lerr == HPE_PAUSED) { + oclog(ws, LOG_HTTP_DEBUG, + "closing connection: unexpected pipelined data"); + exit_worker(ws); + } else if (lerr != HPE_OK) { oclog(ws, LOG_HTTP_DEBUG, "error parsing HTTP POST request: %s", llhttp_errno_name(lerr)); diff --git a/src/worker.h b/src/worker.h index 11f6a8a6..e5459261 100644 --- a/src/worker.h +++ b/src/worker.h @@ -369,6 +369,7 @@ int http_header_value_cb(llhttp_t *parser, const char *at, size_t length); int http_header_field_cb(llhttp_t *parser, const char *at, size_t length); int http_header_complete_cb(llhttp_t *parser); int http_message_complete_cb(llhttp_t *parser); +int http_message_begin_cb(llhttp_t *parser); int http_body_cb(llhttp_t *parser, const char *at, size_t length); void http_req_deinit(worker_st *ws); void http_req_reset(worker_st *ws); diff --git a/tests/meson.build b/tests/meson.build index 447bd366..ab539480 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -97,6 +97,7 @@ always_scripts = [ 'test-replay', 'test-fw-normalize-route', 'test-http-limits', + 'test-http-smuggling', ] foreach s : always_scripts diff --git a/tests/test-http-smuggling b/tests/test-http-smuggling new file mode 100755 index 00000000..5f7afaa4 --- /dev/null +++ b/tests/test-http-smuggling @@ -0,0 +1,111 @@ +#!/bin/sh +# +# Copyright (C) 2026 Nikos Mavrogiannopoulos +# +# This file is part of ocserv. +# +# ocserv is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by the +# Free Software Foundation; either version 2 of the License, or (at +# your option) any later version. +# +# ocserv is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with GnuTLS; if not, write to the Free Software Foundation, +# Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + +# Verify that HTTP request-boundary confusion is not possible. +# When two requests arrive in the same TLS read buffer, the parser must +# stop at the first message_complete boundary (via llhttp_pause) and +# dispatch only the first request. The second request must not reach +# any handler in that same keep-alive cycle. + +SERV="${SERV:-../src/ocserv}" +srcdir=${srcdir:-.} +NO_NEED_ROOT=1 + +. `dirname $0`/common.sh + +eval "${GETPORT}" + +if ! which gnutls-cli > /dev/null 2>&1; then + echo "gnutls-cli not found, needed for this test" + exit 1 +fi + +echo "Testing HTTP request smuggling prevention..." + +update_config test-user-cert.config +launch_simple_sr_server -d 1 -f -c ${CONFIG} +PID=$! +wait_server $PID + +TMPFILE=test-http-smuggling.$$.tmp + +# Send raw bytes over TLS via gnutls-cli and capture the full server +# response. Because stdin is a pipe (not a terminal) gnutls-cli delivers +# all data to gnutls_record_send() in a single call, placing both requests +# in the same TLS record — the exact scenario that triggers the bug. +# tls_send takes a printf(1) format string with \r\n escape sequences and +# sends the result over TLS via gnutls-cli. The argument is intentionally +# NOT passed through $() to avoid shell command substitution stripping +# trailing newlines, which would drop the blank line that terminates HTTP +# headers. +# +# The "sleep 1" keeps the pipe open after the request bytes are sent so +# that gnutls-cli stays in its select loop long enough to receive the +# server response before stdin EOF triggers a TLS close_notify. +tls_send() { + { printf '%b' "$1"; sleep 1; } | \ + LD_PRELOAD=libsocket_wrapper.so timeout 5 \ + gnutls-cli \ + --insecure --x509cafile="${srcdir}/certs/ca.pem" \ + $ADDRESS --port $PORT \ + --sni-hostname localhost > "$TMPFILE" || true +} + +# --- Positive: single GET must still return a response ------------------ +echo -n "Single GET /cert.pem returns a response ... " +tls_send 'GET /cert.pem HTTP/1.1\r\nHost: localhost\r\nConnection: close\r\n\r\n' +grep -q "^HTTP/1\." "$TMPFILE" || + fail $PID "Single GET /cert.pem produced no HTTP response" +echo "ok" + +# --- Negative: second pipelined request must not be dispatched ---------- +# Strategy: second URL is /cert.pem (→ 200). Before the fix the second +# request's URL overwrote the first and the server returned 200. After +# the fix the server detects pipelined data and closes the connection +# immediately — neither request gets a response, but /cert.pem is never +# served. +echo -n "Pipelined GET: /cert.pem not served ... " +tls_send 'GET /BOGUS_SENTINEL HTTP/1.1\r\nHost: localhost\r\nConnection: keep-alive\r\n\r\nGET /cert.pem HTTP/1.1\r\nHost: localhost\r\nConnection: close\r\n\r\n' +if grep -q "^HTTP/1.1 200" "$TMPFILE"; then + fail $PID "Second pipelined request was dispatched: got 200 for /cert.pem" +fi +echo "ok" + +# --- Negative: GET with body bytes + pipelined GET ---------------------- +# The body bytes of the first GET must not bleed into the second request's +# URL parsing. +echo -n "GET with body + pipelined GET: first request dispatched ... " +tls_send 'GET /BOGUS_SENTINEL HTTP/1.1\r\nHost: localhost\r\nContent-Length: 5\r\nConnection: keep-alive\r\n\r\nabcdeGET /cert.pem HTTP/1.1\r\nHost: localhost\r\nConnection: close\r\n\r\n' +if grep -q "^HTTP/1.1 200" "$TMPFILE"; then + fail $PID "Pipelined request after GET body was dispatched: expected 404, got 200 for /cert.pem" +fi +echo "ok" + +# --- Negative: GET with Content-Length: 0 + pipelined GET --------------- +echo -n "GET Content-Length:0 + pipelined GET: first request dispatched ... " +tls_send 'GET /BOGUS_SENTINEL HTTP/1.1\r\nHost: localhost\r\nContent-Length: 0\r\nConnection: keep-alive\r\n\r\nGET /cert.pem HTTP/1.1\r\nHost: localhost\r\nConnection: close\r\n\r\n' +if grep -q "^HTTP/1.1 200" "$TMPFILE"; then + fail $PID "Pipelined request after CL:0 GET was dispatched: expected 404, got 200 for /cert.pem" +fi +echo "ok" + +rm -f "$TMPFILE" +cleanup +exit 0