fix(Vss): correct wrong-field NULL checks and integer overflow in string-array serialization#137
Open
SoundMatt wants to merge 3 commits into
Open
Conversation
Avtp_Vss_DeserializeStringArray and Avtp_Vss_GetVSSDataStringArrayLength in src/avtp/acf/custom/Vss.c walk an on-wire string-array layout of [length-prefix : 2 bytes][string : length bytes] pairs. Both had serious bounds-check defects allowing crafted network input to drive out-of-bounds reads: 1. Avtp_Vss_DeserializeStringArray declared `uint16_t idx = 0;` and only checked `if (idx >= array_length) break;` — but `idx` was never incremented, so the break was dead code (it only fired when array_length == 0). With attacker-controlled str_length values from the wire (uint16_t, up to 65535), the memcpy() inside the loop could read up to ~64 KiB past the end of vss_data_string_array->data on every iteration after the first malicious entry. Heap-disclosure / crash class. 2. Avtp_Vss_GetVSSDataStringArrayLength advanced `ptr_idx += 2 + str_length` in uint16_t arithmetic. A str_length value near UINT16_MAX wraps ptr_idx around to a small number, making the loop iterate from a low offset again. Pseudo-infinite loop / DoS class. In addition, the loop condition `ptr_idx < total_length` was off by one — it permitted reading the 2-byte length prefix when only 1 byte remained. 3. Avtp_Vss_GetVSSDataStringArrayLength also returned uint8_t for a count that could plausibly exceed 255 (since total_length is uint16_t and a string can be 0 bytes, the protocol allows up to ~32k strings in a single array). Same shape as Avtp_Can_GetCanPayloadLength fixed in the previous PR. Fixes: - Add a `consumed` counter in DeserializeStringArray that is actually incremented per iteration and used to drive the break. - Add an explicit "remaining bytes >= str_length" check before memcpy() so a single malformed string-length value cannot trigger the OOB read. - In GetVSSDataStringArrayLength, change the loop guard to `ptr_idx + 2 <= total_length` (uint32_t arithmetic), and add a matching post-length-read check that the string body fits. - Widen the GetVSSDataStringArrayLength return type and the function declaration in include/avtp/acf/custom/Vss.h from uint8_t to uint16_t. - Add a NULL-check for strings[i] before dereferencing it (the previous code would NULL-deref strings[i]->data_length on the first malformed entry of an under-allocated output array). All bound-check additions are computed in uint32_t so the additions themselves cannot overflow when str_length approaches UINT16_MAX. No behaviour change for well-formed input. Existing unit tests in unit/test-vss.c continue to pass (they exercise the 3-string happy path). Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com>
Per @SebastianSchildt's review on this PR. Constructs a 23-byte on-wire fixture identical to the well-formed vss_data_string_array test, except the third length prefix is patched from 0x0007 to 0x00AA — so the third entry claims 170 bytes of body but only 7 bytes of buffer remain after its length prefix. Asserts: - Avtp_Vss_GetVSSDataStringArrayLength returns 2 (not 3, and not 0 which would be the result of bailing on the first read). - Avtp_Vss_DeserializeStringArray fills the first two output structs with 'Hello' and 'World' and leaves the third struct at its initial state (data_length == 0). The third entry is rejected by the new remaining-bytes-vs-str_length check before memcpy. Without the bounds-check fix in this PR the same input would have driven memcpy() to read up to ~64 KiB past the end of the buffer in DeserializeStringArray, and the count loop in GetVSSDataStringArrayLength would have wrapped uint16_t arithmetic on the (uint16_t + 0xAA) addition, producing a pseudo-infinite loop. Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com>
…ing-array serialization Three bugs in Avtp_Vss_GetVssData / Avtp_Vss_SerializeStringArray: 1. VSS_UINT64_ARRAY (line 352): NULL guard read data_int64_array->data instead of data_uint64_array->data. If data_int64_array happens to be non-NULL while data_uint64_array is NULL the memcpy writes through a NULL pointer; conversely a valid buffer could be skipped. 2. VSS_STRING_ARRAY (line 401): NULL guard read data_double_array->data instead of data_string_array->data. Same consequence: the memcpy destination (data_string_array->data) could be NULL even when the guard passes. 3. SerializeStringArray: total_length was uint16_t. Accumulating num_strings entries each up to 65535+2 bytes silently wraps, causing the caller to store a truncated length in the wire packet which downstream deserialization then uses as a buffer size, enabling an over-read. Fix: accumulate in uint32_t and cap at UINT16_MAX before writing back to the uint16_t field. Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three bugs in
Avtp_Vss_GetVssData()andAvtp_Vss_SerializeStringArray()insrc/avtp/acf/custom/Vss.c:1.
VSS_UINT64_ARRAY— NULL guard checks the wrong field (line 352)The guard reads
data_int64_array->datainstead ofdata_uint64_array->data. Ifdata_uint64_array->datais NULL butdata_int64_array->datais not, the loop writes through a NULL pointer. Conversely, ifdata_uint64_array->datais valid butdata_int64_array->datais NULL, the copy is silently skipped.2.
VSS_STRING_ARRAY— NULL guard checks the wrong field (line 401)The guard reads
data_double_array->datawhile thememcpydestination isdata_string_array->data. A crafted VSS packet that setsVSS_STRING_ARRAYtype whiledata_string_array->datais NULL anddata_double_array->datais non-NULL would trigger a NULL-pointer memcpy write.3.
SerializeStringArray—uint16_taccumulator overflows for large arrays (line 623)total_lengthisuint16_t. Accumulating many strings can silently wrap to a value much smaller than the actual payload, causing the storeddata_lengthto be wrong. Downstream deserialization then uses this truncated length as the memcpy size, enabling a buffer over-read.Changes
VSS_UINT64_ARRAYandVSS_STRING_ARRAYto check the correct field.total_lengthinuint32_t; cap atUINT16_MAXbefore assigning to theuint16_tstruct field.Testing
VSS_UINT64_ARRAYdeserialization with validdata_uint64_array->datanow correctly copies data.VSS_STRING_ARRAYdeserialization with NULLdata_string_array->datano longer writes through NULL.SerializeStringArraywith strings summing to >65535 bytes now saturates atUINT16_MAXrather than wrapping.