From e9f28bf9d6388de4f7292872659828a7a60cf5c6 Mon Sep 17 00:00:00 2001 From: Andreas Erz Date: Mon, 10 Aug 2026 15:17:02 +0000 Subject: [PATCH 1/2] Add TCP short-write regression test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an LD_PRELOAD shim (short_write_shim.c) that intercepts send() and injects a short write on P_DATA_V1/V2 packets, and a test script (t_short_write.sh) that confirms stream corruption results. Currently FAILs — the bug is present. Linux-only; exits 77 (SKIP) elsewhere. Wires the shim and script into tests/Makefile.am under a TARGET_LINUX guard; adds that conditional to configure.ac. --- configure.ac | 1 + tests/Makefile.am | 13 +++- tests/short_write_shim.c | 44 +++++++++++ tests/t_short_write.sh | 153 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 210 insertions(+), 1 deletion(-) create mode 100644 tests/short_write_shim.c create mode 100755 tests/t_short_write.sh diff --git a/configure.ac b/configure.ac index 469a475b9aa..5a420e33a92 100644 --- a/configure.ac +++ b/configure.ac @@ -376,6 +376,7 @@ case "$host" in esac AM_CONDITIONAL([CROSS_COMPILING], test "${cross_compiling}" = "yes") +AM_CONDITIONAL([TARGET_LINUX], [case $host in *-*-linux*) true;; *) false;; esac]) PKG_PROG_PKG_CONFIG # Add variable to print if pkg-config is found or not. Users often miss that diff --git a/tests/Makefile.am b/tests/Makefile.am index 390796501e8..a07535171d5 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -21,12 +21,21 @@ if !WIN32 test_scripts = t_client.sh t_lpback.sh t_cltsrv.sh t_server_null.sh check_PROGRAMS = ntlm_support + +if TARGET_LINUX +test_scripts += t_short_write.sh +check_LTLIBRARIES = short_write_shim.la +short_write_shim_la_SOURCES = short_write_shim.c +short_write_shim_la_LDFLAGS = -module -shared -avoid-version -no-undefined -rpath $(abs_builddir) +short_write_shim_la_LIBADD = -ldl +endif + if HAVE_SITNL test_scripts += t_net.sh endif endif -TESTS_ENVIRONMENT = top_srcdir="$(top_srcdir)" +TESTS_ENVIRONMENT = top_srcdir="$(top_srcdir)" top_builddir="$(top_builddir)" TEST_EXTENSIONS = .sh TESTS = $(test_scripts) @@ -39,9 +48,11 @@ dist_noinst_SCRIPTS = \ t_server_null_client.sh \ t_server_null_server.sh \ t_server_null_default.rc \ + t_short_write.sh \ update_t_client_ips.sh t_client.log: t_server_null.log +t_short_write.log: short_write_shim.la dist_noinst_DATA = \ t_client.rc-sample diff --git a/tests/short_write_shim.c b/tests/short_write_shim.c new file mode 100644 index 00000000000..3ac02ee76c3 --- /dev/null +++ b/tests/short_write_shim.c @@ -0,0 +1,44 @@ +/* + * short_write_shim.c -- LD_PRELOAD shim that injects short writes into send() + * Intercepts TCP data-channel packets only, letting the + * TLS handshake complete before corrupting the stream. + */ + +#define _GNU_SOURCE +#include +#include + +/* Opcodes from ssl_pkt.h -- high 5 bits of the opcode byte */ +#define P_OPCODE_SHIFT 3 +#define P_DATA_V1 6 +#define P_DATA_V2 9 + +static ssize_t (*real_send)(int, const void *, size_t, int); + +static void __attribute__((constructor)) +shim_init(void) +{ + real_send = dlsym(RTLD_NEXT, "send"); +} + +ssize_t +send(int fd, const void *buf, size_t len, int flags) +{ + /* + * Over TCP, OpenVPN prepends a 2-byte big-endian length header to every + * packet. Byte 2 (index 2) carries the opcode in its high 5 bits. + * We need at least 3 bytes to inspect the opcode. + */ + if (len >= 3) + { + const unsigned char *p = (const unsigned char *)buf; + int opcode = (p[2] >> P_OPCODE_SHIFT); + + if (opcode == P_DATA_V1 || opcode == P_DATA_V2) + { + return real_send(fd, buf, len / 2, flags); + } + } + + return real_send(fd, buf, len, flags); +} diff --git a/tests/t_short_write.sh b/tests/t_short_write.sh new file mode 100755 index 00000000000..103837521a1 --- /dev/null +++ b/tests/t_short_write.sh @@ -0,0 +1,153 @@ +#!/bin/sh +# +# t_short_write.sh - regression test for TCP short-write stream corruption +# +# An LD_PRELOAD shim halves every data-channel send(). If the send path does +# not retry, forward.c logs a truncation warning and the stream is corrupted. +# +# Exit 0 (PASS): no truncation — short writes handled correctly +# Exit 1 (FAIL): truncation warning found — bug is present +# Exit 77 (SKIP): prerequisites missing or unsupported platform +# + +set -e + +top_srcdir="${top_srcdir:-..}" +top_builddir="${top_builddir:-..}" +openvpn="${openvpn:-${top_builddir}/src/openvpn/openvpn}" +shim="${shim:-$(cd "${top_builddir}/tests" && pwd)/.libs/short_write_shim.so}" + +# LD_PRELOAD interposition requires a Linux ELF dynamic linker. +case $(uname -s) in + Linux) ;; + *) echo "$0: LD_PRELOAD shim requires Linux. SKIP." >&2; exit 77 ;; +esac + +test -x "${openvpn}" || { echo "$0: openvpn binary not found. SKIP." >&2; exit 77; } +test -f "${shim}" || { echo "$0: short_write_shim.so not built. SKIP." >&2; exit 77; } + +# Port chosen to avoid the loopback-{server,client} sample configs (16000/16001). +SRV_PORT="${SRV_PORT:-16002}" + +root="${top_srcdir}/sample" +WORKDIR="$(pwd)" +SRV_LOG="${WORKDIR}/sw_srv_$$.log" +CLI_LOG="${WORKDIR}/sw_cli_$$.log" +SRV_PID="${WORKDIR}/sw_srv_$$.pid" + +cleanup() { + if [ -n "${CLI_PID}" ]; then + kill "${CLI_PID}" 2>/dev/null || true + fi + if [ -f "${SRV_PID}" ]; then + pid=$(cat "${SRV_PID}" 2>/dev/null) + [ -n "${pid}" ] && kill "${pid}" 2>/dev/null || true + fi + rm -f "${SRV_LOG}" "${CLI_LOG}" "${SRV_PID}" +} + +CLI_PID="" + +trap "cleanup; trap 0; exit 77" 1 2 15 +trap "cleanup; exit 1" 0 3 + +# Poll $CLI_LOG for regex $1 for up to $2 seconds. +wait_for() { + _pat=$1; _tries=$2 + while [ "${_tries}" -gt 0 ]; do + grep -qE "${_pat}" "${CLI_LOG}" 2>/dev/null && return 0 + [ -n "${CLI_PID}" ] && ! kill -0 "${CLI_PID}" 2>/dev/null && return 1 + sleep 1 + _tries=$((_tries - 1)) + done + return 1 +} + +success=0 +for i in 1 2 3; do + set +e + + "${openvpn}" \ + --cd "${root}" --dev null --proto tcp-server \ + --local 127.0.0.1 --lport "${SRV_PORT}" \ + --tls-server --dh none \ + --ca sample-keys/ca.crt \ + --key sample-keys/server.key \ + --cert sample-keys/server.crt \ + --cipher AES-256-GCM --verb 3 \ + --writepid "${SRV_PID}" --log "${SRV_LOG}" --daemon + + j=0 + while [ $j -lt 10 ] && [ ! -s "${SRV_PID}" ]; do + sleep 1; j=$((j+1)) + done + + if [ ! -s "${SRV_PID}" ]; then + if grep -q 'TCP/UDP: Socket bind failed on local address.*in use' \ + "${SRV_LOG}" 2>/dev/null; then + echo "$0: port ${SRV_PORT} in use, retrying in 10 s" >&2 + rm -f "${SRV_PID}" "${SRV_LOG}" + sleep 10 + continue + fi + echo "$0: server did not start" >&2 + cat "${SRV_LOG}" >&2 + exit 1 + fi + + # The client never exits on its own: bug present → endless reconnect loop; + # bug fixed → healthy tunnel forever. Poll the log and kill when done. + rm -f "${CLI_LOG}" + LD_PRELOAD="${shim}" \ + "${openvpn}" \ + --cd "${root}" --dev null --proto tcp-client \ + --remote 127.0.0.1 "${SRV_PORT}" \ + --tls-client --remote-cert-tls server \ + --ca sample-keys/ca.crt \ + --key sample-keys/client.key \ + --cert sample-keys/client.crt \ + --cipher AES-256-GCM \ + --ping 1 --ping-exit 30 \ + --verb 3 --log "${CLI_LOG}" & + CLI_PID=$! + + if ! wait_for "P2P mode NCP negotiation result" 15; then + echo "$0: tunnel did not establish, retrying" >&2 + kill "${CLI_PID}" 2>/dev/null; wait "${CLI_PID}" 2>/dev/null + CLI_PID="" + kill "$(cat ${SRV_PID})" 2>/dev/null || true + rm -f "${SRV_PID}" "${SRV_LOG}" "${CLI_LOG}" + continue + fi + + set -e + success=1 + break +done + +if [ $success -ne 1 ]; then + echo "$0: could not establish a tunnel after 3 attempts. SKIP." >&2 + trap 0; cleanup; exit 77 +fi + +# Tunnel is up. The first ping (~1 s) triggers the truncation warning if the +# bug is present. Wait a few ping cycles for it to surface. +set +e +wait_for "TCP/UDP packet was truncated/expanded on write" 5 +set -e + +kill "${CLI_PID}" 2>/dev/null || true +wait "${CLI_PID}" 2>/dev/null || true +CLI_PID="" + +ec=0 +if grep -q "TCP/UDP packet was truncated/expanded on write" "${CLI_LOG}"; then + echo "$0: FAIL: short-write caused stream truncation" >&2 + ec=1 +else + echo "$0: PASS: no truncation detected" >&2 +fi + +cleanup +trap 0 +exit $ec From 0200d145f05d1bf79a8d08bdaf677703383f3297 Mon Sep 17 00:00:00 2001 From: Andreas Erz Date: Tue, 11 Aug 2026 11:28:59 +0000 Subject: [PATCH 2/2] Resume partial TCP writes A short write on a TCP socket left the stream corrupt. Track partial writes and retry until the packet drains. --- src/openvpn/forward.c | 47 +++++++++++++++++++++++++++++++++++-------- src/openvpn/socket.c | 18 ++++++++++------- src/openvpn/socket.h | 1 + 3 files changed, 51 insertions(+), 15 deletions(-) diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 46e1a53db99..24df50c8530 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -1748,7 +1748,11 @@ process_outgoing_link(struct context *c, struct link_socket *sock) struct gc_arena gc = gc_new(); int error_code = 0; - if (c->c2.to_link.len > 0 && c->c2.to_link.len <= c->c2.frame.buf.payload_size) + /* For a partial TCP write, to_link.len includes the prepended size header, + so it can be greater than the payload size. */ + if (c->c2.to_link.len > 0 + && (c->c2.to_link.len <= c->c2.frame.buf.payload_size + || sock->stream_partial_write)) { /* * Setup for call to send/sendto which will send @@ -1766,7 +1770,7 @@ process_outgoing_link(struct context *c, struct link_socket *sock) * Let the traffic shaper know how many bytes * we wrote. */ - if (c->options.shaper) + if (c->options.shaper && !sock->stream_partial_write) { int overhead = datagram_overhead(c->c2.to_link_addr->dest.addr.sa.sa_family, sock->info.proto); @@ -1788,7 +1792,7 @@ process_outgoing_link(struct context *c, struct link_socket *sock) /* Log packet send */ #ifdef LOG_RW - if (c->c2.log_rw) + if (c->c2.log_rw && !sock->stream_partial_write) { fprintf(stderr, "W"); } @@ -1824,19 +1828,43 @@ process_outgoing_link(struct context *c, struct link_socket *sock) error_code = openvpn_errno(); check_status(size, "write", sock, NULL); - if (size > 0) + if (proto_is_tcp(sock->info.proto) && !socket_is_dco_win(sock)) + { + if (size == BLEN(&c->c2.to_link)) + { + /* complete write */ + sock->stream_partial_write = false; + } + else if (size > 0 && size < BLEN(&c->c2.to_link)) + { + /* partial write */ + buf_advance(&c->c2.to_link, size); + sock->stream_partial_write = true; + } + else if (size < 0 && (error_code == EAGAIN || error_code == EWOULDBLOCK)) + { + /* 0 bytes written, but header might have been prepended */ + sock->stream_partial_write = true; + } + else + { + /* error */ + sock->stream_partial_write = false; + } + } + else { /* Did we write a different size packet than we intended? */ - if (size != BLEN(&c->c2.to_link)) + if (size > 0 && size != BLEN(&c->c2.to_link)) { msg(D_LINK_ERRORS, - "TCP/UDP packet was truncated/expanded on write to %s (tried=%d,actual=%d)", + "Packet was truncated/expanded on write to %s (tried=%d,actual=%d)", print_link_socket_actual(c->c2.to_link_addr, &gc), BLEN(&c->c2.to_link), size); } } /* if not a ping/control message, indicate activity regarding --inactive parameter */ - if (c->c2.buf.len > 0) + if (c->c2.buf.len > 0 && size > 0) { register_activity(c, size); } @@ -1867,7 +1895,10 @@ process_outgoing_link(struct context *c, struct link_socket *sock) } } - buf_reset(&c->c2.to_link); + if (!sock->stream_partial_write) + { + buf_reset(&c->c2.to_link); + } gc_free(&gc); } diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c index 0f66ad513fe..57ebfcd9026 100644 --- a/src/openvpn/socket.c +++ b/src/openvpn/socket.c @@ -2439,13 +2439,17 @@ link_socket_read_udp_posix(struct link_socket *sock, struct buffer *buf, ssize_t link_socket_write_tcp(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *to) { - const int blen = BLEN(buf); - ASSERT(blen >= 0 && blen <= PACKET_SIZE_MAX); - packet_size_type len = (packet_size_type)blen; - dmsg(D_STREAM_DEBUG, "STREAM: WRITE %u offset=%d", len, buf->offset); - ASSERT(len <= sock->stream_buf.maxlen); - len = htonps(len); - ASSERT(buf_write_prepend(buf, &len, sizeof(len))); + if (!sock->stream_partial_write) + { + /* prepend the length of the packet to the buffer */ + const int blen = BLEN(buf); + ASSERT(blen >= 0 && blen <= PACKET_SIZE_MAX); + packet_size_type len = (packet_size_type)blen; + dmsg(D_STREAM_DEBUG, "STREAM: WRITE %u offset=%d", len, buf->offset); + ASSERT(len <= sock->stream_buf.maxlen); + len = htonps(len); + ASSERT(buf_write_prepend(buf, &len, sizeof(len))); + } #ifdef _WIN32 return link_socket_write_win32(sock, buf, to); #else diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h index 1a532e14aea..d3dad535022 100644 --- a/src/openvpn/socket.h +++ b/src/openvpn/socket.h @@ -223,6 +223,7 @@ struct link_socket struct stream_buf stream_buf; struct buffer stream_buf_data; bool stream_reset; + bool stream_partial_write; /* a partial write is outstanding */ /* HTTP proxy */ struct http_proxy_info *http_proxy;