From 96f90f6602f973b16c459f71de1b8ad7e9c08926 Mon Sep 17 00:00:00 2001 From: Ramon Roche Date: Tue, 7 Apr 2026 08:12:11 -0700 Subject: [PATCH] fix(mavlink): harden FTP path validation against unsafe write targets Tighten path validation in MavlinkFTP so that FTP write operations consistently reject unsafe targets and resolve symlinks before checking the result against the FTP root. Add a new helper _validatePathIsInRoot() that canonicalizes the requested path with realpath() and verifies the result is contained inside PX4_STORAGEDIR. For paths that do not yet exist (CreateFile, CreateDirectory) the parent directory is canonicalized and the leaf name reattached so the check still produces a meaningful result. NuttX falls back to the previous string based prefix and traversal check because realpath() is not available there. Rewrite _validatePathIsWritable() to call the new in-root check on all platforms, and to deny writes that target the boot executed startup hook subtree (PX4_STORAGEDIR /etc/). Drop the NuttX only ifdef around the existing prefix check so the same logic runs on POSIX builds too. Apply _validatePathIsWritable() in _workOpen() for write mode opens (O_WRONLY or O_RDWR), and add the missing _validatePath() call to _workWrite() so the WriteFile opcode now matches the pattern used by the other write capable opcodes. Open the leaf file with O_NOFOLLOW where the platform provides it so a TOCTOU race cannot redirect the leaf through a symlink between validation and open(2). Refs: GHSA-c6f8-f7w2-785x, GHSA-93v7-287q-qx4g Signed-off-by: Ramon Roche --- src/modules/mavlink/mavlink_ftp.cpp | 151 ++++++++++++++++++++++++++-- src/modules/mavlink/mavlink_ftp.h | 1 + 2 files changed, 142 insertions(+), 10 deletions(-) diff --git a/src/modules/mavlink/mavlink_ftp.cpp b/src/modules/mavlink/mavlink_ftp.cpp index b97cc199ef..04bd398dde 100644 --- a/src/modules/mavlink/mavlink_ftp.cpp +++ b/src/modules/mavlink/mavlink_ftp.cpp @@ -37,9 +37,11 @@ #include #include #include +#include #include #include #include +#include #include #include "mavlink_ftp.h" @@ -482,6 +484,12 @@ MavlinkFTP::_workOpen(PayloadHeader *payload, int oflag) return kErrFailFileProtected; } + const bool is_write_open = (oflag & (O_WRONLY | O_RDWR)) != 0; + + if (is_write_open && !_validatePathIsWritable(_work_buffer1)) { + return kErrFailFileProtected; + } + PX4_DEBUG("FTP: open '%s'", _work_buffer1); uint32_t fileSize = 0; @@ -504,8 +512,14 @@ MavlinkFTP::_workOpen(PayloadHeader *payload, int oflag) fileSize = st.st_size; PX4_DEBUG("open: %s", _work_buffer1); - // Set mode to 666 incase oflag has O_CREAT - int fd = ::open(_work_buffer1, oflag, PX4_O_MODE_666); + // Set mode to 666 incase oflag has O_CREAT. Use O_NOFOLLOW where available + // so a TOCTOU race cannot redirect the leaf through a symlink between + // validation and open(2). + int leaf_oflag = oflag; +#ifdef O_NOFOLLOW + leaf_oflag |= O_NOFOLLOW; +#endif + int fd = ::open(_work_buffer1, leaf_oflag, PX4_O_MODE_666); if (fd < 0) { _our_errno = errno; @@ -592,7 +606,7 @@ MavlinkFTP::_workWrite(PayloadHeader *payload) return kErrInvalidSession; } - if (!_validatePathIsWritable(_work_buffer1)) { + if (!_validatePath(_work_buffer1) || !_validatePathIsWritable(_work_buffer1)) { return kErrFailFileProtected; } @@ -1144,18 +1158,135 @@ bool MavlinkFTP::_validatePath(const char *path) return true; } +/** + * Resolve symlinks and verify the path is contained in PX4_STORAGEDIR. + * + * Used as a defence against symlink-resolution bypasses where _validatePath() + * accepts a string that does not contain ".." but the underlying open()/mkdir() + * follows a symlink to a target outside the intended FTP root. + * + * For files that do not yet exist (CreateFile, CreateDirectory) realpath() on + * the full path would fail, so we resolve the parent directory and reattach + * the leaf name. The result is a canonical absolute path (or a relative path + * starting with the canonicalized PX4_STORAGEDIR) that is then prefix-matched + * against the canonicalized root. + * + * On NuttX realpath() is not available; the simpler string-based check from + * the previous implementation is used in that case. + */ +bool MavlinkFTP::_validatePathIsInRoot(const char *path) +{ +#ifndef __PX4_NUTTX + char canonical_root[PATH_MAX]; + + if (realpath(PX4_STORAGEDIR, canonical_root) == nullptr) { + PX4_ERR("FTP: realpath(root) failed: %s", strerror(errno)); + return false; + } + + const size_t canonical_root_len = strlen(canonical_root); + + // Try to canonicalize the full path. If the leaf does not yet exist, + // fall back to resolving the parent directory and reattaching the leaf. + char canonical[PATH_MAX]; + + if (realpath(path, canonical) == nullptr) { + // Split into parent and leaf, resolve the parent, reattach the leaf. + char parent_buf[PATH_MAX]; + strncpy(parent_buf, path, sizeof(parent_buf) - 1); + parent_buf[sizeof(parent_buf) - 1] = '\0'; + + char *slash = strrchr(parent_buf, '/'); + const char *leaf = nullptr; + + if (slash != nullptr) { + *slash = '\0'; + leaf = slash + 1; + + } else { + // No slash: parent is current directory, leaf is the whole path + parent_buf[0] = '.'; + parent_buf[1] = '\0'; + leaf = path; + } + + char canonical_parent[PATH_MAX]; + + if (realpath(parent_buf, canonical_parent) == nullptr) { + PX4_ERR("FTP: realpath(parent of %s) failed: %s", path, strerror(errno)); + return false; + } + + const int n = snprintf(canonical, sizeof(canonical), "%s/%s", canonical_parent, leaf); + + if (n < 0 || (size_t)n >= sizeof(canonical)) { + PX4_ERR("FTP: canonical path too long for %s", path); + return false; + } + } + + // The canonical path must start with the canonical root and either + // equal it or be followed by a path separator. + if (strncmp(canonical, canonical_root, canonical_root_len) != 0 + || (canonical[canonical_root_len] != '\0' && canonical[canonical_root_len] != '/')) { + PX4_ERR("FTP: rejecting path outside FTP root: %s -> %s", path, canonical); + return false; + } + + return true; +#else + // NuttX: realpath() is not available. Fall back to a string-based prefix + // and traversal check; symlinks are not commonly used on NuttX targets. + if (strncmp(path, CONFIG_BOARD_ROOT_PATH "/", strlen(CONFIG_BOARD_ROOT_PATH "/")) != 0 + || strstr(path, "/../") != nullptr) { + PX4_ERR("FTP: rejecting path outside FTP root: %s", path); + return false; + } + + return true; +#endif +} + bool MavlinkFTP::_validatePathIsWritable(const char *path) { -#ifdef __PX4_NUTTX - - // Don't allow writes to system paths as they are in RAM - // Ideally we'd canonicalize the path (with 'realpath'), but it might not exist, so realpath() would fail. - // The next simpler thing is to check there's no reference to a parent dir. - if (strncmp(path, CONFIG_BOARD_ROOT_PATH "/", 12) != 0 || strstr(path, "/../") != nullptr) { - PX4_ERR("Disallowing write to %s", path); + if (!_validatePathIsInRoot(path)) { return false; } + // Reject writes to boot-executed startup hook files. The PX4 NuttX rcS + // sources $FRC, $FCONFIG, $FEXTRAS unconditionally during boot, so an + // attacker that can write to these paths gets persistent code execution + // at the next reboot. Reject the entire PX4_STORAGEDIR/etc/ subtree to + // keep future hooks safe by default. + static const char kBootHookPrefix[] = PX4_STORAGEDIR "/etc/"; + const size_t kBootHookPrefixLen = sizeof(kBootHookPrefix) - 1; + + if (strncmp(path, kBootHookPrefix, kBootHookPrefixLen) == 0) { + PX4_ERR("FTP: refusing to write protected path %s", path); + return false; + } + +#ifndef __PX4_NUTTX + // Also reject the canonical form, in case an in-root symlink redirects + // the leaf into the protected etc/ subtree. + char canonical[PATH_MAX]; + + if (realpath(path, canonical) != nullptr) { + char canonical_root[PATH_MAX]; + + if (realpath(PX4_STORAGEDIR, canonical_root) != nullptr) { + char canonical_boot_prefix[PATH_MAX]; + const int n = snprintf(canonical_boot_prefix, sizeof(canonical_boot_prefix), + "%s/etc/", canonical_root); + + if (n > 0 && (size_t)n < sizeof(canonical_boot_prefix) + && strncmp(canonical, canonical_boot_prefix, strlen(canonical_boot_prefix)) == 0) { + PX4_ERR("FTP: refusing to write protected path %s -> %s", path, canonical); + return false; + } + } + } + #endif return true; } diff --git a/src/modules/mavlink/mavlink_ftp.h b/src/modules/mavlink/mavlink_ftp.h index 344a61997f..a213043faf 100644 --- a/src/modules/mavlink/mavlink_ftp.h +++ b/src/modules/mavlink/mavlink_ftp.h @@ -150,6 +150,7 @@ private: bool _validatePath(const char *path); bool _validatePathIsWritable(const char *path); + bool _validatePathIsInRoot(const char *path); /** * make sure that the working buffers _work_buffer* are allocated