From d8f94714f055f3b6550de48396df1dff46487fcc Mon Sep 17 00:00:00 2001 From: Dhanraj E Date: Wed, 14 Oct 2026 19:56:37 +0530 Subject: [PATCH] Fixed out-of-bounds read in RemoteDirList when parsing a listing `RemoteDirList` walks the NUL-separated entries of a server reply with `for (sp = recvbuffer; *sp != '\0'; sp += strlen(sp) + 1)`, never looking at the length `ReceiveTransaction` returned, so a reply that does not end in the double `'\0'` carries the scan on into whatever the previous reply left in the buffer, and off the end of it when that residue holds no NUL. a hostile policy server can thus synthesise directory entries out of buffer leftovers, or silently drop the entries of the last packet by leaving a stale `CFD_TERMINATOR` where the scan resumes, which is what the new test does. the encrypted branch discards `DecryptString`'s return as well, so the plaintext is never terminated and the scan runs past that too; both lengths are now honoured, the same way `ProtocolOpenDir` in `libcfnet/protocol.c` already bounds this loop. reached as root from any recursive `copy_from`, through `SourceSearchAndCopy` -> `AbstractDirOpen` -> `RemoteDirList`. Also asserts `conn != NULL` at the top of the function, per the pre-existing CodeQL alerts. Changelog: Title Signed-off-by: Dhanraj E --- libcfnet/client_code.c | 24 ++++++++-- tests/unit/Makefile.am | 1 + tests/unit/client_code_test.c | 87 +++++++++++++++++++++++++++++++++++ 3 files changed, 108 insertions(+), 4 deletions(-) create mode 100644 tests/unit/client_code_test.c diff --git a/libcfnet/client_code.c b/libcfnet/client_code.c index 56935918b92..d5beb0c1ede 100644 --- a/libcfnet/client_code.c +++ b/libcfnet/client_code.c @@ -345,6 +345,8 @@ void DisconnectServer(AgentConnection *conn) * since every directory has to contain at least . and .. */ Item *RemoteDirList(const char *dirname, bool encrypt, AgentConnection *conn) { + assert(conn != NULL); + char sendbuffer[CF_BUFSIZE]; char recvbuffer[CF_BUFSIZE]; char in[CF_BUFSIZE]; @@ -414,8 +416,18 @@ Item *RemoteDirList(const char *dirname, bool encrypt, AgentConnection *conn) if (encrypt) { memcpy(in, recvbuffer, nbytes); - DecryptString(recvbuffer, sizeof(recvbuffer), in, nbytes, - conn->encryption_type, conn->session_key); + nbytes = DecryptString(recvbuffer, sizeof(recvbuffer), in, nbytes, + conn->encryption_type, conn->session_key); + if (nbytes <= 0 || (size_t) nbytes >= sizeof(recvbuffer)) + { + Log(LOG_LEVEL_ERR, + "Failed to decrypt server packet when listing directory '%s'!", + dirname); + goto err; + } + /* ReceiveTransaction() terminates the plaintext packets, this + * terminates the decrypted ones. */ + recvbuffer[nbytes] = '\0'; } if (recvbuffer[0] == '\0') @@ -439,9 +451,13 @@ Item *RemoteDirList(const char *dirname, bool encrypt, AgentConnection *conn) goto err; } - /* Double '\0' means end of packet. */ - for (char *sp = recvbuffer; *sp != '\0'; sp += strlen(sp) + 1) + /* Double '\0' means end of packet, but only the bytes we received + * belong to it. */ + for (int i = 0; i < nbytes && recvbuffer[i] != '\0'; + i += strlen(recvbuffer + i) + 1) { + const char *const sp = recvbuffer + i; + if (strcmp(sp, CFD_TERMINATOR) == 0) /* end of all packets */ { return start; diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am index e2eb77b7098..dca666fd4a8 100644 --- a/tests/unit/Makefile.am +++ b/tests/unit/Makefile.am @@ -156,6 +156,7 @@ check_PROGRAMS = \ new_packages_promise_test \ iteration_test \ protocol_recv_overflow_test \ + client_code_test \ watcher_test \ stoppable_thread_test \ wakeup_channel_test diff --git a/tests/unit/client_code_test.c b/tests/unit/client_code_test.c new file mode 100644 index 00000000000..5c7714044d4 --- /dev/null +++ b/tests/unit/client_code_test.c @@ -0,0 +1,87 @@ +#include + +#include +#include /* RemoteDirList */ +#include /* ConnectionInfo* */ +#include /* AgentConnection, CF_MORE, CF_DONE */ +#include /* SendTransaction */ +#include /* Item, ListLen, DeleteItemList */ +#include /* CFD_TERMINATOR */ + +#include +#include +#include + +#define TERM CFD_TERMINATOR + +/* Entry names are read out of the struct dirent that RemoteDirList() + * allocates for each of them. */ +static const char *EntryName(const Item *ip) +{ + return ((const struct dirent *) ip->name)->d_name; +} + +/* A server reply that does not end in the double '\0' must not make + * RemoteDirList() read the bytes left in the receive buffer by the previous + * reply. Here the stale bytes are a CFD_TERMINATOR, so an unbounded scan ends + * the listing early and silently drops the entries of the last packet. */ +static void test_dirlist_stops_at_end_of_packet(void) +{ + int sv[2]; + int ret = socketpair(AF_UNIX, SOCK_STREAM, 0, sv); + assert_int_equal(ret, 0); + + ConnectionInfo *server = ConnectionInfoNew(); + ConnectionInfoSetSocket(server, sv[1]); + ConnectionInfoSetProtocolVersion(server, CF_PROTOCOL_CLASSIC); + + /* "entry1", end of packet, and a CFD_TERMINATOR that the client must not + * reach because the double '\0' comes first. It stays in the receive + * buffer at offset 8. */ + const char packet1[] = "entry1\0\0" TERM "\0"; + ret = SendTransaction(server, packet1, sizeof(packet1) - 1, CF_MORE); + assert_int_equal(ret, 0); + + /* 7 bytes with no terminating '\0' of their own: the scan resumes at + * offset 8, i.e. exactly on top of the CFD_TERMINATOR above. */ + const char packet2[] = "entry2x"; + ret = SendTransaction(server, packet2, sizeof(packet2) - 1, CF_MORE); + assert_int_equal(ret, 0); + + /* The real last packet, which an unbounded scan never gets to read. */ + const char packet3[] = "entry3\0" TERM "\0\0"; + ret = SendTransaction(server, packet3, sizeof(packet3) - 1, CF_DONE); + assert_int_equal(ret, 0); + + AgentConnection conn; + memset(&conn, 0, sizeof(conn)); + conn.conn_info = ConnectionInfoNew(); + ConnectionInfoSetSocket(conn.conn_info, sv[0]); + ConnectionInfoSetProtocolVersion(conn.conn_info, CF_PROTOCOL_CLASSIC); + conn.this_server = "test-server"; + + Item *list = RemoteDirList("/tmp", false, &conn); + + assert_true(list != NULL); + assert_int_equal(ListLen(list), 3); + assert_string_equal(EntryName(list), "entry1"); + assert_string_equal(EntryName(list->next), "entry2x"); + assert_string_equal(EntryName(list->next->next), "entry3"); + + DeleteItemList(list); + ConnectionInfoDestroy(&conn.conn_info); + ConnectionInfoDestroy(&server); + close(sv[0]); + close(sv[1]); +} + +int main(void) +{ + PRINT_TEST_BANNER(); + const UnitTest tests[] = + { + unit_test(test_dirlist_stops_at_end_of_packet), + }; + + return run_tests(tests); +}