From 01f2787ab6c3f7dd96c022409414bd8a834907cd Mon Sep 17 00:00:00 2001 From: Nikos Mavrogiannopoulos Date: Thu, 14 May 2026 09:29:34 +0200 Subject: [PATCH] worker: reject HTTP pipelining to prevent request confusion When two HTTP requests arrived in the same TLS read buffer, a single llhttp_execute() call would fire callbacks for both requests inline. Because http_req_reset() is not called between them, ws->req ended up reflecting the second request's URL and headers, silently discarding the first. In the worst case, body bytes from the first request accumulated alongside the second request's body. Fix this by registering an on_message_begin callback that returns HPE_PAUSED when an existing message is detected. Resolves: #716 Signed-off-by: Nikos Mavrogiannopoulos --- NEWS | 2 + src/worker-http.c | 13 +++++ src/worker-vpn.c | 11 +++- src/worker.h | 1 + tests/meson.build | 1 + tests/test-http-smuggling | 111 ++++++++++++++++++++++++++++++++++++++ 6 files changed, 138 insertions(+), 1 deletion(-) create mode 100755 tests/test-http-smuggling diff --git a/NEWS b/NEWS index 3769c73f..b9e45cc3 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) diff --git a/src/worker-http.c b/src/worker-http.c index fee47cb3..ba631b52 100644 --- a/src/worker-http.c +++ b/src/worker-http.c @@ -919,6 +919,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 b9d0ead7..25199276 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 ce42d45e..cbda3636 100644 --- a/src/worker.h +++ b/src/worker.h @@ -373,6 +373,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 778dafe5..a9445578 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