Skip to content

Commit

Permalink
Fix #46, Resolve possible string buffer overflows
Browse files Browse the repository at this point in the history
  • Loading branch information
@skliper
skliper committed Aug 17, 2022
1 parent 733b2b2 commit 598379a
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 143 deletions.
5 changes: 2 additions & 3 deletions fsw/src/ds_appdefs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,8 @@

#define DS_INDEX_NONE -1 /**< \brief Packet filter table look-up = not found */

#define DS_PATH_SEPARATOR '/' /**< \brief File system path separator */
#define DS_EMPTY_STRING "" /**< \brief Empty string buffer entries in DS tables */
#define DS_STRING_TERMINATOR '\0' /**< \brief ASCIIZ string terminator character */
#define DS_PATH_SEPARATOR '/' /**< \brief File system path separator */
#define DS_EMPTY_STRING "" /**< \brief Empty string buffer entries in DS tables */

#define DS_TABLE_VERIFY_ERR 0xFFFFFFFF /**< \brief Table verification error return value */

Expand Down
9 changes: 5 additions & 4 deletions fsw/src/ds_cmds.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -763,7 +763,7 @@ void DS_CmdSetDestPath(const CFE_SB_Buffer_t *BufPtr)
** Set path portion of destination table filename...
*/
pDest = &DS_AppData.DestFileTblPtr->File[DS_DestPathCmd->FileTableIndex];
strcpy(pDest->Pathname, DS_DestPathCmd->Pathname);
strncpy(pDest->Pathname, DS_DestPathCmd->Pathname, sizeof(pDest->Pathname));

/*
** Notify cFE that we have modified the table data...
Expand Down Expand Up @@ -833,7 +833,7 @@ void DS_CmdSetDestBase(const CFE_SB_Buffer_t *BufPtr)
** Set base portion of destination table filename...
*/
pDest = &DS_AppData.DestFileTblPtr->File[DS_DestBaseCmd->FileTableIndex];
strcpy(pDest->Basename, DS_DestBaseCmd->Basename);
strncpy(pDest->Basename, DS_DestBaseCmd->Basename, sizeof(pDest->Basename));

/*
** Notify cFE that we have modified the table data...
Expand Down Expand Up @@ -903,7 +903,7 @@ void DS_CmdSetDestExt(const CFE_SB_Buffer_t *BufPtr)
** Set extension portion of destination table filename...
*/
pDest = &DS_AppData.DestFileTblPtr->File[DS_DestExtCmd->FileTableIndex];
strcpy(pDest->Extension, DS_DestExtCmd->Extension);
strncpy(pDest->Extension, DS_DestExtCmd->Extension, sizeof(pDest->Extension));

/*
** Notify cFE that we have modified the table data...
Expand Down Expand Up @@ -1366,7 +1366,8 @@ void DS_CmdGetFileInfo(const CFE_SB_Buffer_t *BufPtr)
/*
** Set current open filename...
*/
strcpy(DS_FileInfoPkt.FileInfo[i].FileName, DS_AppData.FileStatus[i].FileName);
strncpy(DS_FileInfoPkt.FileInfo[i].FileName, DS_AppData.FileStatus[i].FileName,
sizeof(DS_FileInfoPkt.FileInfo[i].FileName));
}
}

Expand Down
160 changes: 53 additions & 107 deletions fsw/src/ds_file.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ void DS_FileWriteHeader(int32 FileIndex)
*/
memset(&CFE_FS_Header, 0, sizeof(CFE_FS_Header));
CFE_FS_Header.SubType = DS_FILE_HDR_SUBTYPE;
strcpy(CFE_FS_Header.Description, DS_FILE_HDR_DESCRIPTION);
strncpy(CFE_FS_Header.Description, DS_FILE_HDR_DESCRIPTION, sizeof(CFE_FS_Header.Description));

/*
** Let cFE finish the init and write the primary header...
Expand All @@ -421,7 +421,7 @@ void DS_FileWriteHeader(int32 FileIndex)
memset(&DS_FileHeader, 0, sizeof(DS_FileHeader));
DS_FileHeader.FileTableIndex = FileIndex;
DS_FileHeader.FileNameType = DestFile->FileNameType;
strcpy(DS_FileHeader.FileName, FileStatus->FileName);
strncpy(DS_FileHeader.FileName, FileStatus->FileName, sizeof(DS_FileHeader.FileName));

/*
** Manually write the secondary header...
Expand Down Expand Up @@ -501,7 +501,7 @@ void DS_FileCreateDest(uint32 FileIndex)
*/
DS_FileCreateName(FileIndex);

if (FileStatus->FileName[0] != DS_STRING_TERMINATOR)
if (FileStatus->FileName[0] != 0)
{
/*
** Success - create a new destination file...
Expand Down Expand Up @@ -574,128 +574,73 @@ void DS_FileCreateName(uint32 FileIndex)
DS_DestFileEntry_t *DestFile = &DS_AppData.DestFileTblPtr->File[FileIndex];
DS_AppFileStatus_t *FileStatus = &DS_AppData.FileStatus[FileIndex];
int32 TotalLength = 0;
int32 WorknameLen = 2 * DS_TOTAL_FNAME_BUFSIZE;

char Workname[WorknameLen];
char Workname[2 * DS_TOTAL_FNAME_BUFSIZE];
char Sequence[DS_TOTAL_FNAME_BUFSIZE];

Workname[0] = DS_STRING_TERMINATOR;
Sequence[0] = DS_STRING_TERMINATOR;
/* Copy in path */
CFE_SB_MessageStringGet(Workname, DestFile->Pathname, NULL, sizeof(Workname), sizeof(DestFile->Pathname));
TotalLength = strlen(Workname);

/*
** Start with the path portion of the filename...
*/
strncpy(Workname, DestFile->Pathname, WorknameLen - 1);
Workname[WorknameLen - 1] = '\0';
TotalLength = strlen(Workname);

/*
** Add a path separator (if needed) before appending the base name...
*/
if (TotalLength > 0)
{
/* Add separator if needed */
if (Workname[TotalLength - 1] != DS_PATH_SEPARATOR)
{
/* if there's room, write the path separator and a new terminating
character. If there's not room, this will be caught by the
next condition */
if (TotalLength != (WorknameLen - 1))
{
Workname[TotalLength] = DS_PATH_SEPARATOR;
Workname[TotalLength + 1] = DS_STRING_TERMINATOR;
}
/* There's always space since Workname is twice the size of Pathname */
Workname[TotalLength++] = DS_PATH_SEPARATOR;
}
}
else
{
/* If path name is empty, start with the path separator. This should
* not happen because the path name is verified as non-empty in
* DS_TableVerifyDestFileEntry */
CFE_EVS_SendEvent(DS_FILE_CREATE_EMPTY_PATH_ERR_EID, CFE_EVS_EventType_ERROR,
"FILE NAME error: Path empty. dest = %d, path = '%s'", (int)FileIndex, DestFile->Pathname);

/*
** Something needs to get fixed before we try again...
*/
DS_AppData.FileStatus[FileIndex].FileState = DS_DISABLED;

return;
}

/*
** Verify that the path plus the base portion is not too large...
*/
if ((strlen(Workname) + strlen(DestFile->Basename)) < DS_TOTAL_FNAME_BUFSIZE)
{
/*
** Append the base portion to the path portion...
*/
strcat(Workname, DestFile->Basename);
/* Add base name */
CFE_SB_MessageStringGet(&Workname[TotalLength], DestFile->Basename, NULL, sizeof(Workname) - TotalLength,
sizeof(DestFile->Basename));
TotalLength = strlen(Workname);

/*
** Create the sequence portion of the filename...
*/
/* Create the sequence portion of the filename */
DS_FileCreateSequence(Sequence, DestFile->FileNameType, FileStatus->FileCount);

/*
** Verify that the path/base plus the sequence portion is not too large...
*/
if ((strlen(Workname) + strlen(Sequence)) < DS_TOTAL_FNAME_BUFSIZE)
{
/*
** Append the sequence portion to the path/base portion...
*/
strcat(Workname, Sequence);
/* Sequence is always null terminated so can use strncat */
strncat(&Workname[TotalLength], Sequence, sizeof(Workname) - TotalLength - 1);
TotalLength = strlen(Workname);

/*
** Check for an optional file extension...
*/
if (strlen(DestFile->Extension) > 0)
/* Only add extension if not empty */
if (DestFile->Extension[0] != '\0')
{
/* Add a "." character (if needed) before appending the extension */
if (DestFile->Extension[0] != '.')
{
/*
** Add a "." character (if needed) before appending the extension...
*/
if (DestFile->Extension[0] != '.')
{
strcat(Workname, ".");
}

/*
** Append the extension portion to the path/base+sequence portion...
*/
strcat(Workname, DestFile->Extension);
strncat(Workname, ".", sizeof(Workname) - strlen(Workname) - 1);
TotalLength++;
}

/*
** Final test - is "path/base+sequence.extension" length valid?...
*/
if (strlen(Workname) < DS_TOTAL_FNAME_BUFSIZE)
{
/*
** Success - copy workname to filename buffer...
*/
strcpy(FileStatus->FileName, Workname);
}
/* Append the extension portion to the path/base+sequence portion */
CFE_SB_MessageStringGet(&Workname[TotalLength], DestFile->Extension, NULL, sizeof(Workname) - TotalLength,
sizeof(DestFile->Extension));
}
}

if (FileStatus->FileName[0] == DS_STRING_TERMINATOR)
/* Confirm working name fits */
if (strlen(Workname) < DS_TOTAL_FNAME_BUFSIZE)
{
/* Success - copy workname to filename buffer */
strcpy(FileStatus->FileName, Workname);
}
else
{
/* Error - send event and disable destination */
CFE_EVS_SendEvent(DS_FILE_NAME_ERR_EID, CFE_EVS_EventType_ERROR,
"FILE NAME error: dest = %d, path = '%s', base = '%s', seq = '%s', ext = '%s'",
(int)FileIndex, DestFile->Pathname, DestFile->Basename, Sequence, DestFile->Extension);
DS_AppData.FileStatus[FileIndex].FileState = DS_DISABLED;
}
}
else
{
/*
** Error - send event and disable destination...
*/
CFE_EVS_SendEvent(DS_FILE_NAME_ERR_EID, CFE_EVS_EventType_ERROR,
"FILE NAME error: dest = %d, path = '%s', base = '%s', seq = '%s', ext = '%s'",
(int)FileIndex, DestFile->Pathname, DestFile->Basename, Sequence, DestFile->Extension);

/*
** Something needs to get fixed before we try again...
*/
/* Send event and disable for invalid path */
CFE_EVS_SendEvent(DS_FILE_CREATE_EMPTY_PATH_ERR_EID, CFE_EVS_EventType_ERROR,
"FILE NAME error: Path empty. dest = %d, path = '%s'", (int)FileIndex, DestFile->Pathname);
DS_AppData.FileStatus[FileIndex].FileState = DS_DISABLED;
}

return;

} /* End of DS_FileCreateName() */

/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */
Expand Down Expand Up @@ -742,7 +687,7 @@ void DS_FileCreateSequence(char *Buffer, uint32 Type, uint32 Count)
/*
** Add string terminator...
*/
Buffer[DS_SEQUENCE_DIGITS] = DS_STRING_TERMINATOR;
Buffer[DS_SEQUENCE_DIGITS] = '\0';
}
else if (Type == DS_BY_TIME)
{
Expand Down Expand Up @@ -817,14 +762,14 @@ void DS_FileCreateSequence(char *Buffer, uint32 Type, uint32 Count)
/*
** Step 7: Add string terminator...
*/
Buffer[DS_TERM_INDEX] = DS_STRING_TERMINATOR;
Buffer[DS_TERM_INDEX] = '\0';
}
else
{
/*
** Bad filename type, init buffer as empty...
*/
Buffer[0] = DS_STRING_TERMINATOR;
Buffer[0] = '\0';
}

return;
Expand Down Expand Up @@ -903,7 +848,8 @@ void DS_FileCloseDest(int32 FileIndex)
/*
** Make sure directory name does not end with slash character...
*/
strcpy(PathName, DS_AppData.DestFileTblPtr->File[FileIndex].Movename);
CFE_SB_MessageStringGet(PathName, DS_AppData.DestFileTblPtr->File[FileIndex].Movename, NULL, sizeof(PathName),
sizeof(DS_AppData.DestFileTblPtr->File[FileIndex].Movename));
PathLength = strlen(PathName);
if (PathName[PathLength - 1] == '/')
{
Expand Down Expand Up @@ -1082,7 +1028,7 @@ void DS_FileTransmit(DS_AppFileStatus_t *FileStatus)
/*
** Set current open filename...
*/
strcpy(PktBuf->Pkt.FileInfo.FileName, FileStatus->FileName);
strncpy(PktBuf->Pkt.FileInfo.FileName, FileStatus->FileName, sizeof(PktBuf->Pkt.FileInfo.FileName));

/*
** Timestamp and send file info telemetry...
Expand Down
30 changes: 1 addition & 29 deletions unit-test/ds_file_tests.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -620,8 +620,6 @@ void DS_FileCreateDest_Test_StringTerminate(void)
{
uint32 FileIndex = 0;

DS_AppData.FileStatus[FileIndex].FileName[0] = DS_STRING_TERMINATOR;

/* Execute the function being tested */
UtAssert_VOIDCALL(DS_FileCreateDest(FileIndex));

Expand Down Expand Up @@ -843,7 +841,7 @@ void DS_FileCreateName_Test_Error(void)
DS_AppData.DestFileTblPtr->File[FileIndex].Basename[i] = 'a';
}

DS_AppData.DestFileTblPtr->File[FileIndex].Basename[DS_TOTAL_FNAME_BUFSIZE - 1] = DS_STRING_TERMINATOR;
DS_AppData.DestFileTblPtr->File[FileIndex].Basename[DS_TOTAL_FNAME_BUFSIZE - 1] = '\0';

/* Execute the function being tested */
DS_FileCreateName(FileIndex);
Expand All @@ -856,31 +854,6 @@ void DS_FileCreateName_Test_Error(void)

} /* end DS_FileCreateName_Test_Error */

void DS_FileCreateName_Test_MaxPathnameLength(void)
{
int32 FileIndex = 0;
int32 WorknameLen = 2 * DS_TOTAL_FNAME_BUFSIZE;
int32 i;

UT_DS_SetDestFileEntry(&DS_AppData.DestFileTblPtr->File[FileIndex]);

/* Set to fail the condition "if (TotalLength != (WorknameLen - 1))" */
for (i = 0; i < WorknameLen - 1; i++)
{
DS_AppData.DestFileTblPtr->File[FileIndex].Pathname[i] = 'a';
}

/* Execute the function being tested */
UtAssert_VOIDCALL(DS_FileCreateName(FileIndex));

/* Verify results */
UtAssert_INT32_EQ(strlen(DS_AppData.DestFileTblPtr->File[FileIndex].Pathname), WorknameLen - 1);
UtAssert_STUB_COUNT(CFE_EVS_SendEvent, 1);
UtAssert_INT32_EQ(context_CFE_EVS_SendEvent[0].EventType, CFE_EVS_EventType_ERROR);
UtAssert_INT32_EQ(context_CFE_EVS_SendEvent[0].EventID, DS_FILE_NAME_ERR_EID);

} /* end DS_FileCreateName_Test_MaxPathnameLength */

void DS_FileCreateName_Test_PathBaseSeqTooLarge(void)
{
int32 FileIndex = 0;
Expand Down Expand Up @@ -1536,7 +1509,6 @@ void UtTest_Setup(void)
UT_DS_TEST_ADD(DS_FileCreateName_Test_NominalWithPeriod);
UT_DS_TEST_ADD(DS_FileCreateName_Test_EmptyPath);
UT_DS_TEST_ADD(DS_FileCreateName_Test_Error);
UT_DS_TEST_ADD(DS_FileCreateName_Test_MaxPathnameLength);
UT_DS_TEST_ADD(DS_FileCreateName_Test_PathBaseSeqTooLarge);
UT_DS_TEST_ADD(DS_FileCreateName_Test_PathBaseSeqExtTooLarge);
UT_DS_TEST_ADD(DS_FileCreateName_Test_ExtensionZero);
Expand Down

0 comments on commit 598379a

Please sign in to comment.