Decode procedures read header fields without checking length

[jphickey]

CF uses many variable-length fields in its header structure. This requires aggressive buffer bounds checking at each point during decode, since a bad value (such as a bad "length" on an LV parameter) can throw off the decoder which may end up reading past the end of data and into undefined behavior territory.

CF really only sanity checks the length at a couple points here in the initial header receive:

Headers alone:

CF/fsw/src/cf_cfdp.c

Line 874 in a894069

if (CF_AppData.engine.in.bytes_received < hsize)

Full PDU length field:

CF/fsw/src/cf_cfdp.c

Line 886 in a894069

if ((temp + hsize) != CF_AppData.engine.in.bytes_received)

However, note that even for the first check above (line 874) the code has already invoked CF_HeaderSize here:

CF/fsw/src/cf_cfdp.c

Line 870 in a894069

const int hsize = CF_HeaderSize(ph);

Notably, the CF_HeaderSize() function reads data from the packet header to compute header size. Therefore this has already read some packet data before the length is even initially sanity checked. If the input data was very short (such as from a MID misconfiguration) this would potentially segfault by immediately reading beyond the buffer.

Furthermore, every packet type that utilizes an LV or TLV style sub-fields (e.g. EOF, FIN, MD) needs to re-check the bounds at each of these entries. For example in RecvMd it only confirms that the size is sufficient for fixed-size fields here:

CF/fsw/src/cf_cfdp.c

Line 926 in a894069

if (CF_AppData.engine.in.bytes_received < (CF_HeaderSize(ph) + sizeof(CF_CFDP_PduMd_t)))

But later on when copying the LV data at these places, there is no check:

CF/fsw/src/cf_cfdp.c

Line 939 in a894069

lv_ret = CF_CFDP_CopyDataFromLv((uint8 *)t->history->fnames.src_filename, (CF_CFDP_lv_t *)md->filename_lvs);

CF/fsw/src/cf_cfdp.c

Lines 950 to 951 in a894069

	lv_ret =
	CF_CFDP_CopyDataFromLv((uint8 *)t->history->fnames.dst_filename, (CF_CFDP_lv_t *)(md->filename_lvs + offs));

These functions only check that the length is less than CF_FILENAME_MAX_LEN ... it does not check if the length has gone beyond the end of the input buffer.

Recommendation: Each and every step of encode + decode should confirm that the process is not reading or writing past the end of the buffer. Particularly for variable length fields.