diff --git a/modules/es/fsw/src/cfe_es_cds.c b/modules/es/fsw/src/cfe_es_cds.c index 7bbf96e63..8e9a8e9e0 100644 --- a/modules/es/fsw/src/cfe_es_cds.c +++ b/modules/es/fsw/src/cfe_es_cds.c @@ -672,8 +672,11 @@ void CFE_ES_FormCDSName(char *FullCDSName, const char *CDSName, CFE_ES_AppId_t T /* Ensure that AppName is null terminated */ AppName[OS_MAX_API_NAME - 1] = '\0'; - /* Complete formation of processor specific table name */ - sprintf(FullCDSName, "%s.%s", AppName, CDSName); + /* Complete formation of processor specific table name. + * Use snprintf with the documented full-name length so that any future + * widening of OS_MAX_API_NAME or CFE_MISSION_ES_CDS_MAX_NAME_LENGTH + * cannot silently overflow the caller-supplied buffer. */ + snprintf(FullCDSName, CFE_MISSION_ES_CDS_MAX_FULL_NAME_LEN, "%s.%s", AppName, CDSName); } /*---------------------------------------------------------------- diff --git a/modules/es/fsw/src/cfe_es_mempool.c b/modules/es/fsw/src/cfe_es_mempool.c index a43f3b302..6bf7592e7 100644 --- a/modules/es/fsw/src/cfe_es_mempool.c +++ b/modules/es/fsw/src/cfe_es_mempool.c @@ -59,6 +59,49 @@ ** Functions */ +/*---------------------------------------------------------------- + * + * Internal helper — take a pool mutex if defined, log and proceed on failure. + * Matches the "log and continue" convention used by CFE_SB_LockSharedData / + * CFE_FS_LockSharedData / CFE_TBL_LockRegistry so a transient OSAL error does + * not silently mask an unprotected critical section. + * + *-----------------------------------------------------------------*/ +static inline void CFE_ES_PoolLock(const CFE_ES_MemPoolRecord_t *PoolRecPtr, const char *FuncName) +{ + int32 OsStatus; + + if (OS_ObjectIdDefined(PoolRecPtr->MutexId)) + { + OsStatus = OS_MutSemTake(PoolRecPtr->MutexId); + if (OsStatus != OS_SUCCESS) + { + CFE_ES_WriteToSysLog("%s: PoolMutex Take Err Stat=%ld,Pool=%lu\n", FuncName, (long)OsStatus, + OS_ObjectIdToInteger(PoolRecPtr->MutexId)); + } + } +} + +/*---------------------------------------------------------------- + * + * Internal helper — release a pool mutex if defined, log on failure. + * + *-----------------------------------------------------------------*/ +static inline void CFE_ES_PoolUnlock(const CFE_ES_MemPoolRecord_t *PoolRecPtr, const char *FuncName) +{ + int32 OsStatus; + + if (OS_ObjectIdDefined(PoolRecPtr->MutexId)) + { + OsStatus = OS_MutSemGive(PoolRecPtr->MutexId); + if (OsStatus != OS_SUCCESS) + { + CFE_ES_WriteToSysLog("%s: PoolMutex Give Err Stat=%ld,Pool=%lu\n", FuncName, (long)OsStatus, + OS_ObjectIdToInteger(PoolRecPtr->MutexId)); + } + } +} + /*---------------------------------------------------------------- * * Internal helper routine only, not part of API. @@ -447,10 +490,7 @@ int32 CFE_ES_GetPoolBuf(CFE_ES_MemPoolBuf_t *BufPtr, CFE_ES_MemHandle_t Handle, * Real work begins here. * If pool is mutex-protected, take the mutex now. */ - if (OS_ObjectIdDefined(PoolRecPtr->MutexId)) - { - OS_MutSemTake(PoolRecPtr->MutexId); - } + CFE_ES_PoolLock(PoolRecPtr, __func__); /* * Fundamental work is done as a generic routine. @@ -464,10 +504,7 @@ int32 CFE_ES_GetPoolBuf(CFE_ES_MemPoolBuf_t *BufPtr, CFE_ES_MemHandle_t Handle, * Real work ends here. * If pool is mutex-protected, release the mutex now. */ - if (OS_ObjectIdDefined(PoolRecPtr->MutexId)) - { - OS_MutSemGive(PoolRecPtr->MutexId); - } + CFE_ES_PoolUnlock(PoolRecPtr, __func__); /* If not successful, return error now */ if (Status != CFE_SUCCESS) @@ -511,10 +548,7 @@ CFE_Status_t CFE_ES_GetPoolBufInfo(CFE_ES_MemHandle_t Handle, CFE_ES_MemPoolBuf_ * Real work begins here. * If pool is mutex-protected, take the mutex now. */ - if (OS_ObjectIdDefined(PoolRecPtr->MutexId)) - { - OS_MutSemTake(PoolRecPtr->MutexId); - } + CFE_ES_PoolLock(PoolRecPtr, __func__); DataOffset = (cpuaddr)BufPtr - PoolRecPtr->BaseAddr; @@ -524,10 +558,7 @@ CFE_Status_t CFE_ES_GetPoolBufInfo(CFE_ES_MemHandle_t Handle, CFE_ES_MemPoolBuf_ * Real work ends here. * If pool is mutex-protected, release the mutex now. */ - if (OS_ObjectIdDefined(PoolRecPtr->MutexId)) - { - OS_MutSemGive(PoolRecPtr->MutexId); - } + CFE_ES_PoolUnlock(PoolRecPtr, __func__); if (Status == CFE_SUCCESS) { @@ -573,10 +604,7 @@ int32 CFE_ES_PutPoolBuf(CFE_ES_MemHandle_t Handle, CFE_ES_MemPoolBuf_t BufPtr) * Real work begins here. * If pool is mutex-protected, take the mutex now. */ - if (OS_ObjectIdDefined(PoolRecPtr->MutexId)) - { - OS_MutSemTake(PoolRecPtr->MutexId); - } + CFE_ES_PoolLock(PoolRecPtr, __func__); DataOffset = (cpuaddr)BufPtr - PoolRecPtr->BaseAddr; @@ -592,10 +620,7 @@ int32 CFE_ES_PutPoolBuf(CFE_ES_MemHandle_t Handle, CFE_ES_MemPoolBuf_t BufPtr) * Real work ends here. * If pool is mutex-protected, release the mutex now. */ - if (OS_ObjectIdDefined(PoolRecPtr->MutexId)) - { - OS_MutSemGive(PoolRecPtr->MutexId); - } + CFE_ES_PoolUnlock(PoolRecPtr, __func__); /* * If successful then modify return code to be @@ -653,10 +678,7 @@ CFE_Status_t CFE_ES_GetMemPoolStats(CFE_ES_MemPoolStats_t *BufPtr, CFE_ES_MemHan * Real work begins here. * If pool is mutex-protected, take the mutex now. */ - if (OS_ObjectIdDefined(PoolRecPtr->MutexId)) - { - OS_MutSemTake(PoolRecPtr->MutexId); - } + CFE_ES_PoolLock(PoolRecPtr, __func__); /* * Obtain the free and total byte count @@ -682,10 +704,7 @@ CFE_Status_t CFE_ES_GetMemPoolStats(CFE_ES_MemPoolStats_t *BufPtr, CFE_ES_MemHan * Real work ends here. * If pool is mutex-protected, release the mutex now. */ - if (OS_ObjectIdDefined(PoolRecPtr->MutexId)) - { - OS_MutSemGive(PoolRecPtr->MutexId); - } + CFE_ES_PoolUnlock(PoolRecPtr, __func__); return CFE_SUCCESS; } diff --git a/modules/sb/fsw/src/cfe_sb_priv.c b/modules/sb/fsw/src/cfe_sb_priv.c index d08036026..4ca26d4a3 100644 --- a/modules/sb/fsw/src/cfe_sb_priv.c +++ b/modules/sb/fsw/src/cfe_sb_priv.c @@ -279,7 +279,11 @@ char *CFE_SB_GetAppTskName(CFE_ES_TaskId_t TaskId, char *FullName) strncpy(TskName, (char *)ptr->TaskName, sizeof(TskName) - 1); TskName[sizeof(TskName) - 1] = '\0'; - sprintf(FullName, "%s.%s", AppName, TskName); + /* Use snprintf with the caller-documented buffer width. All callers + * declare FullName as char[OS_MAX_API_NAME * 2], so the cap below + * matches that contract and protects against any future widening of + * OS_MAX_API_NAME or change in the AppName/TskName field sizes. */ + snprintf(FullName, OS_MAX_API_NAME * 2, "%s.%s", AppName, TskName); } return FullName; diff --git a/modules/tbl/fsw/src/cfe_tbl_eds_codec.c b/modules/tbl/fsw/src/cfe_tbl_eds_codec.c index ca2dbe98d..8748ae871 100644 --- a/modules/tbl/fsw/src/cfe_tbl_eds_codec.c +++ b/modules/tbl/fsw/src/cfe_tbl_eds_codec.c @@ -411,7 +411,18 @@ CFE_Status_t CFE_TBL_ValidateCodecLoadSize(CFE_TBL_TxnState_t *Txn, const CFE_TB if (Status == CFE_SUCCESS) { - ProjectedSize = HeaderPtr->Offset + HeaderPtr->NumBytes; + /* Check Offset+NumBytes for unsigned overflow before performing the addition. + * Both fields originate from a (potentially attacker-controlled) file header, + * so a wrapped sum could otherwise bypass the size check below. */ + if (HeaderPtr->NumBytes > (UINT32_MAX - HeaderPtr->Offset)) + { + ProjectedSize = UINT32_MAX; + } + else + { + ProjectedSize = HeaderPtr->Offset + HeaderPtr->NumBytes; + } + if (ProjectedSize > ActualSize) { Status = CFE_TBL_ERR_FILE_TOO_LARGE; diff --git a/modules/tbl/fsw/src/cfe_tbl_internal.c b/modules/tbl/fsw/src/cfe_tbl_internal.c index 8dee56ae6..c586743ef 100644 --- a/modules/tbl/fsw/src/cfe_tbl_internal.c +++ b/modules/tbl/fsw/src/cfe_tbl_internal.c @@ -867,15 +867,39 @@ CFE_Status_t CFE_TBL_AllocateTableLoadBuffer(CFE_TBL_LoadBuff_t *LoadBuffPtr, si *-----------------------------------------------------------------*/ void CFE_TBL_MarkNameAsModified(char *NameBufPtr, size_t NameBufSize) { - char *EndPtr; + static const char Suffix[] = "(*)"; + const size_t SuffixLen = sizeof(Suffix) - 1; /* 3, excludes NUL */ + char * EndPtr; + size_t EndOffset; + + /* Buffer must hold suffix + NUL. Anything smaller is a programming error; + * silently doing nothing is safer than truncation that would not actually + * mark the name. */ + if (NameBufPtr == NULL || NameBufSize <= SuffixLen) + { + return; + } - EndPtr = memchr(NameBufPtr, 0, NameBufSize); - if ((EndPtr - NameBufPtr) > (NameBufSize - 4)) + /* Find the existing NUL terminator. If none is present within NameBufSize + * (caller error / corrupted name), append the suffix at a position that + * leaves room for it plus a final NUL. */ + EndPtr = memchr(NameBufPtr, '\0', NameBufSize); + if (EndPtr == NULL) { - EndPtr = &NameBufPtr[NameBufSize - 4]; + EndOffset = NameBufSize - SuffixLen - 1; + } + else + { + EndOffset = (size_t)(EndPtr - NameBufPtr); + if (EndOffset > NameBufSize - SuffixLen - 1) + { + EndOffset = NameBufSize - SuffixLen - 1; + } } - strcpy(EndPtr, "(*)"); + /* memcpy suffix + explicit NUL — avoids strcpy and any string.h length scan. */ + memcpy(&NameBufPtr[EndOffset], Suffix, SuffixLen); + NameBufPtr[EndOffset + SuffixLen] = '\0'; } /*---------------------------------------------------------------- diff --git a/modules/tbl/fsw/src/cfe_tbl_load.c b/modules/tbl/fsw/src/cfe_tbl_load.c index a74019ffb..be4ff1fb0 100644 --- a/modules/tbl/fsw/src/cfe_tbl_load.c +++ b/modules/tbl/fsw/src/cfe_tbl_load.c @@ -248,8 +248,22 @@ CFE_Status_t CFE_TBL_LoadContentFromFile(CFE_TBL_TxnState_t *Txn, osal_id_t File return Status; } + /* Compute the tail of the load region. Both Offset and NumBytes originate from + * a (potentially attacker-controlled) file header, so guard against unsigned + * overflow before the addition. On 32-bit targets size_t is 32 bits and the + * sum can wrap; on wider hosts this guard is still cheap and preserves identical + * behavior for valid input. If overflow would occur, force LoadTailSize to + * SIZE_MAX so the existing oversize check below rejects the load. */ + if (NumBytes > (SIZE_MAX - Offset)) + { + LoadTailSize = SIZE_MAX; + } + else + { + LoadTailSize = Offset + NumBytes; + } + /* Confirm that the data about to be loaded will fit */ - LoadTailSize = Offset + NumBytes; if (LoadTailSize > CFE_TBL_LoadBuffGetAllocSize(WorkingBufferPtr)) { Status = CFE_TBL_ERR_FILE_TOO_LARGE; diff --git a/modules/tbl/fsw/src/cfe_tbl_passthru_codec.c b/modules/tbl/fsw/src/cfe_tbl_passthru_codec.c index 5dac65cf1..54beca1b7 100644 --- a/modules/tbl/fsw/src/cfe_tbl_passthru_codec.c +++ b/modules/tbl/fsw/src/cfe_tbl_passthru_codec.c @@ -159,8 +159,20 @@ CFE_Status_t CFE_TBL_ValidateCodecLoadSize(CFE_TBL_TxnState_t *Txn, const CFE_TB uint32 ProjectedSize; /* Because the file sizes and in-memory size is the same, this check is simple */ - ActualSize = CFE_TBL_RegRecGetSize(CFE_TBL_TxnRegRec(Txn)); - ProjectedSize = HeaderPtr->Offset + HeaderPtr->NumBytes; + ActualSize = CFE_TBL_RegRecGetSize(CFE_TBL_TxnRegRec(Txn)); + + /* Check Offset+NumBytes for unsigned overflow before performing the addition. + * Both fields originate from a (potentially attacker-controlled) file header, + * so a wrapped sum could otherwise bypass the size check below. */ + if (HeaderPtr->NumBytes > (UINT32_MAX - HeaderPtr->Offset)) + { + ProjectedSize = UINT32_MAX; + } + else + { + ProjectedSize = HeaderPtr->Offset + HeaderPtr->NumBytes; + } + if (ProjectedSize > ActualSize) { Status = CFE_TBL_ERR_FILE_TOO_LARGE;