From 142780c7be376d245aec340b73fb4f706db6765a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Mon, 20 Jul 2026 10:35:01 +0200 Subject: [PATCH 1/2] Treat backslash as a path separator in XModem filename validation isValidFilename split components on forward slash only, so backslash separated components were not examined individually. Use strpbrk to split on both, and reject drive qualified names. The native Windows daemon build maps FSCom to PortduinoFS on the host filesystem, where both separators are significant. Adds test coverage for backslash and drive qualified inputs. --- src/xmodem.cpp | 4 +++- test/test_xmodem/test_main.cpp | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/xmodem.cpp b/src/xmodem.cpp index ce7ad8020d0..448615c7bc7 100644 --- a/src/xmodem.cpp +++ b/src/xmodem.cpp @@ -62,10 +62,12 @@ bool XModemAdapter::isValidFilename(const char *name) { if (!name || name[0] == '\0') return false; + if (name[1] == ':') + return false; // Reject any ".." path component. Absolute paths and subdirectories are fine; they stay within // the filesystem root, so only traversal out of it needs blocking. for (const char *seg = name; *seg;) { - const char *slash = strchr(seg, '/'); + const char *slash = strpbrk(seg, "/\\"); const size_t len = slash ? (size_t)(slash - seg) : strlen(seg); if (len == 2 && seg[0] == '.' && seg[1] == '.') return false; diff --git a/test/test_xmodem/test_main.cpp b/test/test_xmodem/test_main.cpp index 816c78e9c01..398c889ed16 100644 --- a/test/test_xmodem/test_main.cpp +++ b/test/test_xmodem/test_main.cpp @@ -21,6 +21,22 @@ void test_xmodem_rejects_dotdot_traversal(void) TEST_ASSERT_FALSE(XModemAdapter::isValidFilename("/..")); } +void test_xmodem_rejects_backslash_traversal(void) +{ + TEST_ASSERT_FALSE(XModemAdapter::isValidFilename("..\\secret")); + TEST_ASSERT_FALSE(XModemAdapter::isValidFilename("..\\..\\Windows\\System32\\drivers\\etc\\hosts")); + TEST_ASSERT_FALSE(XModemAdapter::isValidFilename("dir\\..\\..\\x")); + TEST_ASSERT_FALSE(XModemAdapter::isValidFilename("dir/..\\x")); + TEST_ASSERT_FALSE(XModemAdapter::isValidFilename("dir\\..")); +} + +void test_xmodem_rejects_drive_qualified(void) +{ + TEST_ASSERT_FALSE(XModemAdapter::isValidFilename("C:\\Windows\\System32\\x")); + TEST_ASSERT_FALSE(XModemAdapter::isValidFilename("C:/Windows/System32/x")); + TEST_ASSERT_FALSE(XModemAdapter::isValidFilename("c:relative.txt")); +} + void test_xmodem_rejects_empty(void) { TEST_ASSERT_FALSE(XModemAdapter::isValidFilename("")); @@ -46,6 +62,8 @@ void setup() UNITY_BEGIN(); #ifdef FSCom RUN_TEST(test_xmodem_rejects_dotdot_traversal); + RUN_TEST(test_xmodem_rejects_backslash_traversal); + RUN_TEST(test_xmodem_rejects_drive_qualified); RUN_TEST(test_xmodem_rejects_empty); RUN_TEST(test_xmodem_allows_legit_paths); #endif From 9f1bbd66af05a3d707d761eac22e99311f0accd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Mon, 20 Jul 2026 10:43:53 +0200 Subject: [PATCH 2/2] Restrict drive-qualifier rejection to ASCII drive letters Only [A-Za-z] can begin a drive qualifier, so rejecting on the colon alone also refused legal POSIX filenames such as 1:30pm.txt. --- src/xmodem.cpp | 3 ++- test/test_xmodem/test_main.cpp | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/xmodem.cpp b/src/xmodem.cpp index 448615c7bc7..58339a369dc 100644 --- a/src/xmodem.cpp +++ b/src/xmodem.cpp @@ -62,7 +62,8 @@ bool XModemAdapter::isValidFilename(const char *name) { if (!name || name[0] == '\0') return false; - if (name[1] == ':') + const bool driveLetter = (name[0] >= 'A' && name[0] <= 'Z') || (name[0] >= 'a' && name[0] <= 'z'); + if (driveLetter && name[1] == ':') return false; // Reject any ".." path component. Absolute paths and subdirectories are fine; they stay within // the filesystem root, so only traversal out of it needs blocking. diff --git a/test/test_xmodem/test_main.cpp b/test/test_xmodem/test_main.cpp index 398c889ed16..c6a20fdf051 100644 --- a/test/test_xmodem/test_main.cpp +++ b/test/test_xmodem/test_main.cpp @@ -52,6 +52,9 @@ void test_xmodem_allows_legit_paths(void) // ".." only inside a name (not a whole component) is a valid filename, not traversal. TEST_ASSERT_TRUE(XModemAdapter::isValidFilename("my..file")); TEST_ASSERT_TRUE(XModemAdapter::isValidFilename("...")); + // A colon that cannot form a drive qualifier is a legal filename on the POSIX daemon. + TEST_ASSERT_TRUE(XModemAdapter::isValidFilename("1:30pm.txt")); + TEST_ASSERT_TRUE(XModemAdapter::isValidFilename("dir/1:30pm.txt")); } #endif // FSCom