From 419c5535a80a834b08942ef9c445557eb46a2660 Mon Sep 17 00:00:00 2001 From: Ramon Roche Date: Tue, 7 Apr 2026 18:44:28 -0700 Subject: [PATCH] fix(mavlink): scope FTP symlink hardening to POSIX/SITL only NuttX is not affected by GHSA-93v7-287q-qx4g: it has a flat VFS, the FAT driver used for /fs/microsd does not support symlinks, and CONFIG_PSEUDOFS_SOFTLINKS is off by default. Running realpath() and parent canonicalization there is dead weight that costs flash for no security benefit. Move the canonicalize_path helper, the in-root check, the new includes, and O_NOFOLLOW behind #ifndef __PX4_NUTTX so the NuttX build is functionally identical to main, except that _workOpen and _workWrite now also call _validatePathIsWritable for write paths (matching the pattern already used by the other writable opcodes). On POSIX/SITL the symlink-resolution hardening is unchanged. Drop the now-unused _validatePathIsInRoot declaration from the header. Signed-off-by: Ramon Roche --- src/modules/mavlink/mavlink_ftp.cpp | 49 ++++++++++++++--------------- src/modules/mavlink/mavlink_ftp.h | 1 - 2 files changed, 23 insertions(+), 27 deletions(-) diff --git a/src/modules/mavlink/mavlink_ftp.cpp b/src/modules/mavlink/mavlink_ftp.cpp index 59ef7c4e6a..201c74e238 100644 --- a/src/modules/mavlink/mavlink_ftp.cpp +++ b/src/modules/mavlink/mavlink_ftp.cpp @@ -37,13 +37,16 @@ #include #include #include -#include #include #include #include -#include #include +#ifndef __PX4_NUTTX +#include +#include +#endif + #include "mavlink_ftp.h" #include "mavlink_main.h" @@ -512,11 +515,13 @@ 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. Use O_NOFOLLOW where available - // so a TOCTOU race cannot redirect the leaf through a symlink between - // validation and open(2). + // Set mode to 666 incase oflag has O_CREAT. On POSIX use O_NOFOLLOW so a + // TOCTOU race cannot redirect the leaf through a symlink between + // validation and open(2). NuttX has a flat VFS with no symlink support + // in the FAT driver and PSEUDOFS_SOFTLINKS off by default, so the flag + // is unnecessary there. int leaf_oflag = oflag; -#ifdef O_NOFOLLOW +#if !defined(__PX4_NUTTX) && defined(O_NOFOLLOW) leaf_oflag |= O_NOFOLLOW; #endif int fd = ::open(_work_buffer1, leaf_oflag, PX4_O_MODE_666); @@ -1207,19 +1212,18 @@ static bool canonicalize_path(const char *path, char *out, size_t out_len) } #endif // !__PX4_NUTTX -/** - * Resolve symlinks and verify the path is contained in PX4_STORAGEDIR. - * - * 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. - * - * 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) +bool MavlinkFTP::_validatePathIsWritable(const char *path) { #ifndef __PX4_NUTTX + // POSIX/SITL: resolve symlinks and verify the canonical path stays + // inside PX4_STORAGEDIR. 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 (GHSA-93v7-287q-qx4g). + // + // NuttX is not affected: it has a flat VFS, the FAT driver used for + // /fs/microsd does not support symlinks, and CONFIG_PSEUDOFS_SOFTLINKS + // is off by default. char canonical_root[PATH_MAX]; if (realpath(PX4_STORAGEDIR, canonical_root) == nullptr) { @@ -1246,20 +1250,13 @@ bool MavlinkFTP::_validatePathIsInRoot(const char *path) 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. + // NuttX: simple string-based check. Original behavior, unchanged. 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); + PX4_ERR("Disallowing write to %s", path); return false; } return true; #endif } - -bool MavlinkFTP::_validatePathIsWritable(const char *path) -{ - return _validatePathIsInRoot(path); -} diff --git a/src/modules/mavlink/mavlink_ftp.h b/src/modules/mavlink/mavlink_ftp.h index a213043faf..344a61997f 100644 --- a/src/modules/mavlink/mavlink_ftp.h +++ b/src/modules/mavlink/mavlink_ftp.h @@ -150,7 +150,6 @@ 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