From a79b5cec4607bf726d1b5143a022daf3cb168bbc Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Fri, 12 Jun 2026 11:36:07 -0500 Subject: [PATCH 1/2] test: surface off-by-one in protocol recv buffers (CFE-4687) ProtocolOpenDir()/ProtocolGet() receive up to CF_MSGSIZE bytes into a char buf[CF_MSGSIZE], but the receive primitives NUL-terminate at buf[received] where received can equal CF_MSGSIZE -- one past the array (TLSRecv/RecvSocketStream both write toget+1 bytes). Drives the real ProtocolOpenDir() over a classic-protocol socketpair with a record-filling reply. Aborts under AddressSanitizer on current code, passes once the buffers are sized CF_BUFSIZE (cfengine/core#6171). Ref: CFE-4687, cfengine/core#6171 Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/unit/Makefile.am | 3 +- tests/unit/protocol_recv_overflow_test.c | 111 +++++++++++++++++++++++ 2 files changed, 113 insertions(+), 1 deletion(-) create mode 100644 tests/unit/protocol_recv_overflow_test.c diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am index 3a4bb0a9d4..8fb3dfb6ea 100644 --- a/tests/unit/Makefile.am +++ b/tests/unit/Makefile.am @@ -145,7 +145,8 @@ check_PROGRAMS = \ policy_server_test \ split_process_line_test \ new_packages_promise_test \ - iteration_test + iteration_test \ + protocol_recv_overflow_test if HAVE_AVAHI_CLIENT if HAVE_AVAHI_COMMON diff --git a/tests/unit/protocol_recv_overflow_test.c b/tests/unit/protocol_recv_overflow_test.c new file mode 100644 index 0000000000..2d2d9945ee --- /dev/null +++ b/tests/unit/protocol_recv_overflow_test.c @@ -0,0 +1,111 @@ +/* + * Regression test for CFE-4687 / cfengine/core PR #6171: + * off-by-one in the ProtocolGet() and ProtocolOpenDir() receive buffers + * in libcfnet/protocol.c. + * + * Background + * ---------- + * The receive primitives NUL-terminate what they read, and the count read + * can equal the requested length: + * + * TLSRecv() tls_generic.c:807 buffer[received] = '\0'; + * RecvSocketStream() classic.c:54 buffer[already] = '\0'; + * + * Both are documented to write up to "toget + 1" bytes (classic.c:5-7), so a + * caller must hand them a buffer of at least toget + 1 bytes. net.c:151 and + * ProtocolStat() (protocol.c:260) honor this with CF_BUFSIZE buffers. + * + * ProtocolOpenDir() and ProtocolGet() did not: they declared + * + * char buf[CF_MSGSIZE]; // CF_MSGSIZE == CF_BUFSIZE - CF_INBAND_OFFSET + * + * and then received up to CF_MSGSIZE bytes into it. A peer that fills the + * record makes "received" reach CF_MSGSIZE, so the terminating NUL is written + * to buf[CF_MSGSIZE] -- one byte past the array. The byte count is peer + * controlled (the transaction length field for OPENDIR, accepted up to + * CF_MSGSIZE at net.c:214), so the write crosses the trust boundary. + * + * What this test does + * ------------------- + * It drives the real ProtocolOpenDir() over a socketpair using the classic + * protocol. The classic data path uses RecvSocketStream(), whose terminator + * write is identical to the TLS path's TLSRecv(), so the same off-by-one is + * surfaced without needing a TLS handshake. A "server" sends a single CF_DONE + * transaction carrying exactly CF_MSGSIZE payload bytes -- the maximum + * ReceiveTransaction() accepts. + * + * On unpatched code the terminating write lands at buf[CF_MSGSIZE] inside + * ProtocolOpenDir()'s stack frame; under AddressSanitizer (the project's + * "ASAN Unit Tests" CI job) this aborts with a stack-buffer-overflow. With the + * fix (buf sized CF_BUFSIZE) the write is in bounds and the test completes. + * + * ProtocolGet() carries the identical defect and the identical one-line fix, + * but receives via TLSRecv() directly, which requires a live SSL session; + * there is no TLS-handshake harness in tests/unit, so it is not driven here. + */ + +#include + +#include +#include /* ProtocolOpenDir */ +#include /* ConnectionInfo* */ +#include /* AgentConnection, CF_MSGSIZE, CF_DONE */ +#include /* SendTransaction */ +#include /* Seq, SeqDestroy */ + +#include +#include +#include + +static void test_opendir_recv_terminator_overflow(void) +{ + int sv[2]; + int ret = socketpair(AF_UNIX, SOCK_STREAM, 0, sv); + assert_int_equal(ret, 0); + + /* --- malicious/non-conforming server side (sv[1]) --- */ + ConnectionInfo *server = ConnectionInfoNew(); + ConnectionInfoSetSocket(server, sv[1]); + ConnectionInfoSetProtocolVersion(server, CF_PROTOCOL_CLASSIC); + + /* A record-filling reply: exactly CF_MSGSIZE payload bytes, the maximum + * ReceiveTransaction() permits (net.c:214). CF_DONE so the client's + * receive loop runs exactly once. Content is irrelevant -- the overflow + * happens during the receive, before any payload is interpreted. */ + char payload[CF_MSGSIZE]; + memset(payload, 'A', sizeof(payload)); + + ret = SendTransaction(server, payload, CF_MSGSIZE, CF_DONE); + assert_int_equal(ret, 0); + + /* --- client side (sv[0]): the real vulnerable function --- */ + AgentConnection conn; + memset(&conn, 0, sizeof(conn)); + conn.conn_info = ConnectionInfoNew(); + ConnectionInfoSetSocket(conn.conn_info, sv[0]); + ConnectionInfoSetProtocolVersion(conn.conn_info, CF_PROTOCOL_CLASSIC); + + /* On unpatched code this overflows buf[CF_MSGSIZE] while receiving and + * AddressSanitizer aborts here. On patched code it returns normally. */ + Seq *result = ProtocolOpenDir(&conn, "/tmp"); + + /* Reaching this point means no out-of-bounds write occurred. */ + SeqDestroy(result); + ConnectionInfoDestroy(&conn.conn_info); + ConnectionInfoDestroy(&server); + close(sv[0]); + close(sv[1]); + + assert_true(true); +} + +int main(void) +{ + PRINT_TEST_BANNER(); + const UnitTest tests[] = + { + unit_test(test_opendir_recv_terminator_overflow), + }; + + return run_tests(tests); +} From 2d5f4ae31db5465fd2c16551eff9c4a0393fcde9 Mon Sep 17 00:00:00 2001 From: aizu-m Date: Tue, 9 Jun 2026 12:15:12 +0530 Subject: [PATCH 2/2] fix off-by-one in ProtocolGet and ProtocolOpenDir recv buffers --- libcfnet/protocol.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libcfnet/protocol.c b/libcfnet/protocol.c index b0f16e6acf..0719af9554 100644 --- a/libcfnet/protocol.c +++ b/libcfnet/protocol.c @@ -38,7 +38,7 @@ Seq *ProtocolOpenDir(AgentConnection *conn, const char *path) assert(conn != NULL); assert(path != NULL); - char buf[CF_MSGSIZE] = {0}; + char buf[CF_BUFSIZE] = {0}; int tosend = snprintf(buf, CF_MSGSIZE, "OPENDIR %s", path); if (tosend < 0 || tosend >= CF_MSGSIZE) { @@ -110,7 +110,7 @@ bool ProtocolGet(AgentConnection *conn, const char *remote_path, return false; } - char buf[CF_MSGSIZE] = {0}; + char buf[CF_BUFSIZE] = {0}; int to_send = snprintf(buf, CF_MSGSIZE, "GET %d %s", CF_MSGSIZE, remote_path);