feat(FileDownlink): sandboxed reads via Os::SandboxedFile by Abhishek21g · Pull Request #5404 · nasa/fprime

Abhishek21g · 2026-07-13T05:13:57Z

Summary

Mirror Svc::FileUplink write-side sandboxing on the read path for Svc::FileDownlink
Add configure(const char* directory) overload, SourceOutOfSandbox event, and unit test coverage
Opt-in / backward compatible (default sandbox remains unrestricted)

Fixes #5398. Complements #5369 FileDownlink cleanup work.

Test plan

Svc/FileDownlink/test/ut — new SourceOutOfSandbox test passes
Existing FileDownlink unit tests pass with CWD sandbox configured in tester ctor

Mirror FileUplink write-side confinement for commanded downlink paths. Adds configure(directory), SourceOutOfSandbox event, and unit coverage. Fixes nasa#5398

lestarch-autobot · 2026-07-13T17:31:59Z

-        //! The underlying OS file
-        Os::File m_osFile;
+        //! The underlying OS file (sandboxed to restrict read locations)
+        Os::SandboxedFile m_osFile;


[Security] must fix Sandbox validation happens after an unrestricted file-size query.

FileDownlink::File::open() calls Os::FileSystem::getFileSize(sourceFileName, ...) before m_osFile.open(...) performs the sandbox check. A ground-controlled source path can therefore probe metadata for arbitrary paths outside the configured directory, bypassing the new read boundary. Validate the path through the sandbox before querying its size, or add a sandboxed size/stat operation.

Addressed in 453c44a: getFileSize removed. open() now opens via Os::SandboxedFile first, then queries size with m_osFile.size() after sandbox validation.

Avoid unrestricted Os::FileSystem::getFileSize on commanded paths before the SandboxedFile open check. Open first, then use SandboxedFile::size(). Addresses security review on nasa#5404.

lestarch-autobot · 2026-07-13T17:33:54Z

+    char cwd[Os::FilePathUtils::MAX_PATH_LENGTH];
+    FW_ASSERT(getcwd(cwd, sizeof(cwd)) != nullptr);


[C++ Design] must fix CPP-29: FW_ASSERT predicate performs the getcwd write.

FW_ASSERT may compile out, so disabling assertions leaves cwd uninitialized before strlen(cwd) and can cause undefined behavior. Evaluate getcwd before the assertion and initialize the local explicitly.

Suggested change

char cwd[Os::FilePathUtils::MAX_PATH_LENGTH];

FW_ASSERT(getcwd(cwd, sizeof(cwd)) != nullptr);

char cwd[Os::FilePathUtils::MAX_PATH_LENGTH] = {};

const char* cwdResult = getcwd(cwd, sizeof(cwd));

FW_ASSERT(cwdResult != nullptr);

Addressed in 7e8c24b: cwd is zero-initialized and getcwd runs outside FW_ASSERT; the assert only checks the result pointer.

lestarch-autobot · 2026-07-13T17:34:40Z

   command and port.

+4. File access may optionally be sandboxed to a configured directory via `configure(directory)`.
+   When configured, all commanded source paths are validated against the sandbox directory before reading.


[Documentation] must fix The SDD does not describe the new sandbox rejection contract.

CPP behavior applies the sandbox to both commanded and SendFile port requests and emits SourceOutOfSandbox when a source is rejected, but this text only says “commanded” paths and omits the operator-visible warning event. Keep the component SDD aligned with the new FPP behavior.

Suggested change

When configured, all commanded source paths are validated against the sandbox directory before reading.

When configured, all source paths supplied through `SendFile`/`SendPartial` commands or the `SendFile` port are validated against the sandbox directory before reading; rejected paths emit `SourceOutOfSandbox`.

Addressed in 7e8c24b: SDD now documents sandbox configure(directory), SendFile/command coverage, and SourceOutOfSandbox.

lestarch-autobot · 2026-07-13T17:35:40Z

-        //! The underlying OS file
-        Os::File m_osFile;
+        //! The underlying OS file (sandboxed to restrict read locations)
+        Os::SandboxedFile m_osFile;


[Design] Human design adjudication required. This change crosses the Svc::FileDownlink and Os::SandboxedFile layer boundary, so the design owner should confirm the intended confinement contract before deeper review proceeds.

The linked issue proposes symmetry with FileUplink, while this PR adds a new component-level configuration path and a new operator-visible event; please confirm this is the approved integration boundary and lifecycle.

cc @LeStarch @thomas-bc — design needs human adjudication before deeper review.

lestarch-autobot · 2026-07-13T17:35:40Z

-        //! The underlying OS file
-        Os::File m_osFile;
+        //! The underlying OS file (sandboxed to restrict read locations)
+        Os::SandboxedFile m_osFile;


[Design] must fix The implementation does not fully match the sandboxed-read design.

FileDownlink::File::open() still calls unrestricted Os::FileSystem::getFileSize() before m_osFile.open() performs sandbox validation, so the new boundary is not applied to the complete source-access operation. Use a sandbox-aware size query or validate before the metadata lookup.

Addressed in 453c44a / 7e8c24b: unrestricted pre-open size lookup removed; size is queried only after sandboxed open succeeds.

lestarch-autobot

Automated review summary (run 1)

Per-agent results

Agent	must fix	suggestion	could fix	future work	outstanding	Verdict
Security Vulnerabilities	1	0	0	0	1	No-Go
Supply Chain / Runner Safety	0	0	0	0	0	Go
F Prime C/C++ Design	1	0	0	0	1	No-Go
Documentation Currency	1	0	0	0	1	No-Go
Design	2	0	0	0	2	No-Go
Architecture	0	0	0	0	0	Go
Test Quality	0	0	0	0	0	Go
CI safety	—	—	—	—	—	Go
Totals	5	0	0	0	5	No-Go

Supply-chain surfaces

Surface	Outstanding
Dependencies	clean
Vendored / submodule	clean
Build / test infrastructure	clean
Workflows / actions / scripts	clean
Generator output	clean
Prompt-injection	clean
Review-system integrity	clean

Outstanding must-fix items (5)

Security Vulnerabilities

Unrestricted getFileSize runs before sandbox validation, allowing metadata access outside the configured directory. — finding

F Prime C/C++ Design

getcwd is executed inside an assertion predicate, leaving cwd uninitialized when assertions are compiled out. — finding

Documentation Currency

The SDD omits sandbox behavior for the SendFile port and the SourceOutOfSandbox event. — finding

Design

Human design-owner adjudication is required for the new Svc/Os confinement integration boundary. — finding
The implementation does not apply sandbox validation to the complete source-access operation before metadata lookup. — finding

Merge readiness

Merge readiness: No-Go — five outstanding must-fix findings remain across security, C/C++ correctness, documentation, and design review.

The implementation is cleared for CI-safety concerns, but merge should wait for the outstanding findings and design-owner adjudication.

The review constellation has mapped the hazards; resolve them before launch.

- CPP-29: initialize cwd buffer before FW_ASSERT(getcwd) - Document SourceOutOfSandbox and port/command sandbox contract in SDD - Apply clang-format

github-actions · 2026-07-13T17:43:04Z

Coverage report — base `devel`

Overall (line): 82.08% → 82.10% (+0.02)
Regression threshold: 0.50% (line).

Regressions

(none over threshold)

Modules changed

Module	Line	Δ	Function	Δ	Branch	Δ
`Os/Posix`	62.00	-0.40	84.21	+0.00	44.10	+0.00
`Os/Generic`	89.03	-0.13	88.16	+0.00	72.86	-0.25
`Fw/DataStructures`	98.48	+0.27	97.14	+0.00	83.21	+0.37
`Os/Generic/Types`	92.39	+0.36	92.86	+0.00	75.24	+2.86
`Svc/FileDownlink`	84.57	+0.42	91.49	+0.58	72.13	+0.23

Modules without UTs

CFDP/Checksum/GTest, Drv/ByteStreamDriverModel, Drv/Interfaces, Drv/LinuxGpioDriver, Drv/LinuxI2cDriver, Drv/LinuxSpiDriver, Drv/LinuxUartDriver, Drv/Ports, Drv/Ports/DataTypes, FppTestProject/FppTest/interfaces, FppTestProject/FppTest/topology/async, FppTestProject/FppTest/topology/components/Comp, FppTestProject/FppTest/topology/components/Framework, FppTestProject/FppTest/topology/components/Receiver, FppTestProject/FppTest/topology/components/Sender, FppTestProject/FppTest/topology/guarded, FppTestProject/FppTest/topology/ports, FppTestProject/FppTest/topology/sync, FppTestProject/FppTest/topology/top_ports, FppTestProject/FppTest/topology/types, Fw/Cmd, Fw/Com, Fw/Comp, Fw/FilePacket/GTest, Fw/Fpy, Fw/Interfaces, Fw/Obj, Fw/Port, Fw/Ports/CompletionStatus, Fw/Ports/Ready, Fw/Ports/Signal, Fw/Ports/SuccessCondition, Fw/Prm, Fw/SerializableFile/test/TestSerializable, Fw/Sm, Fw/Test, Fw/Types/GTest, Os/Models, Svc/Cycle, Svc/DpPorts, Svc/Fatal, Svc/FatalHandler, Svc/FileDownlinkPorts, Svc/FprimeProtocol, Svc/Interfaces, Svc/PassiveConsoleTextLogger, Svc/Ping, Svc/PolyIf, Svc/Ports/CommsPorts, Svc/Ports/FilePorts, Svc/Ports/OsTimeEpoch, Svc/Ports/TlmPacketizerPorts, Svc/Ports/VersionPorts, Svc/Sched, Svc/Seq, Svc/Subtopologies/CdhCore, Svc/Subtopologies/ComCcsds, Svc/Subtopologies/ComFprime, Svc/Subtopologies/ComLoggerTee, Svc/Subtopologies/DataProducts, Svc/Subtopologies/DpCompression, Svc/Subtopologies/FileHandling, Svc/Types/TlmPacketizerTypes, Svc/WatchDog, TestDeploymentsProject/Ref/PingReceiver, TestDeploymentsProject/Ref/RecvBuffApp, TestDeploymentsProject/Ref/SendBuffApp, TestDeploymentsProject/Ref/Top, TestDeploymentsProject/Ref/TypeDemo, cmake/test/data/TestDeployment/TestBuildAutocoder, cmake/test/data/TestDeployment/TestChainedAutocoder, cmake/test/data/TestDeployment/TestHeaderAutocoder, cmake/test/data/TestDeployment/TestTargetAutocoder, cmake/test/data/test-fprime-library/TestLibrary/TestComponent, cmake/test/data/test-fprime-library2/TestLibrary2/TestComponent

Abhishek21g · 2026-07-13T22:47:10Z

Follow-ups for the automated review findings:

Security / design (getFileSize before sandbox): fixed in 453c44a — open via Os::SandboxedFile first, then m_osFile.size().
CPP-29 getcwd in assert: fixed in 7e8c24b.
SDD sandbox / SendFile / SourceOutOfSandbox: fixed in 7e8c24b.
Human design adjudication: still needs a maintainer ack (thomas-bc previously approved; that review was dismissed by the follow-up commits).

CI workflows on this fork PR currently show no checks pending maintainer approval — happy to wait on a re-run / approve workflows when convenient.

thomas-bc · 2026-07-18T00:29:11Z

CCB approved

feat(FileDownlink): add sandboxed reads via Os::SandboxedFile

f2ca9ad

Mirror FileUplink write-side confinement for commanded downlink paths. Adds configure(directory), SourceOutOfSandbox event, and unit coverage. Fixes nasa#5398

thomas-bc previously approved these changes Jul 13, 2026

View reviewed changes

thomas-bc self-requested a review July 13, 2026 17:27

lestarch-autobot reviewed Jul 13, 2026

View reviewed changes

fix(FileDownlink): query file size only after sandbox validation

453c44a

Avoid unrestricted Os::FileSystem::getFileSize on commanded paths before the SandboxedFile open check. Open first, then use SandboxedFile::size(). Addresses security review on nasa#5404.

Abhishek21g dismissed thomas-bc’s stale review via 453c44a July 13, 2026 17:33

lestarch-autobot reviewed Jul 13, 2026

View reviewed changes

lestarch-autobot suggested changes Jul 13, 2026

View reviewed changes

fix(FileDownlink): address review feedback for sandbox PR

7e8c24b

- CPP-29: initialize cwd buffer before FW_ASSERT(getcwd) - Document SourceOutOfSandbox and port/command sandbox contract in SDD - Apply clang-format

thomas-bc added the CCB PR needs to go through CCB process label Jul 13, 2026

thomas-bc removed the CCB PR needs to go through CCB process label Jul 18, 2026

		char cwd[Os::FilePathUtils::MAX_PATH_LENGTH];
		FW_ASSERT(getcwd(cwd, sizeof(cwd)) != nullptr);

-    char cwd[Os::FilePathUtils::MAX_PATH_LENGTH];
-    FW_ASSERT(getcwd(cwd, sizeof(cwd)) != nullptr);
+char cwd[Os::FilePathUtils::MAX_PATH_LENGTH] = {};
+const char* cwdResult = getcwd(cwd, sizeof(cwd));
+FW_ASSERT(cwdResult != nullptr);

	When configured, all commanded source paths are validated against the sandbox directory before reading.
	When configured, all source paths supplied through `SendFile`/`SendPartial` commands or the `SendFile` port are validated against the sandbox directory before reading; rejected paths emit `SourceOutOfSandbox`.

Uh oh!

Conversation

Abhishek21g commented Jul 13, 2026

Summary

Test plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lestarch-autobot left a comment

Choose a reason for hiding this comment

Automated review summary (run 1)

Per-agent results

Merge readiness

Uh oh!

github-actions Bot commented Jul 13, 2026

Coverage report — base devel

Regressions

Modules changed

Modules without UTs

Uh oh!

Abhishek21g commented Jul 13, 2026

Uh oh!

thomas-bc commented Jul 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Coverage report — base `devel`