XModem: reject path-traversal filenames in the transfer handler (#11037)

The SOH/STX control frame carries a client-supplied filename that was passed
straight to FSCom open/remove/exists, so a ".." component could write, read, or
delete outside the filesystem root. On embedded LittleFS this is largely inert
(no parent of the partition root); on the Portduino daemon FSCom is the host
filesystem under a mountpoint, so it is a real arbitrary-path write/read/delete.

Validate the filename before any FS access: reject empty and any ".." path
component, and NAK the transfer. Absolute and subdirectory paths are still
accepted - the file manager transfers them from the manifest and PortduinoFS
confines them to its mountpoint - so only traversal out of the root is blocked.

Reachable only from a local client connection (PhoneAPI: BLE/USB/serial/TCP),
not over the RF mesh; on the daemon the TCP API makes it network-reachable.

native-suite-count goes to 34: +1 for the new test_xmodem suite and +1 correcting
a pre-existing miscount (it read 32 for 33 suite directories).

Co-authored-by: Ben Meadors <benmmeadors@gmail.com>

src/xmodem.cpp

@@ -50,6 +50,7 @@
 
 #include "xmodem.h"
 #include "SPILock.h"
+#include <cstring>
 
 #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();

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;

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 <unity.h>
+
+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() {}