From ee732b74378b6556de844680e770054c1719a3b4 Mon Sep 17 00:00:00 2001 From: aizu-m Date: Wed, 10 Jun 2026 10:50:57 +0530 Subject: [PATCH] Reject directory listing entries containing a path separator Entries from a remote directory listing are joined to a local path during recursive copy (cf-agent) and getdir (cf-net). A genuine dirent is a single path component; one carrying a separator escapes the destination once joined, so a hostile server can write outside it. ConsiderFile() already blocks ".", ".." and "..."; reject separator-bearing names there and in process_dir_recursive() too. Signed-off-by: aizu-m --- cf-agent/files_properties.c | 14 +++++++++++ cf-net/cf-net.c | 20 ++++++++++++++++ tests/unit/Makefile.am | 3 +++ tests/unit/files_properties_test.c | 38 ++++++++++++++++++++++++++++++ 4 files changed, 75 insertions(+) create mode 100644 tests/unit/files_properties_test.c diff --git a/cf-agent/files_properties.c b/cf-agent/files_properties.c index d5fa3b6581..ef9101de04 100644 --- a/cf-agent/files_properties.c +++ b/cf-agent/files_properties.c @@ -26,6 +26,7 @@ #include #include +#include /* IsFileSep */ #include static Item *SUSPICIOUSLIST = NULL; /* GLOBAL_P */ @@ -69,6 +70,19 @@ static bool ConsiderFile(const char *nodename, const char *path, struct stat *st return false; } + /* A directory entry is a single path component and never contains a + * separator. One that does (e.g. "../x" from a hostile server's listing) + * would escape 'path' when the two are joined, see + * http://cwe.mitre.org/data/definitions/32.html */ + for (const char *s = nodename; *s != '\0'; s++) + { + if (IsFileSep(*s)) + { + Log(LOG_LEVEL_WARNING, "Path separator in directory entry '%s/%s', skipping", path, nodename); + return false; + } + } + for (i = 0; SKIPFILES[i] != NULL; i++) { if (strcmp(nodename, SKIPFILES[i]) == 0) diff --git a/cf-net/cf-net.c b/cf-net/cf-net.c index 82f9bbd7b7..6d120f61b4 100644 --- a/cf-net/cf-net.c +++ b/cf-net/cf-net.c @@ -1088,6 +1088,26 @@ static int process_dir_recursive(AgentConnection *conn, continue; } + /* A directory entry is a single path component. A separator here + * (e.g. "../x" from a hostile server) would escape local_path once + * joined below, see http://cwe.mitre.org/data/definitions/32.html */ + bool has_separator = false; + for (const char *s = item; *s != '\0'; s++) + { + if (IsFileSep(*s)) + { + has_separator = true; + break; + } + } + if (has_separator) + { + Log(LOG_LEVEL_ERR, + "Skipping remote directory entry with path separator: '%s'", + item); + continue; + } + char remote_full[PATH_MAX]; int written = snprintf(remote_full, sizeof(remote_full), "%s/%s", remote_path, item); if (written < 0 || (size_t) written >= sizeof(remote_full)) diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am index 3a4bb0a9d4..652b838fa6 100644 --- a/tests/unit/Makefile.am +++ b/tests/unit/Makefile.am @@ -131,6 +131,7 @@ check_PROGRAMS = \ sysinfo_test \ variable_test \ verify_databases_test \ + files_properties_test \ protocol_test \ mon_cpu_test \ mon_load_test \ @@ -383,6 +384,8 @@ strlist_test_SOURCES = strlist_test.c \ verify_databases_test_LDADD = ../../cf-agent/libcf-agent.la libtest.la +files_properties_test_LDADD = ../../cf-agent/libcf-agent.la libtest.la + iteration_test_SOURCES = iteration_test.c cf_upgrade_test_SOURCES = cf_upgrade_test.c \ diff --git a/tests/unit/files_properties_test.c b/tests/unit/files_properties_test.c new file mode 100644 index 0000000000..bfc141d36f --- /dev/null +++ b/tests/unit/files_properties_test.c @@ -0,0 +1,38 @@ +#include + +#include // Include .c file to test static functions + +void test_ConsiderFile_path_separators(void) +{ + // A genuine directory entry is a single path component. One carrying a + // separator (e.g. from a hostile server's directory listing) would escape + // the directory once joined, see http://cwe.mitre.org/data/definitions/32.html + assert_false(ConsiderFile("../etc", "dir", NULL)); + assert_false(ConsiderFile("a/b", "dir", NULL)); + assert_false(ConsiderFile("../../etc/cron.d/x", "dir", NULL)); + assert_false(ConsiderFile("/etc/passwd", "dir", NULL)); +#ifdef _WIN32 + assert_false(ConsiderFile("a\\b", "dir", NULL)); +#endif + + // Existing guards still hold. + assert_false(ConsiderFile("", "dir", NULL)); + assert_false(ConsiderFile(".", "dir", NULL)); + assert_false(ConsiderFile("..", "dir", NULL)); + assert_false(ConsiderFile("...", "dir", NULL)); + + // Ordinary names are still accepted. + assert_true(ConsiderFile("promises.cf", "dir", NULL)); + assert_true(ConsiderFile("a.b", "dir", NULL)); +} + +int main() +{ + PRINT_TEST_BANNER(); + const UnitTest tests[] = + { + unit_test(test_ConsiderFile_path_separators), + }; + + return run_tests(tests); +}