Add sandbox confinement to Svc::FileManager - #5852
Open
Moferanoluwa wants to merge 1 commit into
Open
Conversation
Fixes nasa#5849. FileManager was the only file-handling component without optional path confinement: every command handler passed its ground-supplied path straight to Os::FileSystem/Os::File/Os::Directory, and the FileHandling subtopology's sandboxDir knob did not cover it, so a deployment relying on that knob could believe FileManager's commands were confined when they were not. Added FileManager::configure(sandboxDir), fail-closed like FileUplink, FileDownlink, and PrmDb (routed through Os::FileSystem/Os::Directory directly rather than Os::SandboxedFile, since FileManager doesn't go through Os::File uniformly). Every path argument across all nine command handlers is validated via a shared checkSandbox() helper (Os::FilePathUtils resolve + checkContainment) before it reaches the OSAL, including both legs of MoveFile/AppendFile and the per-entry path ListDirectory constructs while walking a directory; rejections emit a new PathOutsideSandbox warning. ignoreErrors on RemoveFile does not waive the check. Extracted a shared Os::FilePathUtils::resolveDirectory() helper (resolve + trailing slash) so SandboxedFile and FileManager don't each reimplement it. FileHandling now configures fileManager to sandboxDir ("/" by default, for backwards compatibility) alongside its siblings, and Ref reconfigures it to "." like fileUplink/fileDownlink/prmDb. Testing: added unit tests covering the fail-closed default, escaping paths, ignoreErrors not bypassing the check, MoveFile's independent per-leg reporting, and the distinct ListDirectory/GenerateDp/CalculateCrc code paths, plus a Ref integration test mirroring test_prm_db_sandbox. Verified via a full WSL build: Os_FilePathUtils_Test, Os_SandboxedFile_Test, and Svc_FileManager_ut_exe all pass, and the Ref deployment (which exercises the FileHandling.fpp phase change and the RefTopology.cpp wiring) builds and links cleanly. Did not run the new Ref integration test itself (requires fprime-gds, not available in this environment). AI-assisted development (Claude Code) — I traced the gap through the sibling components, designed and implemented the fix and tests myself, verified the build and unit tests in a WSL environment since this dev machine can't natively build F', and take responsibility for every line.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change Description
Svc::FileManagerwas the only file-handling component without optional path confinement. All nine of its command handlers (CreateDirectory,RemoveFile,MoveFile,RemoveDirectory,AppendFile,FileSize,ListDirectory,CalculateCrc,GenerateDp) passed ground-supplied paths straight toOs::FileSystem/Os::File/Os::Directory, and theFileHandlingsubtopology'ssandboxDirknob did not cover it.FileManager::configure(sandboxDir), fail-closed likeFileUplink/FileDownlink/PrmDb. SinceFileManagerdoesn't go throughOs::Fileuniformly (most handlers callOs::FileSystemstatics orOs::Directorydirectly), it validates paths itself via a new privatecheckSandbox()helper built onOs::FilePathUtils::resolveFromCwd+checkContainment, rather than wrappingOs::SandboxedFile(which only coversOs::File::open).MoveFile/AppendFile(both are checked independently, not short-circuited, so a violation on either side is reported) and the per-entry pathListDirectoryconstructs while walking a directory (defense in depth against a platform that yields a./..entry — POSIX'sOs::Directoryalready filters these, but the check doesn't assume that). Rejections emit a newPathOutsideSandboxwarning event.RemoveFile'signoreErrorsflag only waives missing-file errors, not the sandbox check.Os::FilePathUtils::resolveDirectory()(resolve + ensure trailing/) out ofOs::SandboxedFile::configure()soFileManager::configure()doesn't reimplement that logic.FileHandlingnow configuresfileManager's sandbox toFileHandlingConfig::Paths::sandboxDir("/"by default) alongside its siblings, andRefreconfigures it to"."likefileUplink/fileDownlink/prmDb.Rationale
Fixes #5849. A deployment that sets
FileHandlingConfig::Paths::sandboxDir(or reconfigures it) to restrict file access could reasonably believe the wholeFileHandlingsubtopology is confined, whenFileManager's command set — which can create/remove/move/append/CRC any path, and package arbitrary files into data products — was not.Testing/Review Recommendations
Added to
Svc/FileManager/test/ut: fail-closed-by-default rejection, an escaping absolute path rejected on a single-path command, a path inside the configured sandbox still succeeding, both legs ofMoveFilereported independently when both escape,ignoreErrors=truenot bypassing the check onRemoveFile, and the three structurally distinct code paths (ListDirectory's async open,GenerateDp's always-OK-response convention,CalculateCrc's directOs::Fileuse). Also addedtest_file_manager_sandboxto theRefintegration suite, mirroring the existingtest_prm_db_sandbox.This dev environment can't natively build F' (no Windows platform support in the CMake build), so I set up WSL Ubuntu to verify:
Os_FilePathUtils_Test(24/24) andOs_SandboxedFile_Test(11/11) still pass after extractingresolveDirectory().Svc_FileManager_ut_exe(36/36, all pre-existing tests plus the 8 new sandbox tests) passes.Refdeployment builds and links cleanly end-to-end, which exercises both theFileHandling.fppconfigComponentsphase change and theRefTopology.cppwiring (these aren't touched by a component-levelfprime-util check, so this was the only way to compile-check them).I did not run the new
test_file_manager_sandboxRef integration test itself — that needsfprime-gds, which isn't set up in this environment. It follows the same pattern astest_prm_db_sandboximmediately above it in the same file, which does pass in CI.Future Work
None.
AI Usage (see policy)
FileUplink,FileDownlink,PrmDb) and the recentOs::SandboxedFilefail-closed rework (Keep Os::SandboxedFile fail-closed, default subtopology sandboxes to "/", sandbox PrmDb whole-component, restrict Ref to "." #5833), designed and implemented the fix, wrote the unit/integration tests, and verified the build in WSL since this machine can't natively build F'.FileManager,Os::FilePathUtils/Os::SandboxedFile(extracted a shared helper, no behavior change there),FileHandlingsubtopology wiring,Reftopology/integration test.IAMAI