Reject directory listing entries containing a path separator

cf-agent/files_properties.c

@@ -26,6 +26,7 @@
 
 #include <files_names.h>
 #include <files_interfaces.h>
+#include <file_lib.h>                                      /* IsFileSep */
 #include <item_lib.h>
 
 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)

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))

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 \

tests/unit/files_properties_test.c

@@ -0,0 +1,38 @@
+#include <test.h>
+
+#include <files_properties.c> // 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);
+}