diff --git a/src/xmodem.cpp b/src/xmodem.cpp index 447a05ff0ae..ce7ad8020d0 100644 --- a/src/xmodem.cpp +++ b/src/xmodem.cpp @@ -50,6 +50,7 @@ #include "xmodem.h" #include "SPILock.h" +#include #ifdef FSCom @@ -57,6 +58,24 @@ XModemAdapter xModem; XModemAdapter::XModemAdapter() {} +bool XModemAdapter::isValidFilename(const char *name) +{ + if (!name || name[0] == '\0') + 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 size_t len = slash ? (size_t)(slash - seg) : strlen(seg); + if (len == 2 && seg[0] == '.' && seg[1] == '.') + return false; + if (!slash) + break; + seg = slash + 1; + } + return true; +} + /** * Calculates the CRC-16 CCITT checksum of the given buffer. * @@ -122,6 +141,14 @@ void XModemAdapter::handlePacket(meshtastic_XModem xmodemPacket) strncpy(filename, (const char *)xmodemPacket.buffer.bytes, sizeof(filename) - 1); filename[sizeof(filename) - 1] = '\0'; + // The filename is attacker-controlled; refuse a ".." that could write/read/delete + // outside the filesystem root (real host paths on the posix daemon). + if (!isValidFilename(filename)) { + LOG_WARN("XModem: rejecting unsafe filename"); + sendControl(meshtastic_XModem_Control_NAK); + break; + } + if (xmodemPacket.control == meshtastic_XModem_Control_SOH) { // Receive this file and put to Flash // FILE_O_WRITE on Adafruit_LittleFS is append, not truncate - remove first. spiLock->lock(); diff --git a/src/xmodem.h b/src/xmodem.h index 97418782032..6119a7b5097 100644 --- a/src/xmodem.h +++ b/src/xmodem.h @@ -55,6 +55,11 @@ class XModemAdapter // True while a file transfer is in flight; lets callers avoid racing our `file` handle. bool isBusy() const { return isReceiving || isTransmitting; } + // Reject a transfer filename that could escape the filesystem root via a ".." path component. + // Absolute/subdirectory paths are allowed - PortduinoFS confines them to its mountpoint - so + // this is only about traversal, which matters on the posix daemon where FSCom is the host FS. + static bool isValidFilename(const char *name); + private: bool isReceiving = false; bool isTransmitting = false; diff --git a/test/test_xmodem/test_main.cpp b/test/test_xmodem/test_main.cpp new file mode 100644 index 00000000000..816c78e9c01 --- /dev/null +++ b/test/test_xmodem/test_main.cpp @@ -0,0 +1,55 @@ +// Tests for XModemAdapter::isValidFilename - the path-traversal guard on the XModem file-transfer +// handler (src/xmodem.cpp). The filename in a SOH/STX control frame is attacker-controlled and +// drives FSCom open/remove; on the Portduino daemon FSCom is the host filesystem, so a ".." +// component could escape the mountpoint. Absolute/subdirectory paths must still be accepted. +#include "TestUtil.h" +#include "xmodem.h" +#include + +void setUp(void) {} +void tearDown(void) {} + +#ifdef FSCom + +void test_xmodem_rejects_dotdot_traversal(void) +{ + TEST_ASSERT_FALSE(XModemAdapter::isValidFilename("..")); + TEST_ASSERT_FALSE(XModemAdapter::isValidFilename("../secret")); + TEST_ASSERT_FALSE(XModemAdapter::isValidFilename("/prefs/../../etc/passwd")); + TEST_ASSERT_FALSE(XModemAdapter::isValidFilename("a/../b")); + TEST_ASSERT_FALSE(XModemAdapter::isValidFilename("dir/..")); + TEST_ASSERT_FALSE(XModemAdapter::isValidFilename("/..")); +} + +void test_xmodem_rejects_empty(void) +{ + TEST_ASSERT_FALSE(XModemAdapter::isValidFilename("")); + TEST_ASSERT_FALSE(XModemAdapter::isValidFilename(nullptr)); +} + +void test_xmodem_allows_legit_paths(void) +{ + // The file manager transfers absolute, subdirectoried paths from the manifest. + TEST_ASSERT_TRUE(XModemAdapter::isValidFilename("/prefs/config.proto")); + TEST_ASSERT_TRUE(XModemAdapter::isValidFilename("firmware.bin")); + TEST_ASSERT_TRUE(XModemAdapter::isValidFilename("dir/sub/file.txt")); + // ".." 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("...")); +} + +#endif // FSCom + +void setup() +{ + initializeTestEnvironment(); + UNITY_BEGIN(); +#ifdef FSCom + RUN_TEST(test_xmodem_rejects_dotdot_traversal); + RUN_TEST(test_xmodem_rejects_empty); + RUN_TEST(test_xmodem_allows_legit_paths); +#endif + exit(UNITY_END()); +} + +void loop() {}