Merge pull request #310 from acudmore/fix-277-detect-pdu-truncation

Fix #277, Detect pdu truncation

docs/cf_FunctionalRequirements.csv

@@ -118,6 +118,8 @@ CF5021,CF5021,"When CF receives a ""Write Queue"" command, CF shall write the co
 CF5021.1,CF5021.1,"When CF receives a ""Write Queue"" command, if the command-specified queue is not defined, CF shall reject the command. ",Ensure robust operations and provide operator feedback.
 CF5023,CF5023,"The maximum number of transmissions, that is, the sum of simultaneous transmit and receive transactions, shall be defined at compile time.",Supports scaling resource use for mission variability
 CF5024,CF5024,"When CF receives a file-transfer request, if the requested file's size is larger than 2^32 bytes, CF shall reject the request and issue an error event message.",Constrain resources use and exclude implementation complexity of CCSDS  Large-file-size header extensions. CFDP-1S-01
+CF5025,CF5025,"When CF receives a PDU, if the size of the Entity ID fields are too large for the configured internal storage, CF shall reject the PDU and issue an error event message.",Prevent silent truncation of variable length Entity ID fields in PDU by checking the configured storage size and rejecting PDUs that with an Entity ID field that would be truncated.
+CF5026,CF5026,"When CF receives a PDU, if the size of the Transaction Sequence Number field is too large for the configured internal storage, CF shall reject the PDU and issue an error event message.",Prevent silent truncation of variable length Transaction Sequence Number field in PDU by checking configured storage size and rejecting PDUs that with a Transaction Sequence Number field that would be truncated.
 CF5030,CF5030,Each CF output channel shall have 256 file-transfer priority levels.,Priority levels are used to control the order of file transfer PDUs within a channel. Priority levels allow the control of PDU interleaving in a output channel and prevent a lower-priority transfer from blocking a high-priority operational transfer. 256 priority levels provide a high-level of granularity in the compact space of 1 byte.
 CF5030.1,CF5030.1,The CF file transmission priority level for each polling directory shall be configurable.,Supports management of priorities for each polling directory.  Commanded transfers (single file or playback directory) use the priority level contained in the command.
 CF5030.2,CF5030.2,The highest file-transfer priority level shall be zero.,Standardized interface. If the number of levels increases or decreases the highest priority will always be the same

fsw/src/cf_cfdp.c

@@ -633,16 +633,25 @@ int CF_CFDP_RecvPh(uint8 chan_num, CF_Logical_PduBuffer_t *ph)
     int ret = 0;
 
     CF_Assert(chan_num < CF_NUM_CHANNELS);
-
-    CF_CFDP_DecodeHeader(ph->pdec, &ph->pdu_header);
-
+    /*
+     * If the source eid, destination eid, or sequence number fields
+     * are larger than the sizes configured in the cf platform config
+     * file, then reject the PDU.
+     */
+    if (CF_CFDP_DecodeHeader(ph->pdec, &ph->pdu_header) != CFE_SUCCESS)
+    {
+        CFE_EVS_SendEvent(CF_EID_ERR_PDU_TRUNCATION, CFE_EVS_EventType_ERROR,
+                          "CF: pdu rejected due to eid/seq number field truncation");
+        ++CF_AppData.hk.channel_hk[chan_num].counters.recv.error;
+        ret = -1;
+    }
     /*
      * The "large file" flag is not supported by this implementation yet.
      * This means file sizes and offsets will be 64 bits, so codec routines
      * will need to be updated to understand this.  OSAL also doesn't support
      * 64-bit file access yet.
      */
-    if (CF_CODEC_IS_OK(ph->pdec) && ph->pdu_header.large_flag)
+    else if (CF_CODEC_IS_OK(ph->pdec) && ph->pdu_header.large_flag)
     {
         CFE_EVS_SendEvent(CF_EID_ERR_PDU_LARGE_FILE, CFE_EVS_EventType_ERROR,
                           "CF: pdu with large file bit received (unsupported)");

fsw/src/cf_codec.c

@@ -822,9 +822,10 @@ uint64 CF_DecodeIntegerInSize(CF_DecoderState_t *state, uint8 decode_size)
  * See description in cf_codec.h for argument/return detail
  *
  *-----------------------------------------------------------------*/
-void CF_CFDP_DecodeHeader(CF_DecoderState_t *state, CF_Logical_PduHeader_t *plh)
+int32 CF_CFDP_DecodeHeader(CF_DecoderState_t *state, CF_Logical_PduHeader_t *plh)
 {
     const CF_CFDP_PduHeader_t *peh; /* for decoding fixed sized fields */
+    int32                      ret = CFE_SUCCESS;
 
     /* decode the standard PDU header */
     peh = CF_DECODE_FIXED_CHUNK(state, CF_CFDP_PduHeader_t);
@@ -843,15 +844,22 @@ void CF_CFDP_DecodeHeader(CF_DecoderState_t *state, CF_Logical_PduHeader_t *plh)
 
         /* Length is a simple 16-bit quantity and refers to the content after this header */
         CF_Codec_Load_uint16(&(plh->data_encoded_length), &(peh->length));
+        if ((plh->eid_length > sizeof(plh->source_eid)) || (plh->txn_seq_length > sizeof(plh->sequence_num)))
+        {
+            ret = -1;
+        }
+        else
+        {
+            /* Now copy variable-length fields */
+            plh->source_eid      = CF_DecodeIntegerInSize(state, plh->eid_length);
+            plh->sequence_num    = CF_DecodeIntegerInSize(state, plh->txn_seq_length);
+            plh->destination_eid = CF_DecodeIntegerInSize(state, plh->eid_length);
 
-        /* Now copy variable-length fields */
-        plh->source_eid      = CF_DecodeIntegerInSize(state, plh->eid_length);
-        plh->sequence_num    = CF_DecodeIntegerInSize(state, plh->txn_seq_length);
-        plh->destination_eid = CF_DecodeIntegerInSize(state, plh->eid_length);
-
-        /* The header length is where decoding ended at this point */
-        plh->header_encoded_length = CF_CODEC_GET_POSITION(state);
+            /* The header length is where decoding ended at this point */
+            plh->header_encoded_length = CF_CODEC_GET_POSITION(state);
+        }
     }
+    return ret;
 }
 
 /*----------------------------------------------------------------

fsw/src/cf_codec.h

@@ -619,8 +619,10 @@ void CF_CFDP_EncodeCrc(CF_EncoderState_t *state, uint32 *plcrc);
  *
  * @param state  Decoder state object
  * @param plh    Pointer to logical PDU base header data
+ * @retval #CFE_SUCCESS \copydoc CFE_SUCCESS
+ * @retval Returns anything else on error.
  */
-void CF_CFDP_DecodeHeader(CF_DecoderState_t *state, CF_Logical_PduHeader_t *plh);
+int32 CF_CFDP_DecodeHeader(CF_DecoderState_t *state, CF_Logical_PduHeader_t *plh);
 
 /************************************************************************/
 /**

fsw/src/cf_events.h

@@ -348,6 +348,17 @@
  */
 #define CF_EID_ERR_PDU_LARGE_FILE (55)
 
+/**
+ * \brief CF PDU Header Field Truncation
+ *
+ *  \par Type: ERROR
+ *
+ *  \par Cause:
+ *
+ *  PDU Header received with fields that would be truncated with the cf configuration
+ */
+#define CF_EID_ERR_PDU_TRUNCATION (56)
+
 /**************************************************************************
  * CF_CFDP event IDs - Engine
  */

unit-test/cf_cfdp_tests.c

@@ -282,6 +282,12 @@ void Test_CF_CFDP_RecvPh(void)
     ph->pdu_header.large_flag = true;
     UtAssert_INT32_EQ(CF_CFDP_RecvPh(UT_CFDP_CHANNEL, ph), -1);
     UT_CF_AssertEventID(CF_EID_ERR_PDU_LARGE_FILE);
+
+    /* decode error, insufficient storage for EID or seq num */
+    UT_CFDP_SetupBasicTestState(UT_CF_Setup_RX, &ph, NULL, NULL, NULL, NULL);
+    UT_SetDeferredRetcode(UT_KEY(CF_CFDP_DecodeHeader), 1, -1);
+    UtAssert_INT32_EQ(CF_CFDP_RecvPh(UT_CFDP_CHANNEL, ph), -1);
+    UT_CF_AssertEventID(CF_EID_ERR_PDU_TRUNCATION);
 }
 
 void Test_CF_CFDP_RecvMd(void)

unit-test/cf_codec_tests.c

@@ -714,21 +714,24 @@ void Test_CF_CFDP_DecodeHeader(void)
      */
     CF_DecoderState_t      state;
     CF_Logical_PduHeader_t out;
-    const uint8            bytes[]     = {0x3c, 0x01, 0x02, 0x00, 0x44, 0x55, 0x66};
-    const uint8            bad_input[] = {0x20, 0x01, 0x02, 0x33, 0x01, 0x02, 0x03};
+    int32                  ret_val;
+    const uint8            bytes[]   = {0x3c, 0x01, 0x02, 0x00, 0x44, 0x55, 0x66};
+    const uint8            bad_eid[] = {0x3c, 0x01, 0x02, 0x73, 0x44, 0x55, 0x66};
+    const uint8            bad_tsn[] = {0x3c, 0x01, 0x02, 0x37, 0x44, 0x55, 0x66};
 
     /* fill with nonzero bytes so it is evident what was set */
     memset(&out, 0xEE, sizeof(out));
 
     /* call w/zero state should be noop */
     UT_CF_SetupDecodeState(&state, bytes, 0);
-    CF_CFDP_DecodeHeader(&state, &out);
+    ret_val = CF_CFDP_DecodeHeader(&state, &out);
     UtAssert_BOOL_FALSE(CF_CODEC_IS_OK(&state));
     UtAssert_MemCmpValue(&out, 0xEE, sizeof(out), "Bytes unchanged");
+    UtAssert_True(ret_val == CFE_SUCCESS, "CF_CFDP_DecodeHeader returned %d and should be 0", ret_val);
 
     /* setup nominal */
     UT_CF_SetupDecodeState(&state, bytes, sizeof(bytes));
-    CF_CFDP_DecodeHeader(&state, &out);
+    ret_val = CF_CFDP_DecodeHeader(&state, &out);
     UtAssert_BOOL_TRUE(CF_CODEC_IS_OK(&state));
     UtAssert_UINT32_EQ(CF_CODEC_GET_POSITION(&state), sizeof(bytes));
     UtAssert_UINT32_EQ(out.version, 1);
@@ -742,16 +745,21 @@ void Test_CF_CFDP_DecodeHeader(void)
     UtAssert_UINT32_EQ(out.sequence_num, 0x55);
     UtAssert_UINT32_EQ(out.destination_eid, 0x66);
     UtAssert_UINT32_EQ(out.header_encoded_length, sizeof(bytes));
+    UtAssert_True(ret_val == CFE_SUCCESS, "CF_CFDP_DecodeHeader returned %d and should be 0", ret_val);
 
     /*
-     * The bad input has large embedded EID/TSN lengths that would
-     * cause it to read beyond the end of the buffer.  This is to
-     * verify that the decode detects the problem and does not
-     * read beyond the end.
+     * Check for EID that would be truncated
      */
-    UT_CF_SetupDecodeState(&state, bad_input, sizeof(bad_input));
-    CF_CFDP_DecodeHeader(&state, &out);
-    UtAssert_BOOL_FALSE(CF_CODEC_IS_OK(&state));
+    UT_CF_SetupDecodeState(&state, bad_eid, sizeof(bad_eid));
+    ret_val = CF_CFDP_DecodeHeader(&state, &out);
+    UtAssert_True(ret_val == -1, "CF_CFDP_DecodeHeader returned %d and should be -1", ret_val);
+
+    /*
+     * Check for TSN that would be truncated
+     */
+    UT_CF_SetupDecodeState(&state, bad_tsn, sizeof(bad_tsn));
+    ret_val = CF_CFDP_DecodeHeader(&state, &out);
+    UtAssert_True(ret_val == -1, "CF_CFDP_DecodeHeader returned %d and should be -1", ret_val);
 }
 void Test_CF_CFDP_DecodeFileDirectiveHeader(void)
 {
@@ -1278,4 +1286,4 @@ void UtTest_Setup(void)
 
 } /* end UtTest_Setup for cf_codec_tests.c */
 
-/* end cf_codec_tests.c */
+/* end cf_codec_tests.c */

unit-test/stubs/cf_codec_stubs.c

@@ -161,12 +161,15 @@ void CF_CFDP_DecodeFin(CF_DecoderState_t *state, CF_Logical_PduFin_t *plfin)
  * Generated stub function for CF_CFDP_DecodeHeader()
  * ----------------------------------------------------
  */
-void CF_CFDP_DecodeHeader(CF_DecoderState_t *state, CF_Logical_PduHeader_t *plh)
+int32 CF_CFDP_DecodeHeader(CF_DecoderState_t *state, CF_Logical_PduHeader_t *plh)
 {
+    UT_GenStub_SetupReturnBuffer(CF_CFDP_DecodeHeader, int32);
     UT_GenStub_AddParam(CF_CFDP_DecodeHeader, CF_DecoderState_t *, state);
     UT_GenStub_AddParam(CF_CFDP_DecodeHeader, CF_Logical_PduHeader_t *, plh);
 
     UT_GenStub_Execute(CF_CFDP_DecodeHeader, Basic, NULL);
+
+    return UT_GenStub_GetReturnValue(CF_CFDP_DecodeHeader, int32);
 }
 
 /*