Skip to content

Commit 7eacd55

Browse files
lestarch-autobotdevin-ai-integration[bot]LeStarchthomas-bc
authored
Svc: harden externally-reachable inputs against assertion DoS and path traversal (#5633)
* Svc: harden externally-reachable inputs against assertion DoS and path traversal Addresses the still-open subset of the reported security advisories: - GenericHub: size cmdDispIn staging buffer for a max-size command plus context - EventManager: drop oversized event packets instead of asserting - TlmPacketizer: reject oversized channel values with a warning event - TlmChan: drop new channels gracefully when the bucket pool is exhausted - PrmDb: optional sandbox directory restriction for commanded parameter file loads - Ref SignalGen: reject frequencies that cause divide/modulo-by-zero Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov> * Svc/PrmDb: use Os::SandboxedFile for commanded loads; add regression UTs Review feedback: delegate path containment to Os::SandboxedFile instead of reimplementing it with FilePathUtils. The sandboxed file is used only for commanded (staging) loads when a sandbox directory is configured, since the init-time load path must keep working where relative-path resolution is unavailable. Adds regression unit tests: - PrmDb: sandbox violation rejected, contained path reaches file I/O - TlmPacketizer: oversized channel value rejected with OversizedChannel event - TlmChan: bucket pool exhaustion drops channel with warning event - GenericHub: maximum-size command payload round-trips Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov> * Remove FIXME comment for file checking Removed FIXME comment about file checking robustness. * Fix OversizedChannel event size types Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: michael.d.starch <michael.d.starch@jpl.nasa.gov> Co-authored-by: M Starch <LeStarch@googlemail.com> Co-authored-by: thomas.boyer.chammard <thomas.boyer.chammard@jpl.nasa.gov>
1 parent ceee420 commit 7eacd55

19 files changed

Lines changed: 237 additions & 13 deletions

‎Svc/EventManager/EventManager.cpp‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
* Author: tcanham
66
*/
77

8+
#include <Fw/Logger/Logger.hpp>
89
#include <Fw/Types/Assert.hpp>
910
#include <Os/File.hpp>
1011
#include <Svc/EventManager/EventManager.hpp>
@@ -66,7 +67,14 @@ void EventManager::loqQueue_internalInterfaceHandler(FwEventIdType id,
6667
this->m_logPacket.setLogBuffer(args);
6768
this->m_comBuffer.resetSer();
6869
Fw::SerializeStatus stat = this->m_logPacket.serializeTo(this->m_comBuffer);
69-
FW_ASSERT(Fw::FW_SERIALIZE_OK == stat, static_cast<FwAssertArgType>(stat));
70+
// A maximum-size LogBuffer plus the packet header can exceed the com buffer capacity.
71+
// Drop the event rather than asserting, since the arguments may arrive from
72+
// external sources (e.g. a hub bridging another address space).
73+
if (Fw::FW_SERIALIZE_OK != stat) {
74+
Fw::Logger::log("[ERROR] EventManager: dropping event 0x%x (serialize status %d)\n", static_cast<U32>(id),
75+
static_cast<I32>(stat));
76+
return;
77+
}
7078

7179
if (this->isConnected_PktSend_OutputPort(0)) {
7280
this->PktSend_out(0, this->m_comBuffer, 0);

‎Svc/GenericHub/GenericHub.cpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,8 +61,9 @@ void GenericHub::bufferOutReturn_handler(FwIndexType portNum, Fw::Buffer& fwBuff
6161

6262
void GenericHub ::cmdDispIn_handler(FwIndexType portNum, Fw::ComBuffer& data, U32 context) {
6363
Fw::SerializeStatus status;
64-
// Buffer to send and a buffer used to write to it
65-
U8 buffer[Fw::ComBuffer::SERIALIZED_SIZE];
64+
// Buffer to send and a buffer used to write to it. Must hold a maximum-size
65+
// command buffer (serialized without its length token) plus the context word.
66+
U8 buffer[FW_COM_BUFFER_MAX_SIZE + sizeof(U32)];
6667

6768
Fw::ExternalSerializeBuffer serializer(buffer, sizeof(buffer));
6869
serializer.resetSer();

‎Svc/GenericHub/test/ut/GenericHubTester.cpp‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,18 @@ void GenericHubTester ::test_command_dispatch() {
160160
ASSERT_from_cmdDispOut_SIZE(1);
161161
ASSERT_from_cmdDispOut(0, buffer, 279);
162162
clearFromPortHistory();
163+
164+
// A maximum-size command payload must round-trip without overflow
165+
buffer.resetSer();
166+
for (U32 i = 0; i < FW_COM_BUFFER_MAX_SIZE; i++) {
167+
ASSERT_EQ(Fw::FW_SERIALIZE_OK, buffer.serializeFrom(static_cast<U8>(STest::Pick::any())));
168+
}
169+
invoke_to_cmdDispIn(0, buffer, 280);
170+
171+
ASSERT_from_fromBufferDriverReturn_SIZE(1);
172+
ASSERT_from_cmdDispOut_SIZE(1);
173+
ASSERT_from_cmdDispOut(0, buffer, 280);
174+
clearFromPortHistory();
163175
}
164176

165177
void GenericHubTester ::test_command_response() {

‎Svc/PrmDb/PrmDbImpl.cpp‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include <Svc/PrmDb/PrmDbImpl.hpp>
1010

1111
#include <Os/File.hpp>
12+
#include <Os/SandboxedFile.hpp>
1213
#include <Utils/Hash/Hash.hpp>
1314

1415
#include <cstdio>
@@ -54,6 +55,11 @@ void PrmDbImpl::configure(const char* file) {
5455
this->m_fileName = file;
5556
}
5657

58+
void PrmDbImpl::configureLoadSandbox(const char* directory) {
59+
FW_ASSERT(directory != nullptr);
60+
this->m_sandboxDir = directory;
61+
}
62+
5763
void PrmDbImpl::readParamFile() {
5864
// Assumed to run at initialization time
5965
// State should be IDLE upon entry
@@ -389,10 +395,22 @@ PrmDbImpl::PrmLoadStatus PrmDbImpl::readParamFileImpl(const Fw::StringBase& file
389395
FW_ASSERT(dbType == PrmDbType::DB_ACTIVE or dbType == PrmDbType::DB_STAGING);
390396
FW_ASSERT(fileName.length() > 0);
391397

392-
Fw::String dbString = getDbString(dbType);
393-
394-
// load file. FIXME: Put more robust file checking, such as a CRC.
398+
// Commanded loads (staging) may carry a ground-supplied path; restrict them
399+
// to the configured sandbox directory to prevent path traversal
400+
if ((dbType == PrmDbType::DB_STAGING) && (this->m_sandboxDir.length() > 0)) {
401+
Os::SandboxedFile paramFile;
402+
paramFile.configure(this->m_sandboxDir.toChar());
403+
return this->readParamFileWork(paramFile, fileName, dbType);
404+
}
395405
Os::File paramFile;
406+
return this->readParamFileWork(paramFile, fileName, dbType);
407+
}
408+
409+
template <typename FileType>
410+
PrmDbImpl::PrmLoadStatus PrmDbImpl::readParamFileWork(FileType& paramFile,
411+
const Fw::StringBase& fileName,
412+
PrmDbType dbType) {
413+
Fw::String dbString = getDbString(dbType);
396414

397415
Os::File::Status stat = paramFile.open(fileName.toChar(), Os::File::OPEN_READ);
398416
if (stat != Os::File::OP_OK) {

‎Svc/PrmDb/PrmDbImpl.hpp‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,14 @@ class PrmDbImpl final : public PrmDbComponentBase {
5757
//! \param file file where parameters are stored.
5858
void configure(const char* file);
5959

60+
//! \brief PrmDb load sandbox configure method
61+
//!
62+
//! Restricts the directory from which PRM_LOAD_FILE may read parameter
63+
//! files. If never called, any path is allowed (legacy behavior).
64+
//!
65+
//! \param directory directory that ground-commanded parameter file loads are restricted to.
66+
void configureLoadSandbox(const char* directory);
67+
6068
//! \brief PrmDb file read function
6169
//!
6270
//! The readFile function reads the set of parameters from the file passed in to
@@ -84,7 +92,8 @@ class PrmDbImpl final : public PrmDbComponentBase {
8492

8593
protected:
8694
private:
87-
Fw::String m_fileName; //!< filename for parameter storage
95+
Fw::String m_fileName; //!< filename for parameter storage
96+
Fw::String m_sandboxDir; //!< directory restriction for commanded file loads (empty = unrestricted)
8897

8998
PrmDbFileLoadState m_state; // Current file load state of the parameter database
9099

@@ -117,6 +126,14 @@ class PrmDbImpl final : public PrmDbComponentBase {
117126
//! \return status success (True) / failure(False)
118127
PrmLoadStatus readParamFileImpl(const Fw::StringBase& fileName, PrmDbType dbType);
119128

129+
//! \brief Read parameter file contents through an open-capable file object
130+
//!
131+
//! \param paramFile file object (Os::File or Os::SandboxedFile) to read through
132+
//! \param fileName file where parameters are stored
133+
//! \param dbType database to store the parameters in
134+
template <typename FileType>
135+
PrmLoadStatus readParamFileWork(FileType& paramFile, const Fw::StringBase& fileName, PrmDbType dbType);
136+
120137
//! \brief PrmDb parameter get handler
121138
//!
122139
//! This function retrieves a parameter value from the loaded set of stored parameters

‎Svc/PrmDb/test/ut/PrmDbTestMain.cpp‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,6 +271,22 @@ TEST(ParameterDbTest, PrmFileLoadIllegalActions) {
271271
tester.runPrmFileLoadIllegal();
272272
}
273273

274+
TEST(ParameterDbTest, PrmFileLoadSandboxViolation) {
275+
Svc::PrmDbImpl impl("PrmDbImpl");
276+
277+
impl.init(10, 0);
278+
impl.configure("TestFile.prm");
279+
280+
Svc::PrmDbTester tester(impl);
281+
282+
tester.init();
283+
284+
// connect ports
285+
connectPorts(impl, tester);
286+
287+
tester.runPrmFileLoadSandboxViolation();
288+
}
289+
274290
TEST(ParameterDbTest, PrmShorterSaveDoesNotCorrupt) {
275291
Svc::PrmDbImpl impl("PrmDbImpl");
276292

‎Svc/PrmDb/test/ut/PrmDbTester.cpp‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1130,6 +1130,40 @@ void PrmDbTester::runPrmFileLoadWithErrors() {
11301130
EXPECT_EQ(this->m_impl.m_stagingDb->getSize(), 0);
11311131
}
11321132

1133+
void PrmDbTester::runPrmFileLoadSandboxViolation() {
1134+
Fw::QueuedComponentBase::MsgDispatchStatus dispatchStatus;
1135+
1136+
// Restrict commanded loads to /prm
1137+
this->m_impl.configureLoadSandbox("/prm");
1138+
1139+
// 1. A path escaping the sandbox must be rejected before any file I/O
1140+
this->clearEvents();
1141+
this->clearHistory();
1142+
this->sendCmd_PRM_LOAD_FILE(0, 10, Fw::String("/prm/../etc/passwd"), PrmDb_Merge::RESET);
1143+
dispatchStatus = this->m_impl.doDispatch();
1144+
EXPECT_EQ(dispatchStatus, Fw::QueuedComponentBase::MSG_DISPATCH_OK);
1145+
1146+
ASSERT_CMD_RESPONSE_SIZE(1);
1147+
ASSERT_CMD_RESPONSE(0, PrmDbImpl::OPCODE_PRM_LOAD_FILE, 10, Fw::CmdResponse::EXECUTION_ERROR);
1148+
ASSERT_EVENTS_PrmFileReadError_SIZE(1);
1149+
ASSERT_EVENTS_PrmFileReadError(0, PrmDb_PrmReadError::OPEN, 0, Os::File::OUTSIDE_SANDBOX);
1150+
ASSERT_EVENTS_PrmDbFileLoadFailed_SIZE(1);
1151+
1152+
// 2. A path inside the sandbox passes containment and reaches file I/O
1153+
this->clearEvents();
1154+
this->clearHistory();
1155+
Os::Stub::File::Test::StaticData::setNextStatus(Os::File::DOESNT_EXIST);
1156+
this->sendCmd_PRM_LOAD_FILE(0, 11, Fw::String("/prm/good.prm"), PrmDb_Merge::RESET);
1157+
dispatchStatus = this->m_impl.doDispatch();
1158+
EXPECT_EQ(dispatchStatus, Fw::QueuedComponentBase::MSG_DISPATCH_OK);
1159+
1160+
ASSERT_CMD_RESPONSE_SIZE(1);
1161+
ASSERT_CMD_RESPONSE(0, PrmDbImpl::OPCODE_PRM_LOAD_FILE, 11, Fw::CmdResponse::EXECUTION_ERROR);
1162+
ASSERT_EVENTS_PrmFileReadError_SIZE(1);
1163+
ASSERT_EVENTS_PrmFileReadError(0, PrmDb_PrmReadError::OPEN, 0, Os::File::DOESNT_EXIST);
1164+
Os::Stub::File::Test::StaticData::setNextStatus(Os::File::OP_OK);
1165+
}
1166+
11331167
void PrmDbTester::runPrmFileLoadIllegal() {
11341168
Fw::QueuedComponentBase::MsgDispatchStatus dispatchStatus;
11351169
Fw::ParamBuffer pBuff;

‎Svc/PrmDb/test/ut/PrmDbTester.hpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ class PrmDbTester : public PrmDbGTestBase {
3232
void runPrmFileLoadNominal();
3333
void runPrmFileLoadWithErrors();
3434
void runPrmFileLoadIllegal();
35+
void runPrmFileLoadSandboxViolation();
3536
void runShorterSaveDoesNotCorrupt();
3637

3738
void runRefPrmFile();

‎Svc/TlmChan/TlmChan.cpp‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -246,8 +246,12 @@ void TlmChan::TlmRecv_handler(FwIndexType portNum, FwChanIdType id, Fw::Time& ti
246246
entryToUse = entryToUse->next;
247247
}
248248
} else {
249-
// Make sure that we haven't run out of buckets
250-
FW_ASSERT(this->m_tlmEntries[static_cast<U8>(this->m_activeBuffer)].free < TLMCHAN_HASH_BUCKETS);
249+
// Out of buckets: drop the new channel rather than asserting, since
250+
// telemetry IDs may arrive from external sources (e.g. a hub)
251+
if (this->m_tlmEntries[static_cast<U8>(this->m_activeBuffer)].free >= TLMCHAN_HASH_BUCKETS) {
252+
this->log_WARNING_HI_TlmChanBucketPoolExhausted(id);
253+
return;
254+
}
251255
// add new bucket from free list
252256
entryToUse = &this->m_tlmEntries[static_cast<U8>(this->m_activeBuffer)]
253257
.buckets[this->m_tlmEntries[static_cast<U8>(this->m_activeBuffer)].free++];
@@ -258,7 +262,12 @@ void TlmChan::TlmRecv_handler(FwIndexType portNum, FwChanIdType id, Fw::Time& ti
258262
}
259263
}
260264
} else {
261-
FW_ASSERT(this->m_tlmEntries[static_cast<U8>(this->m_activeBuffer)].free < TLMCHAN_HASH_BUCKETS);
265+
// Out of buckets: drop the new channel rather than asserting, since
266+
// telemetry IDs may arrive from external sources (e.g. a hub)
267+
if (this->m_tlmEntries[static_cast<U8>(this->m_activeBuffer)].free >= TLMCHAN_HASH_BUCKETS) {
268+
this->log_WARNING_HI_TlmChanBucketPoolExhausted(id);
269+
return;
270+
}
262271
this->m_tlmEntries[static_cast<U8>(this->m_activeBuffer)].slots[index] =
263272
&this->m_tlmEntries[static_cast<U8>(this->m_activeBuffer)]
264273
.buckets[this->m_tlmEntries[static_cast<U8>(this->m_activeBuffer)].free++];

‎Svc/TlmChan/TlmChan.fpp‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,13 @@ module Svc {
1212
) severity warning high \
1313
format "TlmChan epoch processing cap reached: {} entries deferred (cumulative: {})"
1414

15+
@ Hash bucket pool is exhausted; the new telemetry channel was dropped
16+
event TlmChanBucketPoolExhausted(
17+
Id: FwChanIdType @< The telemetry ID that could not be stored
18+
) severity warning high \
19+
format "TlmChan bucket pool exhausted: dropping new telemetry ID 0x{x}" \
20+
throttle 10
21+
1522
#-- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
1623
#Ports
1724
#-- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --

0 commit comments

Comments
 (0)