CfdpManager: add per-channel rx_dir to confine received file destinat… - #5801
bitWarrior wants to merge 5 commits into
Conversation
Coverage report — base
|
| Module | Line | Δ | Function | Δ | Branch | Δ |
|---|---|---|---|---|---|---|
Os/Posix |
70.64 | -0.39 | 87.50 | +0.00 | 50.67 | -0.22 |
Os/Generic |
90.14 | -0.20 | 89.13 | +0.00 | 74.90 | -0.77 |
Svc/Ccsds/CfdpManager |
89.32 | +0.05 | 96.03 | +0.02 | 74.62 | +0.32 |
Os |
19.92 | +0.15 | 22.58 | +0.13 | 17.60 | +0.19 |
Utils |
46.68 | +0.33 | 45.71 | +0.00 | 49.44 | +0.56 |
New modules
| Module | Line | Function | Branch |
|---|---|---|---|
Fw/Prm |
0.00 | 0.00 | 0.00 |
Svc/TimeConverter |
99.12 | 100.00 | 97.10 |
Modules without UTs
CFDP/Checksum/GTest, Drv/LinuxGpioDriver, Drv/LinuxI2cDriver, Drv/LinuxSpiDriver, Drv/Ports/DataTypes, Drv/PosixUartDriver, 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/sync, FppTestProject/FppTest/topology/top_ports, FppTestProject/FppTest/topology/types, Fw/Com, Fw/Comp, Fw/FilePacket/GTest, Fw/Fpy, Fw/Obj, Fw/Port, Fw/Sm, Fw/Test, Fw/Types/GTest, Os/Models, Svc/Ccsds/Types, Svc/Ccsds/Utils, Svc/FatalHandler, Svc/Subtopologies/CdhCore, Svc/Subtopologies/ComCcsds, Svc/Subtopologies/ComCcsdsSdls, Svc/Subtopologies/ComFprime, Svc/Subtopologies/ComLoggerTee, Svc/Subtopologies/DataProducts, Svc/Subtopologies/DpCompression, Svc/Subtopologies/FileHandling, Svc/Subtopologies/FileHandlingCfdp, Svc/Subtopologies/FileHandlingCfdp/FileHandlingCfdpConfig, TestDeploymentsProject/Ref/DpDemo, TestDeploymentsProject/Ref/PingReceiver, TestDeploymentsProject/Ref/RecvBuffApp, TestDeploymentsProject/Ref/SendBuffApp, TestDeploymentsProject/Ref/Top, TestDeploymentsProject/Ref/TypeDemo, cmake/test/data/TestConfigDeployment, cmake/test/data/TestDeployment/TestBuildAutocoder, cmake/test/data/TestDeployment/TestDuplicateDirective, cmake/test/data/TestDeployment/TestHeaderAutocoder, cmake/test/data/TestDeployment/TestLinkDepends, cmake/test/data/TestDeployment/TestRelative, cmake/test/data/test-fprime-library/TestLibrary/TestComponent, cmake/test/data/test-fprime-library2/TestLibrary2/TestComponent
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
The containment check is lexical only: resolvePath() collapses ./.., but it does not resolve symlinks. A destination such as rx_dir/link/file can therefore pass checkContainment() even if link points outside rx_dir, and later file operations will follow it. Could this use a symlink-safe filesystem primitive or validate the real parent path before creating/moving the received file?
|
@Brian-Campuzano if you have a moment, would you please review this? I plan to look through it too, but it was your contribution. |
| | ChannelConfig.tmp_dir | Directory path for storing temporary files during receive (RX) transactions. Files are written here during transfer and moved to their final destination upon successful completion | | ||
| | ChannelConfig.tmp_dir | Directory path for temporary files during receive (RX) transactions. Only used for Class 2 transfers whose File Data PDUs arrive before the Metadata PDU; the temporary file is moved to the final destination once Metadata arrives | | ||
| | ChannelConfig.fail_dir | Directory path for storing files from polling operations that failed to transfer successfully. If empty or if the move fails, files are deleted from the filesystem | | ||
| | ChannelConfig.rx_dir | Receive sandbox directory. If non-empty, destination paths from incoming Metadata PDUs are resolved against this directory and rejected if they resolve outside it (see Security Considerations). If empty (default), destination paths are accepted as-is | |
There was a problem hiding this comment.
[Documentation] must fix (Anchored above the offending line; the diff does not include line 514.) The RX Transaction Events table does not list the new RxDestPathRejected event.
Events.fppi adds RxDestPathRejected (warning high) and the Security Considerations prose references it, but the SDD's event catalog (the table operators consult) was not updated. Add a row, e.g. after RxFileCreateFailed:
| RxDestPathRejected | warning high | RX transaction rejected a Metadata PDU destination path that resolves outside the channel's rx_dir |
| > file set by `prmDb.configure(<file name>)` (which is **not** itself a sandbox). Additionally, | ||
| > CFDP file transfers via `cfdpManager` are not sandboxed: ground-commanded transactions may read | ||
| > or write any path accessible to the process. |
There was a problem hiding this comment.
[Documentation] suggestion The pre-existing "not sandboxed ... may read or write any path" sentence is now stale and duplicated by the new paragraph below it.
With rx_dir available, received-file writes are confinable, so the old unconditional statement contradicts the paragraph this PR adds (which also starts with "Additionally"). Drop the superseded sentence and let the new paragraph carry the note.
| > file set by `prmDb.configure(<file name>)` (which is **not** itself a sandbox). Additionally, | |
| > CFDP file transfers via `cfdpManager` are not sandboxed: ground-commanded transactions may read | |
| > or write any path accessible to the process. | |
| > `FileHandlingCfdp::prmDb.configureSandbox(<directory>)`, where the directory contains the store | |
| > file set by `prmDb.configure(<file name>)` (which is **not** itself a sandbox). |
| > by default: ground-commanded transactions may read any path accessible to the process, and | ||
| > received files are written wherever the incoming Metadata PDU says. Set the per-channel | ||
| > `cfdpManager.ChannelConfig[n].rx_dir` parameter to confine received files to a directory | ||
| > (see the CfdpManager SDD, *Security Considerations*). |
There was a problem hiding this comment.
[Documentation] could fix (Anchored above the offending line; the diff does not include line 117.) §3.2 "Runtime Parameters (per channel)" lists only "Temporary, failure, and move directories".
The per-channel parameter set now also includes the receive sandbox directory (rx_dir); the bullet should read e.g. "Temporary, failure, move, and receive-sandbox (rx_dir) directories for file operations".
|
|
||
| For mission deployments, ensure radio links employ hardware encryption or cryptographic authentication, ground systems implement proper authentication and authorization controls, and operational procedures include verification of file paths before commanding transfers. | ||
| - When `rx_dir` is **empty (the default)** destination paths are accepted as-is, consistent with the CCSDS 727.0-B-5 CFDP standard's assumption of operation over authenticated channels. This is a fail-open default chosen for backward compatibility. | ||
| - When `rx_dir` is **set**, every received destination path is canonicalized with `Os::FilePathUtils::resolvePath` (relative paths are resolved against `rx_dir`; `.` and `..` segments are collapsed textually, without following symlinks) and must remain inside `rx_dir` per `Os::FilePathUtils::checkContainment`. The check is applied once, in `Engine::recvMd`, before the path is stored in the transaction history, so every downstream file operation (`open`, `moveFile`, `removeFile`) only ever sees a validated path. A rejected path raises the `RxDestPathRejected` warning, increments the channel's `faultFileOpen` counter, and terminates the transaction without opening, moving, or removing any file (for a Class 2 transfer whose temporary file was already open, the transaction is finished with `FILESTORE_REJECTION` against the temporary file only). This keeps a mistyped or unexpected destination from writing outside the area the mission has set aside for received files. |
There was a problem hiding this comment.
[Documentation] could fix (Anchored above the offending line; the diff does not include line 668.) The Telemetry table still describes faultFileOpen as "Number of file open failures" only.
Engine::recvMd now also increments faultFileOpen when a destination path is rejected by rx_dir (no open is attempted). Extend the row so operators can interpret the counter, e.g. "Number of file open failures, plus Metadata PDU destination paths rejected by rx_dir".
| #include <string.h> | ||
| #include <Fw/Types/StringUtils.hpp> | ||
| #include <Os/FilePathUtils.hpp> | ||
| #include <new> | ||
|
|
||
| #include <Fw/Types/StringUtils.hpp> |
There was a problem hiding this comment.
[C++ Design] suggestion CPP-26 (style: include grouping): Fw/Types/StringUtils.hpp is now included twice, and the new Os/FilePathUtils.hpp sits in the system-header group ahead of <new> instead of next to Os/FileSystem.hpp.
| #include <string.h> | |
| #include <Fw/Types/StringUtils.hpp> | |
| #include <Os/FilePathUtils.hpp> | |
| #include <new> | |
| #include <Fw/Types/StringUtils.hpp> | |
| #include <string.h> | |
| #include <new> | |
| #include <Fw/Types/StringUtils.hpp> | |
| #include <Os/FilePathUtils.hpp> |
There was a problem hiding this comment.
Fixed at 0e11519: duplicate include dropped and regrouped with the F Prime headers.
There was a problem hiding this comment.
(Corrected text, previous reply lost its code spans.) Fixed at 0e11519: duplicate Fw/Types/StringUtils.hpp include dropped and Os/FilePathUtils.hpp regrouped with the F Prime headers.
| const FwSizeType rootLen = Fw::StringUtils::string_length(root, sizeof(root)); | ||
| if ((rootLen == 0) || (rootLen + 2 > sizeof(root))) { | ||
| return false; | ||
| } | ||
| if (root[rootLen - 1] != '/') { | ||
| root[rootLen] = '/'; | ||
| root[rootLen + 1] = '\0'; | ||
| } |
There was a problem hiding this comment.
[C++ Design] could fix CPP-33 (extract shared logic): this resolve-then-ensure-trailing-/ block is a line-for-line copy of Os::SandboxedFile::configureSandbox (Os/SandboxedFile.cpp, the resolveFromCwd + '/' append). checkContainment requires the trailing /, so every caller will need it.
Consider a shared helper in Os::FilePathUtils (e.g. resolveDirectoryFromCwd(const char*, char*, FwSizeType) that returns the canonical dir with trailing /) and use it from both sites.
There was a problem hiding this comment.
Fixed at 0e11519: the resolve-then-trailing- block now lives in and is used by both and .
There was a problem hiding this comment.
(Corrected text, previous reply lost its code spans.) Fixed at 0e11519: the resolve-then-trailing-/ block now lives in Os::FilePathUtils::resolveDirectoryFromCwd, used by both Engine::validateRxDestPath and Os::SandboxedFile::configure.
| this->m_manager->log_ACTIVITY_LO_MetadataReceived(txn->m_history->fnames.src_filename, | ||
| txn->m_history->fnames.dst_filename, txn->m_history->seq_num); |
There was a problem hiding this comment.
[C++ Design] could fix CPP-31 (silent truncation, display): dst_filename is now the canonical absolute path, up to Os::FilePathUtils::MAX_PATH_LENGTH (FileNameStringSize, 240 by default), but MetadataReceived.destFile is string size MaxFilePathSize (200 by default). Before this PR the PDU bound kept it ≤ MaxFilePathSize; now a long rx_dir + relative name silently truncates in the event (and in the other dst_filename events, e.g. RxFileCreateFailed).
Display-only, so low severity; sizing those event args with FileNameStringSize, or rejecting in validateRxDestPath when resolved exceeds MaxFilePathSize, would close it.
There was a problem hiding this comment.
Fixed at 0e11519: rejects a canonical path longer than , so and the events sized to it no longer truncate.
There was a problem hiding this comment.
(Corrected text, previous reply lost its code spans.) Fixed at 0e11519: validateRxDestPath rejects a canonical path longer than Cfdp::MaxFilePathSize, so dst_filename and the events sized to it no longer truncate.
| this->m_manager->log_WARNING_HI_RxDestPathRejected(txn->m_chan_num, txn->m_history->src_eid, | ||
| txn->m_history->seq_num, dst, | ||
| this->m_manager->getRxDirParam(txn->m_chan_num)); | ||
| this->m_manager->incrementFaultFileOpen(txn->m_chan_num); |
There was a problem hiding this comment.
[Correctness] suggestion Metadata-first rejection leaves txn_stat at UNDEFINED, so the rejected transfer is reported as completed.
When recvMd() rejects the path from recvInit(), the transaction stays in INIT and finishTransaction() runs with txn_stat == TXN_STATUS_UNDEFINED; TxnStatusIsError() is false, so RxFileTransferCompleted (ACTIVITY_HI) is emitted for a transfer that wrote nothing, and RxFileTransferFailed never fires. The late-metadata path (r2RecvMd) sets FILESTORE_REJECTION, so the two rejection paths report inconsistently. Setting the status inside recvMd() covers both callers (setTxnStatus is idempotent once an error is set, and r2SetFinTxnStatus still marks the FIN). Reachable in testRxDestPathRejectedEvent by adding ASSERT_EVENTS_RxFileTransferCompleted_SIZE(0).
| this->m_manager->incrementFaultFileOpen(txn->m_chan_num); | |
| this->m_manager->incrementFaultFileOpen(txn->m_chan_num); | |
| this->setTxnStatus(txn, TxnStatus::TXN_STATUS_FILESTORE_REJECTION); |
| } | ||
| // else: destination path rejected. State stays INIT so the transaction is finished | ||
| // below (parked in HOLD until the inactivity timer recycles it) without any file | ||
| // ever being opened. |
There was a problem hiding this comment.
[Security] suggestion Rejected Class 1/metadata-first destination is finished with a non-error txn_stat, so finishTransaction emits RxFileTransferCompleted for a sandbox-escape attempt.
recvMd returns false without calling setTxnStatus, and the transaction is then finished from INIT with txn_stat still non-error. finishTransaction therefore logs ACTIVITY_HI RxFileTransferCompleted (empty dst_filename) right after WARNING_HI RxDestPathRejected, and a Class 2 sender is never told the transfer failed. Marking the transaction FILESTORE_REJECTION (as the late-metadata path in r2RecvMd already does) makes the audit trail report the rejection as a failure.
| } | |
| // else: destination path rejected. State stays INIT so the transaction is finished | |
| // below (parked in HOLD until the inactivity timer recycles it) without any file | |
| // ever being opened. | |
| } else { | |
| // Rejected destination: report the transfer as failed, not completed | |
| this->setTxnStatus(txn, TxnStatus::TXN_STATUS_FILESTORE_REJECTION); | |
| } |
There was a problem hiding this comment.
[Summary] Severity escalated to must fix — the Design reviewer flagged this same issue at must-fix severity (duplicate thread consolidated here: #5801 (comment)), and the Operational reviewer concurred on that thread. This thread is the canonical home of the finding.
There was a problem hiding this comment.
[Operational] Fixed in 0e11519. The concurrence's remaining point (no FIN on the Metadata-first Class 2 path) is tracked as a separate suggestion on docs/sdd.md.
| #include <Fw/Types/StringUtils.hpp> | ||
| #include <Os/FilePathUtils.hpp> | ||
| #include <new> | ||
|
|
||
| #include <Fw/Types/StringUtils.hpp> |
There was a problem hiding this comment.
[Maintainability] suggestion #include <Fw/Types/StringUtils.hpp> is added here even though the same include already sits four lines below, and both new F Prime headers were inserted into the C/C++ standard-header group rather than the F Prime group used by the rest of this file.
A duplicated include is a second line to keep in sync when the dependency changes, and the split grouping leaves the next reader unsure which block a new F Prime include belongs in.
| #include <Fw/Types/StringUtils.hpp> | |
| #include <Os/FilePathUtils.hpp> | |
| #include <new> | |
| #include <Fw/Types/StringUtils.hpp> | |
| #include <new> | |
| #include <Fw/Types/StringUtils.hpp> | |
| #include <Os/FilePathUtils.hpp> |
| bool Engine::recvMd(Transaction* txn, const MetadataPdu& md) { | ||
| /* store the expected file size in transaction */ | ||
| txn->m_fsize = md.getFileSize(); | ||
|
|
||
| /* store the filenames in transaction - validation already done during deserialization */ | ||
| /* structural validation (length, non-empty) already done during deserialization */ | ||
| txn->m_history->fnames.src_filename = md.getSourceFilename(); | ||
| txn->m_history->fnames.dst_filename = md.getDestFilename(); | ||
|
|
||
| /* the destination path comes from the remote entity: canonicalize it and confirm it lies within | ||
| * the configured receive directory before any file operation (open/move/remove) consumes it */ | ||
| Fw::String dst = md.getDestFilename(); | ||
| if (!this->validateRxDestPath(txn->m_chan_num, dst)) { | ||
| this->m_manager->log_WARNING_HI_RxDestPathRejected(txn->m_chan_num, txn->m_history->src_eid, | ||
| txn->m_history->seq_num, dst, | ||
| this->m_manager->getRxDirParam(txn->m_chan_num)); | ||
| this->m_manager->incrementFaultFileOpen(txn->m_chan_num); | ||
| // Never leave a rejected path where a later file operation could consume it | ||
| txn->m_history->fnames.dst_filename = ""; | ||
| return false; | ||
| } | ||
| txn->m_history->fnames.dst_filename = dst; |
There was a problem hiding this comment.
[Maintainability] suggestion recvMd commits m_fsize and src_filename into the transaction before validating the destination, so a false return leaves the transaction partially updated from a rejected Metadata PDU.
Only dst_filename is undone — here, plus the manual restore added in Transaction::r2RecvMd. Nothing structurally ties that rollback to the writes, so a third caller, or one more field written above the check, silently inherits remote data from a PDU the component refused. Validating first makes "rejected" mean "nothing was committed" and removes the need for callers to compensate.
| bool Engine::recvMd(Transaction* txn, const MetadataPdu& md) { | |
| /* store the expected file size in transaction */ | |
| txn->m_fsize = md.getFileSize(); | |
| /* store the filenames in transaction - validation already done during deserialization */ | |
| /* structural validation (length, non-empty) already done during deserialization */ | |
| txn->m_history->fnames.src_filename = md.getSourceFilename(); | |
| txn->m_history->fnames.dst_filename = md.getDestFilename(); | |
| /* the destination path comes from the remote entity: canonicalize it and confirm it lies within | |
| * the configured receive directory before any file operation (open/move/remove) consumes it */ | |
| Fw::String dst = md.getDestFilename(); | |
| if (!this->validateRxDestPath(txn->m_chan_num, dst)) { | |
| this->m_manager->log_WARNING_HI_RxDestPathRejected(txn->m_chan_num, txn->m_history->src_eid, | |
| txn->m_history->seq_num, dst, | |
| this->m_manager->getRxDirParam(txn->m_chan_num)); | |
| this->m_manager->incrementFaultFileOpen(txn->m_chan_num); | |
| // Never leave a rejected path where a later file operation could consume it | |
| txn->m_history->fnames.dst_filename = ""; | |
| return false; | |
| } | |
| txn->m_history->fnames.dst_filename = dst; | |
| bool Engine::recvMd(Transaction* txn, const MetadataPdu& md) { | |
| /* the destination path comes from the remote entity: canonicalize it and confirm it lies within | |
| * the configured receive directory before any transaction state or file operation consumes it */ | |
| Fw::String dst = md.getDestFilename(); | |
| if (!this->validateRxDestPath(txn->m_chan_num, dst)) { | |
| this->m_manager->log_WARNING_HI_RxDestPathRejected(txn->m_chan_num, txn->m_history->src_eid, | |
| txn->m_history->seq_num, dst, | |
| this->m_manager->getRxDirParam(txn->m_chan_num)); | |
| this->m_manager->incrementFaultFileOpen(txn->m_chan_num); | |
| // Never leave a rejected path where a later file operation could consume it | |
| txn->m_history->fnames.dst_filename = ""; | |
| return false; | |
| } | |
| /* store the expected file size in transaction */ | |
| txn->m_fsize = md.getFileSize(); | |
| /* structural validation (length, non-empty) already done during deserialization */ | |
| txn->m_history->fnames.src_filename = md.getSourceFilename(); | |
| txn->m_history->fnames.dst_filename = dst; |
| * was rejected (event logged, fault counted, dst_filename left empty) | ||
| */ | ||
| void recvMd(Transaction* txn, const MetadataPdu& pdu); | ||
| bool recvMd(Transaction* txn, const MetadataPdu& pdu); |
There was a problem hiding this comment.
[Maintainability] could fix recvMd changes from void to a bare bool, while its four sibling PDU handlers in this class (recvFd, recvEof, recvFin, recvNak) all return Status::T, and the one other bool-returning handler here (recvInit) uses true to mean "re-dispatch needed".
Cost: a reader of if (this->recvMd(txn, md)) must open the header to learn which of the two bool conventions applies, and the value cannot express why the PDU was refused when a second rejection reason is added. Status::T keeps the handler family uniform; the two call sites (Engine::recvInit, Transaction::r2RecvMd) would compare against Cfdp::Status::SUCCESS.
| ASSERT_STREQ(dstFile, this->eventHistory_RxDestPathRejected->at(0).filename.toChar()); | ||
| ASSERT_STREQ(rxDir, this->eventHistory_RxDestPathRejected->at(0).rxDir.toChar()); | ||
| ASSERT_EQ(transactionSeq, this->eventHistory_RxDestPathRejected->at(0).seqNum); |
There was a problem hiding this comment.
[Test Quality] suggestion The new RxDestPathRejected event declares five arguments, but only filename, rxDir and seqNum are asserted here (and only filename on the second rejection at L278); chanId and srcEid are never checked, so a swapped or hard-coded channel/EID argument in Engine::recvMd would pass. The autocoded full-payload macro covers all five at once:
| ASSERT_STREQ(dstFile, this->eventHistory_RxDestPathRejected->at(0).filename.toChar()); | |
| ASSERT_STREQ(rxDir, this->eventHistory_RxDestPathRejected->at(0).rxDir.toChar()); | |
| ASSERT_EQ(transactionSeq, this->eventHistory_RxDestPathRejected->at(0).seqNum); | |
| ASSERT_EVENTS_RxDestPathRejected(0, channelId, TEST_GROUND_EID, transactionSeq, dstFile, rxDir); |
(best-effort fix; verify before applying)
| EXPECT_STREQ("", txn->m_history->fnames.dst_filename.toChar()); | ||
| } |
There was a problem hiding this comment.
[Test Quality] suggestion Engine::validateRxDestPath has four rejection returns (root resolve, root length, resolvePath of the joined path, checkContainment); both sub-cases here and the late-metadata test only reach the last one. A MaxFilePathSize (200) destination joined to rx_dir exceeds Os::FilePathUtils::MAX_PATH_LENGTH (240), so the length-driven rejection is reachable from the ground but untested, and a regression there (e.g. writing root unterminated) would go unnoticed. Add a max-length sub-case:
| EXPECT_STREQ("", txn->m_history->fnames.dst_filename.toChar()); | |
| } | |
| EXPECT_STREQ("", txn->m_history->fnames.dst_filename.toChar()); | |
| // Max-length destination: rejected by the joined-path length check, not by containment | |
| char longDst[Cfdp::MaxFilePathSize + 1]; | |
| (void)memset(longDst, 'a', sizeof(longDst) - 1); | |
| longDst[sizeof(longDst) - 1] = '\0'; | |
| this->clearEvents(); | |
| this->sendMetadataPdu(channelId, TEST_GROUND_EID, this->component.getLocalEidParam(), transactionSeq + 2, 100, | |
| srcFile, longDst, Cfdp::Class::CLASS_1, 0); | |
| this->component.doDispatch(); | |
| ASSERT_EVENTS_RxDestPathRejected_SIZE(1); | |
| ASSERT_EVENTS_MetadataReceived_SIZE(0); | |
| txn = this->findTransaction(channelId, transactionSeq + 2); | |
| ASSERT_NE(nullptr, txn); | |
| EXPECT_FALSE(txn->m_fd.isOpen()); | |
| EXPECT_STREQ("", txn->m_history->fnames.dst_filename.toChar()); | |
| } |
(best-effort fix; verify before applying)
| // Receive directory (rx_dir) | ||
| // ---------------------------------------------------------------------- | ||
|
|
||
| void CfdpManagerTester::setChannel0RxDir(const char* rxDir) { |
There was a problem hiding this comment.
[Test Quality] could fix rx_dir is a per-channel parameter (ChannelArrayParams[NumChannels]) and getRxDirParam(txn->m_chan_num) indexes it by the receiving channel, but every new test configures and receives on channel 0 only. A test that sets rx_dir on channel 0 while leaving channel 1 empty, then receives an outside path on TEST_CHANNEL_ID_1, would confirm the confinement is per-channel and that the empty-string default is genuinely unrestricted (currently the default path is only covered indirectly by pre-existing tests).
| this->m_manager->log_WARNING_HI_RxDestPathRejected(txn->m_chan_num, txn->m_history->src_eid, | ||
| txn->m_history->seq_num, dst, | ||
| this->m_manager->getRxDirParam(txn->m_chan_num)); | ||
| this->m_manager->incrementFaultFileOpen(txn->m_chan_num); |
There was a problem hiding this comment.
[Test Quality] could fix The rejection path now bumps faultFileOpen, which is the only telemetry-visible signal of a rejected destination, but none of the three new tests read tlmHistory_ChannelTelemetry. After the rejection, one invoke_to_run1Hz(0, 0) + doDispatch() (as testTxFileTransferStartedEvent does) followed by EXPECT_EQ(1u, tlm[TEST_CHANNEL_ID_0].get_faultFileOpen()) would pin the counter; without it, dropping this line would not fail any test.
| } | ||
|
|
||
| void Engine::recvMd(Transaction* txn, const MetadataPdu& md) { | ||
| bool Engine::validateRxDestPath(U8 chan_num, Fw::String& path) { |
There was a problem hiding this comment.
[Design] suggestion Receive-path confinement is re-implemented here instead of reusing Os::SandboxedFile.
Os::SandboxedFile already wraps Os::File with configure() plus resolve-and-contain on every open(), is fail-closed, and carries the documented (textual, no-symlink) threat model this helper repeats; Svc::FileUplink, Svc::FileDownlink and Svc::PrmDb all use it. Forking the same resolve/containment sequence into Engine means two copies of the confinement rule can drift, and the guarantee now holds only at Metadata-PDU time rather than at each file operation. The PR description's rationale (the descriptor is shared with the transmit path, and moveFile/removeFile are not covered) makes this a partial-overlap case, where the preferred F Prime direction is to extend the existing facility — e.g. expose the validation entry point and sandboxed move/remove on Os::SandboxedFile and give Transaction::m_fd that type — rather than to fork the check into a component.
cc @LeStarch @thomas-bc — low-confidence finding, please confirm.
| } | ||
| // else: destination path rejected. State stays INIT so the transaction is finished | ||
| // below (parked in HOLD until the inactivity timer recycles it) without any file | ||
| // ever being opened. |
There was a problem hiding this comment.
[Design] must fix A metadata-first rejection ends the transaction with no failure status, so a rejected reception is reported as a completed one.
When recvMd() returns false the state stays INIT and recvInit() falls through to finishTransaction(txn, false) with m_history->txn_stat still TXN_STATUS_UNDEFINED. TxnStatusIsError() is false for that value, so finishTransaction() takes the success branch and emits ACTIVITY_HI RxFileTransferCompleted (with an empty destination filename) for a transfer that was refused, and a port-initiated notification would report STATUS_OK. The late-metadata path in Transaction::r2RecvMd does set FILESTORE_REJECTION; the two rejection paths should agree, and for Class 2 the peer also learns nothing here and keeps retransmitting until its ACK/inactivity limits.
| } | |
| // else: destination path rejected. State stays INIT so the transaction is finished | |
| // below (parked in HOLD until the inactivity timer recycles it) without any file | |
| // ever being opened. | |
| } else { | |
| // Rejected destination is a filestore failure, not a completed reception | |
| this->setTxnStatus(txn, TxnStatus::TXN_STATUS_FILESTORE_REJECTION); | |
| } |
There was a problem hiding this comment.
[Operational] Concur — also in scope for failure-path consequences: a Class 2 peer gets no FIN here, so it retransmits EOF for ack_limit x ack_timer = 10 x 2 s = 20 s and its file-data PDUs land in HOLD as recvSpurious, while ground sees ACTIVITY_HI RxFileTransferCompleted for a refused transfer.
There was a problem hiding this comment.
[Summary] Duplicate — consolidated into #5801 (comment).
There was a problem hiding this comment.
[Design] Fixed in 0e11519. recvMd now sets FILESTORE_REJECTION before returning, so finishTransaction emits RxFileTransferFailed and port-initiated callers see STATUS_ERROR.
| max_outgoing_pdus_per_cycle: U32 @< Maximum number of PDUs to send per cycle per channel for throttling | ||
| tmp_dir: string size MaxFilePathSize @< Temporary directory for uplink file reception | ||
| fail_dir: string size MaxFilePathSize @< Directory for failed poll files | ||
| rx_dir: string size MaxFilePathSize @< If non-empty, received file destinations must resolve inside this directory |
There was a problem hiding this comment.
[Design] Human design adjudication required. rx_dir introduces a second, component-local file-confinement mechanism with semantics that differ from the framework's.
The framework's answer to remotely supplied destination paths is Os::SandboxedFile: fail-closed, enforced at open(), already adopted by FileUplink, FileDownlink and PrmDb. This parameter instead makes confinement a per-channel CFDP parameter that is fail-open by default and enforced once, at Metadata-PDU handling. Both choices are defensible, but they are not the same guarantee, and the SDD describes the new one as mirroring Os::SandboxedFile when it does not. The decision determines the public parameter surface (ChannelParams), the security section's threat model, and per-deployment operational guidance, so a design owner should settle whether F Prime wants one shared fail-closed mechanism (extended to cover CFDP's move/remove and shared descriptor) or a per-component opt-in policy before the details are reviewed further.
cc @LeStarch @thomas-bc — design needs human adjudication before deeper review.
|
|
||
| For mission deployments, ensure radio links employ hardware encryption or cryptographic authentication, ground systems implement proper authentication and authorization controls, and operational procedures include verification of file paths before commanding transfers. | ||
| - When `rx_dir` is **empty (the default)** destination paths are accepted as-is, consistent with the CCSDS 727.0-B-5 CFDP standard's assumption of operation over authenticated channels. This is a fail-open default chosen for backward compatibility. | ||
| - When `rx_dir` is **set**, every received destination path is canonicalized with `Os::FilePathUtils::resolvePath` (relative paths are resolved against `rx_dir`; `.` and `..` segments are collapsed textually, without following symlinks) and must remain inside `rx_dir` per `Os::FilePathUtils::checkContainment`. The check is applied once, in `Engine::recvMd`, before the path is stored in the transaction history, so every downstream file operation (`open`, `moveFile`, `removeFile`) only ever sees a validated path. A rejected path raises the `RxDestPathRejected` warning, increments the channel's `faultFileOpen` counter, and terminates the transaction without opening, moving, or removing any file (for a Class 2 transfer whose temporary file was already open, the transaction is finished with `FILESTORE_REJECTION` against the temporary file only). This keeps a mistyped or unexpected destination from writing outside the area the mission has set aside for received files. |
There was a problem hiding this comment.
[Operational] suggestion With rx_dir set, the accepted destination-name length shrinks to MAX_PATH_LENGTH (240) - |rx_dir| - 2, and over-length or unresolvable paths are reported as "outside receive directory".
validateRxDestPath returns false on four causes (root unresolvable, root > 238 chars, joined path TOO_LONG, containment), and recvMd emits the same RxDestPathRejected text and faultFileOpen bump for all of them. At the legal extreme (rx_dir = MaxFilePathSize = 200 chars) only names <= 38 chars pass, so an operator who sizes filenames to the 200-char PDU limit sees every reception fail with an event that blames containment. Operational judgment call: the smallest remedy is documenting the combined bound and the shared event so ops can size rx_dir and diagnose it.
| - When `rx_dir` is **set**, every received destination path is canonicalized with `Os::FilePathUtils::resolvePath` (relative paths are resolved against `rx_dir`; `.` and `..` segments are collapsed textually, without following symlinks) and must remain inside `rx_dir` per `Os::FilePathUtils::checkContainment`. The check is applied once, in `Engine::recvMd`, before the path is stored in the transaction history, so every downstream file operation (`open`, `moveFile`, `removeFile`) only ever sees a validated path. A rejected path raises the `RxDestPathRejected` warning, increments the channel's `faultFileOpen` counter, and terminates the transaction without opening, moving, or removing any file (for a Class 2 transfer whose temporary file was already open, the transaction is finished with `FILESTORE_REJECTION` against the temporary file only). This keeps a mistyped or unexpected destination from writing outside the area the mission has set aside for received files. | |
| - When `rx_dir` is **set**, every received destination path is canonicalized with `Os::FilePathUtils::resolvePath` (relative paths are resolved against `rx_dir`; `.` and `..` segments are collapsed textually, without following symlinks) and must remain inside `rx_dir` per `Os::FilePathUtils::checkContainment`. The check is applied once, in `Engine::recvMd`, before the path is stored in the transaction history, so every downstream file operation (`open`, `moveFile`, `removeFile`) only ever sees a validated path. A rejected path raises the `RxDestPathRejected` warning, increments the channel's `faultFileOpen` counter, and terminates the transaction without opening, moving, or removing any file (for a Class 2 transfer whose temporary file was already open, the transaction is finished with `FILESTORE_REJECTION` against the temporary file only). This keeps a mistyped or unexpected destination from writing outside the area the mission has set aside for received files. The canonical path (`rx_dir` joined with the received name) must also fit in `Os::FilePathUtils::MAX_PATH_LENGTH` (`FileNameStringSize`, 240 by default), so a long `rx_dir` shortens the acceptable destination name (e.g. a 200-character `rx_dir` leaves 38 characters); over-length or unresolvable paths are reported with the same `RxDestPathRejected` event as a containment failure. |
| - When `rx_dir` is **empty (the default)** destination paths are accepted as-is, consistent with the CCSDS 727.0-B-5 CFDP standard's assumption of operation over authenticated channels. This is a fail-open default chosen for backward compatibility. | ||
| - When `rx_dir` is **set**, every received destination path is canonicalized with `Os::FilePathUtils::resolvePath` (relative paths are resolved against `rx_dir`; `.` and `..` segments are collapsed textually, without following symlinks) and must remain inside `rx_dir` per `Os::FilePathUtils::checkContainment`. The check is applied once, in `Engine::recvMd`, before the path is stored in the transaction history, so every downstream file operation (`open`, `moveFile`, `removeFile`) only ever sees a validated path. A rejected path raises the `RxDestPathRejected` warning, increments the channel's `faultFileOpen` counter, and terminates the transaction without opening, moving, or removing any file (for a Class 2 transfer whose temporary file was already open, the transaction is finished with `FILESTORE_REJECTION` against the temporary file only). This keeps a mistyped or unexpected destination from writing outside the area the mission has set aside for received files. | ||
|
|
||
| Deployments are encouraged to set `rx_dir` on every receiving channel. In all deployments, ensure radio links employ hardware encryption or cryptographic authentication, ground systems implement proper authentication and authorization controls, and operational procedures include verification of file paths before commanding transfers. |
There was a problem hiding this comment.
[Operational] could fix A relative rx_dir is re-resolved against the process CWD on every Metadata PDU, and fails closed where getWorkingDirectory is unsupported.
validateRxDestPath calls resolveFromCwd per PDU, so the sandbox root follows whatever CWD the launcher or a later chdir leaves (the unit tests use test/ut/output/rxbox, i.e. relative). On the Stub Os::FileSystem (NOT_SUPPORTED) every reception on that channel is rejected with an "outside receive directory" event. Operational judgment call: guidance to use an absolute path is the smallest remedy.
| Deployments are encouraged to set `rx_dir` on every receiving channel. In all deployments, ensure radio links employ hardware encryption or cryptographic authentication, ground systems implement proper authentication and authorization controls, and operational procedures include verification of file paths before commanding transfers. | |
| Deployments are encouraged to set `rx_dir` on every receiving channel, as an absolute path: a relative `rx_dir` is resolved against the process working directory at each Metadata PDU, and is rejected outright (every reception fails) on platforms whose `Os::FileSystem::getWorkingDirectory` is unsupported. In all deployments, ensure radio links employ hardware encryption or cryptographic authentication, ground systems implement proper authentication and authorization controls, and operational procedures include verification of file paths before commanding transfers. |
lestarch-autobot
left a comment
lestarch-autobot
left a comment
There was a problem hiding this comment.
Automated review summary (run 1)
Per-agent results
| Agent | must fix | suggestion | could fix | future work | outstanding | Verdict |
|---|---|---|---|---|---|---|
| Security Vulnerabilities | 0 | 1 | 2 | 0 | 0 | Go |
| Supply Chain / Runner Safety | 0 | 0 | 0 | 0 | 0 | Go |
| F Prime C/C++ Design | 0 | 1 | 3 | 0 | 4 | Go |
| Documentation Currency | 1 | 1 | 1 | 0 | 3 | No-Go |
| Design | 2 | 1 | 0 | 0 | 2 | No-Go |
| Architecture | 0 | 1 | 0 | 0 | 1 | Go |
| Test Quality | 0 | 2 | 1 | 0 | 3 | Go |
| Correctness | 0 | 2 | 1 | 0 | 4 | Go |
| Operational | 1 | 1 | 1 | 0 | 2 | No-Go |
| Maintainability | 0 | 1 | 1 | 0 | 3 | Go |
| CI safety | — | — | — | — | — | Go |
| Totals | 3 | 9 | 10 | 0 | 19 | No-Go |
Totals count the finding consolidated by the de-duplication post-pass once (Security/Design/Operational, Engine.cpp:630).
Duplicates consolidated this run: 1 (threads closed by the §5h post-pass)
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 (3)
Security Vulnerabilities
- Metadata-first / Class 1 destination rejection finishes the transaction with a non-error
txn_stat, soRxFileTransferCompletedis emitted for a refused transfer and a Class 2 peer is never told it failed (escalated to must fix by Design) — also: Design, Operational — #5801 (comment)
Documentation Currency
CfdpManager/docs/sdd.mdRX Transaction Events table is missing the newRxDestPathRejectedevent — #5801 (comment)
Design
- Human design adjudication required.
rx_dirintroduces a second, component-local file-confinement mechanism (fail-open by default, checked once at Metadata PDU) whose semantics differ fromOs::SandboxedFile; a design owner should settle which model F Prime wants before deeper review — #5801 (comment)
Merge readiness
Merge readiness: No-Go — Documentation Currency, Design, and Operational each have outstanding must-fix items (3 consolidated must-fix threads open), including a design question that needs human adjudication.
Thanks for hardening the CFDP receive path — the rx_dir confinement is a welcome addition; once the rejection status and doc table are squared away and the confinement model is settled, this will be in good shape.
…sandboxed-rx-path
…mmit, share the directory-resolve helper Addresses the review findings on nasa#5801: - A rejected Metadata-first or Class 1 destination left the transaction status undefined, so finishTransaction emitted RxFileTransferCompleted for a refused reception and a Class 2 peer was never told. recvMd now sets FILESTORE_REJECTION itself, covering both call paths. - recvMd validates the destination before writing anything to the transaction, so a rejected PDU commits no file size, source name, or destination, and r2RecvMd no longer needs to restore the temp path. - recvMd returns Cfdp::Status like its sibling PDU handlers instead of a bare bool. - The resolve-then-append-'/' block duplicated from SandboxedFile::configure moves into Os::FilePathUtils::resolveDirectoryFromCwd, used by both. - A canonical destination longer than MaxFilePathSize is rejected instead of being silently truncated in the transaction and events. - Includes regrouped; duplicate StringUtils include dropped. - SDD: RxDestPathRejected added to the RX event table; faultFileOpen row covers rejections; Security Considerations no longer claims to mirror Os::SandboxedFile and instead states the actual guarantee, the four rejection causes and the combined length bound, the textual/no-symlink threat model, and absolute-path guidance for rx_dir. FileHandlingCfdp SDD: stale unsandboxed sentence removed, per-channel directory bullet updated. - Tests: full-payload event assertion, RxFileTransferCompleted absent and RxFileTransferFailed present on rejection, no partial commit, faultFileOpen telemetry pinned, max-length rejection case, late-metadata path asserts no completion, new per-channel test (channel 0 confined, channel 1 unrestricted), and seven Os tests for resolveDirectoryFromCwd. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@LeStarch Thanks for pulling @Brian-Campuzano in. I've merged devel (the earlier red CI was the WasmSequencer fpp mismatch, not this change) and pushed fixes for the automated findings, including the must-fix: a rejected destination now sets FILESTORE_REJECTION, so it is reported as a failed reception and a Class 2 sender learns of it in the FIN. The metadata handler also validates before committing anything to the transaction, and the resolve-with-trailing-slash logic is now a shared One item needs a design owner: the review bot asks whether F Prime wants confinement here as a per-channel
Extending |
|
@sylvesterkaczmarek Thanks, that's right: containment is textual and does not follow symlinks. That is deliberately the same model as I've now written that assumption into the CfdpManager SDD's Security Considerations so operators see it next to |
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Thanks, this addresses the point I raised. The current SDD now explicitly states that rx_dir containment is textual, symlinks are not followed, and the threat model assumes untrusted actors cannot create symlinks inside rx_dir, matching Os::SandboxedFile. Under that documented threat model I do not have a remaining blocker on the symlink point. The current red Test job is in the unrelated Svc/SystemResources integration test, while the CFDP/unit, build, integration, formatting and analysis checks are green.
| } | ||
| // The canonical path is stored in the transaction and reported in events sized to | ||
| // MaxFilePathSize; reject rather than silently truncate a longer result | ||
| if (Fw::StringUtils::string_length(resolved, sizeof(resolved)) > Cfdp::MaxFilePathSize) { |
There was a problem hiding this comment.
[C++ Design] could fix CPP-35 (bool where a status enum fits): validateRxDestPath now folds four distinct rejection causes (rx_dir resolve failure, dest resolve failure, OUTSIDE_SANDBOX, and this new length bound) into a single bool, and RxDestPathRejected reports none of them. An operator seeing the event cannot tell a traversal attempt from a mis-configured rx_dir or a merely too-long name without reproducing the resolve by hand. Returning the Os::FilePathUtils::Status (or a small local enum) and carrying it as an extra event argument would make the cause visible at no runtime cost. Non-blocking: the path and rx_dir are already in the event.
| /* the destination path comes from the remote entity: canonicalize it and confirm it lies within | ||
| * the configured receive directory BEFORE anything from this PDU is committed to the transaction, | ||
| * so a rejection leaves the transaction exactly as it was */ |
There was a problem hiding this comment.
[Maintainability] suggestion Follow-up to #5801 (comment): the comment now says a rejection "leaves the transaction exactly as it was", but the rejection branch eight lines below calls setTxnStatus(txn, TXN_STATUS_FILESTORE_REJECTION).
The next engineer reading only the header comment will assume the transaction status is untouched on rejection and look elsewhere for why finishTransaction reports a failure. Say what is actually true: nothing from the PDU is committed.
| /* the destination path comes from the remote entity: canonicalize it and confirm it lies within | |
| * the configured receive directory BEFORE anything from this PDU is committed to the transaction, | |
| * so a rejection leaves the transaction exactly as it was */ | |
| /* the destination path comes from the remote entity: canonicalize it and confirm it lies within | |
| * the configured receive directory BEFORE anything from this PDU is committed to the transaction; | |
| * on rejection only the transaction status changes (FILESTORE_REJECTION), no PDU field is stored */ |
|
|
||
| For mission deployments, ensure radio links employ hardware encryption or cryptographic authentication, ground systems implement proper authentication and authorization controls, and operational procedures include verification of file paths before commanding transfers. | ||
| - When `rx_dir` is **empty (the default)** destination paths are accepted as-is, consistent with the CCSDS 727.0-B-5 CFDP standard's assumption of operation over authenticated channels. This is a fail-open default chosen for backward compatibility. | ||
| - When `rx_dir` is **set**, every received destination path is canonicalized with `Os::FilePathUtils::resolvePath` (relative paths are resolved against `rx_dir`; `.` and `..` segments are collapsed textually, without following symlinks) and must remain inside `rx_dir` per `Os::FilePathUtils::checkContainment`. The check is applied once, in `Engine::recvMd`, before anything from the Metadata PDU is committed to the transaction, so every downstream file operation (`open`, `moveFile`, `removeFile`) only ever sees a validated path and a rejected PDU leaves the transaction unmodified. A rejected path raises the `RxDestPathRejected` warning, increments the channel's `faultFileOpen` counter, marks the transaction `FILESTORE_REJECTION` so it is reported as a failed reception (and, for Class 2, refused to the sender in the FIN), and terminates it without opening, moving, or removing any file (for a Class 2 transfer whose temporary file was already open, the transaction is finished against the temporary file only). This keeps a mistyped or unexpected destination from writing outside the area the mission has set aside for received files. |
There was a problem hiding this comment.
[Operational] suggestion The SDD (and the Engine::recvMd comment) say a rejection is "refused to the sender in the FIN", but the Metadata-first Class 2 path sends no FIN.
recvInit leaves the state INIT and finishTransaction parks it in HOLD without setting send_fin; only r2RecvMd (late metadata) sets it. So a Class 2 sender whose destination is refused on the first PDU still transmits the entire file, then retransmits EOF for ack_limit x ack_timer = 10 x 2 s = 20 s (defaults) and ends with ACK_LIMIT_NO_EOF rather than FILESTORE_REJECTION, while the receiver logs its PDUs as recvSpurious. Operational judgment call: the smallest remedy is stating the actual behaviour and its link cost; sending a FIN from INIT would be the larger fix.
| - When `rx_dir` is **set**, every received destination path is canonicalized with `Os::FilePathUtils::resolvePath` (relative paths are resolved against `rx_dir`; `.` and `..` segments are collapsed textually, without following symlinks) and must remain inside `rx_dir` per `Os::FilePathUtils::checkContainment`. The check is applied once, in `Engine::recvMd`, before anything from the Metadata PDU is committed to the transaction, so every downstream file operation (`open`, `moveFile`, `removeFile`) only ever sees a validated path and a rejected PDU leaves the transaction unmodified. A rejected path raises the `RxDestPathRejected` warning, increments the channel's `faultFileOpen` counter, marks the transaction `FILESTORE_REJECTION` so it is reported as a failed reception (and, for Class 2, refused to the sender in the FIN), and terminates it without opening, moving, or removing any file (for a Class 2 transfer whose temporary file was already open, the transaction is finished against the temporary file only). This keeps a mistyped or unexpected destination from writing outside the area the mission has set aside for received files. | |
| - When `rx_dir` is **set**, every received destination path is canonicalized with `Os::FilePathUtils::resolvePath` (relative paths are resolved against `rx_dir`; `.` and `..` segments are collapsed textually, without following symlinks) and must remain inside `rx_dir` per `Os::FilePathUtils::checkContainment`. The check is applied once, in `Engine::recvMd`, before anything from the Metadata PDU is committed to the transaction, so every downstream file operation (`open`, `moveFile`, `removeFile`) only ever sees a validated path and a rejected PDU leaves the transaction unmodified. A rejected path raises the `RxDestPathRejected` warning, increments the channel's `faultFileOpen` counter, marks the transaction `FILESTORE_REJECTION` so it is reported as a failed reception (`RxFileTransferFailed`). Only a Class 2 transfer whose Metadata PDU arrives late is refused to the sender in a FIN; when the Metadata PDU is the first PDU the transaction is finished without a FIN, so a Class 2 sender still transmits the whole file, then retransmits EOF for `ack_limit` x `ack_timer` (10 x 2 s = 20 s by default) and fails with `ACK_LIMIT_NO_EOF` while the receiver counts those PDUs as `recvSpurious`. The transaction is terminated, and terminates it without opening, moving, or removing any file (for a Class 2 transfer whose temporary file was already open, the transaction is finished against the temporary file only). This keeps a mistyped or unexpected destination from writing outside the area the mission has set aside for received files. |
| ASSERT_EVENTS_MetadataReceived_SIZE(0); | ||
| ASSERT_EVENTS_RxFileCreateFailed_SIZE(0); | ||
| ASSERT_EVENTS_RxFileTransferCompleted_SIZE(0); | ||
| ASSERT_EVENTS_RxFileTransferFailed_SIZE(1); |
There was a problem hiding this comment.
[Test Quality] suggestion RxFileTransferFailed is asserted by count only. Its conditionCode is the ground-visible reason this fix introduced (FILESTORE_REJECTION); without pinning it, a regression that fails the transaction with a different status would still pass here.
| ASSERT_EVENTS_RxFileTransferFailed_SIZE(1); | |
| ASSERT_EVENTS_RxFileTransferFailed_SIZE(1); | |
| EXPECT_EQ(static_cast<U8>(TxnStatus::TXN_STATUS_FILESTORE_REJECTION), | |
| this->eventHistory_RxFileTransferFailed->at(0).conditionCode); |
(best-effort fix; verify before applying)
| } | ||
| // The canonical path is stored in the transaction and reported in events sized to | ||
| // MaxFilePathSize; reject rather than silently truncate a longer result | ||
| if (Fw::StringUtils::string_length(resolved, sizeof(resolved)) > Cfdp::MaxFilePathSize) { |
There was a problem hiding this comment.
[Test Quality] could fix This new > MaxFilePathSize guard is not deterministically covered. The max-length sub-case in testRxDestPathRejectedEvent sends a 200-char name, but with a typical build-dir CWD root alone is ~100 chars, so resolvePath returns TOO_LONG (240) first and this branch never executes; removing it would not fail any test. Consider a name of exactly MaxFilePathSize + 1 - strlen(root) chars (root from resolveDirectoryFromCwd(rxDir)) so the joined path lands in (200, 240], or a direct unit test of the bound.
|
|
||
| For mission deployments, ensure radio links employ hardware encryption or cryptographic authentication, ground systems implement proper authentication and authorization controls, and operational procedures include verification of file paths before commanding transfers. | ||
| - When `rx_dir` is **empty (the default)** destination paths are accepted as-is, consistent with the CCSDS 727.0-B-5 CFDP standard's assumption of operation over authenticated channels. This is a fail-open default chosen for backward compatibility. | ||
| - When `rx_dir` is **set**, every received destination path is canonicalized with `Os::FilePathUtils::resolvePath` (relative paths are resolved against `rx_dir`; `.` and `..` segments are collapsed textually, without following symlinks) and must remain inside `rx_dir` per `Os::FilePathUtils::checkContainment`. The check is applied once, in `Engine::recvMd`, before anything from the Metadata PDU is committed to the transaction, so every downstream file operation (`open`, `moveFile`, `removeFile`) only ever sees a validated path and a rejected PDU leaves the transaction unmodified. A rejected path raises the `RxDestPathRejected` warning, increments the channel's `faultFileOpen` counter, marks the transaction `FILESTORE_REJECTION` so it is reported as a failed reception (and, for Class 2, refused to the sender in the FIN), and terminates it without opening, moving, or removing any file (for a Class 2 transfer whose temporary file was already open, the transaction is finished against the temporary file only). This keeps a mistyped or unexpected destination from writing outside the area the mission has set aside for received files. |
There was a problem hiding this comment.
[Design] suggestion The SDD (and the comment at Engine::recvMd) say a Class 2 rejection is "refused to the sender in the FIN", but that is only true for the late-metadata path.
For a metadata-first rejection recvInit leaves the state at INIT and calls finishTransaction, which parks the transaction in HOLD and sends no PDU; recvHold only re-acks a peer FIN. The sender therefore gets no FIN and only times out on its own ACK/inactivity limits (already noted on the Operational concurrence). The design text should state the guarantee the code actually gives, or the metadata-first path should be routed through R2 so a FIN is emitted; the doc fix is the smaller change.
| - When `rx_dir` is **set**, every received destination path is canonicalized with `Os::FilePathUtils::resolvePath` (relative paths are resolved against `rx_dir`; `.` and `..` segments are collapsed textually, without following symlinks) and must remain inside `rx_dir` per `Os::FilePathUtils::checkContainment`. The check is applied once, in `Engine::recvMd`, before anything from the Metadata PDU is committed to the transaction, so every downstream file operation (`open`, `moveFile`, `removeFile`) only ever sees a validated path and a rejected PDU leaves the transaction unmodified. A rejected path raises the `RxDestPathRejected` warning, increments the channel's `faultFileOpen` counter, marks the transaction `FILESTORE_REJECTION` so it is reported as a failed reception (and, for Class 2, refused to the sender in the FIN), and terminates it without opening, moving, or removing any file (for a Class 2 transfer whose temporary file was already open, the transaction is finished against the temporary file only). This keeps a mistyped or unexpected destination from writing outside the area the mission has set aside for received files. | |
| - When `rx_dir` is **set**, every received destination path is canonicalized with `Os::FilePathUtils::resolvePath` (relative paths are resolved against `rx_dir`; `.` and `..` segments are collapsed textually, without following symlinks) and must remain inside `rx_dir` per `Os::FilePathUtils::checkContainment`. The check is applied once, in `Engine::recvMd`, before anything from the Metadata PDU is committed to the transaction, so every downstream file operation (`open`, `moveFile`, `removeFile`) only ever sees a validated path and a rejected PDU leaves the transaction unmodified. A rejected path raises the `RxDestPathRejected` warning, increments the channel's `faultFileOpen` counter, marks the transaction `FILESTORE_REJECTION` so it is reported locally as a failed reception, and terminates it without opening, moving, or removing any file. For a Class 2 transfer whose temporary file was already open (late metadata), the transaction is finished against the temporary file only and the rejection is carried to the sender in the FIN; a metadata-first rejection never enters R2 and sends no FIN, so the sender learns of the failure only through its own ACK/inactivity limits. This keeps a mistyped or unexpected destination from writing outside the area the mission has set aside for received files. |
There was a problem hiding this comment.
[Summary] Duplicate — consolidated into #5801 (comment).
| this->m_manager->getRxDirParam(txn->m_chan_num)); | ||
| this->m_manager->incrementFaultFileOpen(txn->m_chan_num); | ||
| // A refused destination is a filestore rejection, not a completed reception: this makes | ||
| // finishTransaction report the failure and, for Class 2, tells the sender in the FIN |
There was a problem hiding this comment.
[Correctness] could fix Follow-up to #5801 (comment): state-machine — a Class 2 metadata-first rejection still never sends a FIN, so the sender is not told.
recvInit() leaves the transaction in INIT and finishTransaction() parks it in HOLD; send_fin is never set and recvHold() only re-acks an incoming FIN, so for a Class 2 Metadata PDU rejected on first receipt no FIN/FILESTORE_REJECTION ever reaches the peer, which retransmits EOF until its own ack limit and then fails by timeout. The status fix makes the local events correct; this line's claim that the sender is told applies only to the late-metadata path (r2RecvMd → r2SetFinTxnStatus). Either drive the Class 2 case through R2 with r2SetFinTxnStatus before finishing, or narrow the comment.
lestarch-autobot
left a comment
lestarch-autobot
left a comment
There was a problem hiding this comment.
Automated review summary (run 2)
Per-agent results
| Agent | must fix | suggestion | could fix | future work | outstanding | Verdict |
|---|---|---|---|---|---|---|
| Security Vulnerabilities | 0 | 1 | 0 | 0 | 0 | Go |
| Supply Chain / Runner Safety | 0 | 0 | 0 | 0 | 0 | Go |
| F Prime C/C++ Design | 0 | 1 | 3 | 0 | 1 | Go |
| Documentation Currency | 1 | 1 | 2 | 0 | 0 | Go |
| Design | 2 | 2 | 0 | 0 | 3 | No-Go |
| Architecture | 0 | 0 | 0 | 0 | 0 | Go |
| Test Quality | 0 | 3 | 3 | 0 | 2 | Go |
| Correctness | 0 | 1 | 1 | 0 | 1 | Go |
| Operational | 1 | 2 | 1 | 0 | 1 | Go |
| Maintainability | 0 | 3 | 1 | 0 | 1 | Go |
| CI safety | — | — | — | — | — | Go |
| Totals | 4 | 13 | 11 | 0 | 9 | No-Go |
Totals count the finding consolidated by the de-duplication post-pass once (Operational/Design, docs/sdd.md:134 "refused in the FIN" wording).
Since last run
| Agent | resolved | still open | newly added | incorrect-fix follow-ups | improperly resolved | disagreements escalated |
|---|---|---|---|---|---|---|
| Security Vulnerabilities | 1 | 0 | 0 | 0 | 0 | 0 |
| Supply Chain / Runner Safety | 0 | 0 | 0 | 0 | 0 | 0 |
| F Prime C/C++ Design | 3 | 0 | 1 | 0 | 0 | 0 |
| Documentation Currency | 4 | 0 | 0 | 0 | 0 | 0 |
| Design | 1 | 2 | 1 | 0 | 0 | 0 |
| Architecture | 0 | 0 | 0 | 0 | 0 | 0 |
| Test Quality | 4 | 0 | 2 | 0 | 0 | 0 |
| Correctness | 1 | 0 | 0 | 1 | 0 | 0 |
| Operational | 3 | 0 | 1 | 0 | 0 | 0 |
| Maintainability | 3 | 0 | 1 | 1 | 0 | 0 |
Duplicates consolidated this run: 1 (threads closed by the §5h post-pass)
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 (1)
Design
- Human design adjudication required.
rx_dirintroduces a second, component-local file-confinement mechanism (fail-open by default, checked once at Metadata PDU) whose semantics differ fromOs::SandboxedFile; a design owner should settle which model F Prime wants before deeper review — #5801 (comment)
Merge readiness
Merge readiness: No-Go — Design has 1 outstanding must-fix item: the confinement-model question still awaits human adjudication (the metadata-first rejection status and the SDD event table were fixed in 0e11519).
Nice work landing the rejection-status and doc fixes — one design call from mission control and this is cleared for departure.
4 participants
Change Description
Adds an optional per-channel receive directory to Svc::Ccsds::CfdpManager so that destination paths carried in incoming CFDP Metadata PDUs are confined to a configured directory, bringing CfdpManager in line with the Os::SandboxedFile confinement already used by Svc::FileUplink, Svc::FileDownlink, and Svc::PrmDb.
Transaction::m_fd intentionally remains Os::File rather than Os::SandboxedFile: it is shared with the TX path, which reads command-supplied source files that must not be confined, and SandboxedFile would not cover the Os::FileSystem::moveFile/removeFile calls anyway.
Rationale
CfdpManager currently opens, moves, and deletes files at whatever path the Metadata PDU names, with only a length check. A mistyped or expected destination from the sending entity can thenywhere the process can write, or overwrite an existingfile. The sibling file-handling components already confine their file access to a configured directory; CfdpManager was the one uplink fileiter without that option. The SDD also described temapplied generally, when it only applies to one Class 2ordering. This change adds the same confinement option and corrects the documentation while preserving backward compatibility through an restricted default.
sting/Review Recommendations
ree new unit tests in test/ut/CfdpManagerEventTests. (fprime-util build --ut, run fromSvc/Ccsds/CfdpManager).
path retained, FILESTORE_REJECTION, no RxFileRenameF.
Review focus:
Not run here: clang-format (not installed locally; 120-column limit checked manually) and the full native build of Ref/FileHandlingCfdp.
Future Work
AI Usage (see policy)
Claude Code was used to draft the implementation, unit tests, and to run the unit test suite. All changes were reviewed by the author.