[Prior2] fix: handle bufferoverflow and memory issues in v2 protocol - #895
Closed
santhoshsivanhere wants to merge 9 commits into
Closed
[Prior2] fix: handle bufferoverflow and memory issues in v2 protocol#895santhoshsivanhere wants to merge 9 commits into
santhoshsivanhere wants to merge 9 commits into
Conversation
The fixed-size precheck at the entry of this function validates only the 11-byte minimum GET_LOG_INFO V2 request (apidlen = ctidlen = 0). The function then reads two attacker-controlled uint8_t length fields (apidlen, ctidlen) from msg->databuffer and uses them to advance an offset, before passing pointers into the buffer to dlt_set_id_v2() (which reads up to apidlen / ctidlen bytes via dlt_strnlen_s) and finishing with a 4-byte memcpy of the trailing com field. A short message with non-zero length fields causes the variable-length reads to walk past the end of msg->databuffer. Add bounds checks after each length is parsed, freeing the calloc'd request and returning when the message cannot satisfy the next read. Note: the existing early-return paths inside this function already leak the calloc'd req pointer; left untouched here to keep this commit focused on the security fix. Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com>
Companion fix to the OOB-read fix in the previous commit. The function never assigned req->apid or req->ctid: req is calloc'd, so the char * pointer fields start NULL, and the dlt_set_id_v2(req->apid, ...) call to populate them was a no-op (dlt_set_id_v2 early-returns when its destination is NULL). req->apid / req->ctid stayed NULL and were then passed to dlt_daemon_application_find_v2 and dlt_daemon_context_find_v2 despite req->apidlen / req->ctidlen being non-zero — every non-empty lookup was silently turned into a zero-length one. Copy the apid/ctid into local fixed-size buffers through the shared dlt_set_id_v2() helper and point req->apid / req->ctid at those buffers, keeping id initialisation inside the helper API rather than aliasing the receive buffer. The buffers live for the whole function scope and req is freed (struct only) before return, so the pointers stay valid for every use. The bounds checks added in the previous commit keep the helper's reads within msg->databuffer. Closes COVESA#870. Related: COVESA#866. Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com>
santhoshsivanhere
marked this pull request as draft
July 7, 2026 05:15
minminlittleshrimp
changed the title
minminlittleshrimp
changed the title
This was referenced Jul 9, 2026
Fix two issues in dlt_daemon_control_get_log_info_v2() when parsing GET_LOG_INFO V2 requests. First, validate the variable-length APID and CTID fields before reading them. The existing fixed-size check only verified the minimum 11-byte request, allowing attacker-controlled apidlen and ctidlen values to cause reads beyond the end of msg->databuffer through dlt_set_id_v2() and the trailing com field memcpy(). Add bounds checks after each length is parsed and abort processing when the remaining message is too short. Second, correctly initialize req->apid and req->ctid. The request structure is allocated with calloc(), leaving these pointers NULL. Passing them to dlt_set_id_v2() was therefore a no-op, causing all subsequent APID/CTID lookups to behave as zero-length lookups even when valid IDs were present. Copy the IDs into local fixed-size buffers using dlt_set_id_v2() and point req->apid and req->ctid to those buffers for the lifetime of the function. The existing early-return paths that leak the allocated request structure are intentionally left unchanged. Closes: COVESA#870 Related: COVESA#866 Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com> Signed-off-by: Santhosh Sivan Murugan <122221363+santhoshsivanhere@users.noreply.github.com>
Validate the variable-length APID and CTID fields when parsing SET_LOG_LEVEL V2 requests. The existing fixed-size precheck verifies only the minimum 11-byte request (apidlen = ctidlen = 0). The function then reads two attacker-controlled uint8_t length fields and uses them to advance an offset into msg->databuffer before passing pointers into the buffer to dlt_set_id_v2(), reading the log_level field, and copying the trailing com field. A short message with non-zero length fields can therefore cause reads beyond the end of msg->databuffer. Add bounds checks after each length field is parsed and return without further processing when the remaining message is too short. Since the request structure is allocated on the stack, no cleanup is required on these error paths. Related: COVESA#861 Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com> Signed-off-by: Santhosh Sivan Murugan <122221363+santhoshsivanhere@users.noreply.github.com>
Correct APID and CTID parsing in dlt_daemon_control_set_log_level_v2(). The request structure stores APID and CTID as char * fields, but these pointers were never initialized before being passed to dlt_set_id_v2(). Since the helper returns immediately when given a NULL destination, the IDs were never parsed from the request, leaving req.apid and req.ctid NULL. Requests with empty APID/CTID fields silently became no-ops, while requests containing non-empty IDs could dereference a NULL pointer in the wildcard and field-only handling paths, crashing the daemon. Copy the APID and CTID into local fixed-size buffers using dlt_set_id_v2() and point req.apid and req.ctid at those buffers. This keeps ID initialization within the shared helper, avoids aliasing the receive buffer, and ensures the parsed IDs remain valid for the lifetime of the function. Also add unit tests covering dlt_set_id_v2() normal operation, truncation at DLT_V2_ID_SIZE, overflow handling, and NULL/zero-length inputs. Closes: COVESA#863 Related: COVESA#862 Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Santhosh Sivan Murugan <122221363+santhoshsivanhere@users.noreply.github.com>
dlt_with_filename_and_line_number() stores the filename length in the
uint8_t dlt_user.filenamelen field, but used the truncated value to size
the heap allocation while copying the full filename with strcpy().
When the source filename exceeds UINT8_MAX bytes, the length is
truncated modulo 256, causing the allocated buffer to be smaller than
the source string. The subsequent strcpy() then writes past the end of
the heap buffer.
This can occur with long compile-time __FILE__ paths (for example,
absolute paths in large monorepos) or when applications forward
user-controlled filenames through this API.
Clamp the filename length to UINT8_MAX before storing it, allocate the
buffer using the clamped length, and copy only that many bytes followed
by an explicit null terminator. This preserves the existing V2 protocol
limit while preventing the heap overflow.
Signed-off-by: Matt Jones
<47545907+SoundMatt@users.noreply.github.com>
Signed-off-by: Santhosh Sivan Murugan
<122221363+santhoshsivanhere@users.noreply.github.com>
Correct several issues in dlt_daemon_control_message_unregister_context_v2() when building the UNREGISTER_CONTEXT V2 response. The response size calculation accumulated the payload length in a uint8_t, allowing it to wrap for large APID and CTID values. This could underallocate the response buffer and lead to a heap buffer overflow during serialization. The response structure also failed to initialize and populate its APID and CTID fields correctly. The structure's apid and ctid members are char * pointers, but they were passed directly to dlt_set_id_v2() while still NULL, so the helper copied nothing. The corresponding length fields were likewise left unset, resulting in incorrect response payloads and undefined behaviour during serialization. Zero-initialize the response structure, compute the payload size using a uint32_t accumulator, copy APID and CTID into local fixed-size buffers through dlt_set_id_v2(), and point the response structure at those buffers for serialization. Also initialize the length fields and guard variable-length copies against NULL pointers. This preserves ID initialization through the shared helper, avoids aliasing the caller's buffers, prevents size calculation overflow, and ensures the response is serialized correctly. Closes: COVESA#867 Related: COVESA#866 Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Santhosh Sivan Murugan <122221363+santhoshsivanhere@users.noreply.github.com>
Rewrite the handler to parse the GET request field-by-field instead of casting msg->databuffer directly to DltServiceSetLogLevelV2 *. The struct contains char *apid and char *ctid pointer fields, so the direct cast caused these pointers to be populated with attacker-controlled network data and later dereferenced. The previous implementation also triggered the same NULL-dereference issue identified in COVESA#863. Add bounds checks for the attacker-controlled APID and CTID length fields to ensure variable-length reads remain within msg->databuffer. Copy APID/CTID into local fixed-size buffers using the shared dlt_set_id_v2() helper and point apid/ctid to those buffers. This keeps ID initialisation within the helper API instead of aliasing the receive buffer, consistent with the fixes in COVESA#864, COVESA#868, and COVESA#861. Related: COVESA#866. Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com> Signed-off-by: Santhosh Sivan Murugan <122221363+santhoshsivanhere@users.noreply.github.com>
…r_info
Three chained strcat() calls build a path as dir + "/" + d_name into the
fixed-size tmpfile[] buffer (DLT_OFFLINE_LOGSTORAGE_MAX_LOG_FILE_LEN + 1,
i.e. 121 bytes).
Although dir is derived from dirname() applied to a configuration field
bounded by DLT_OFFLINE_LOGSTORAGE_MAX_FILE_NAME_LEN (100 bytes), d_name is
returned by scandir() and can be up to NAME_MAX (255 bytes) on Linux.
Therefore, dir + "/" + d_name can exceed the tmpfile buffer size and cause
a stack buffer overflow.
Replace the chained strcat() calls with a single snprintf() using
sizeof(tmpfile) to enforce the destination buffer limit. Also replace the
no-directory branch with strncpy() for consistent bounded string handling.
Signed-off-by: Matt Jones
<47545907+SoundMatt@users.noreply.github.com>
Signed-off-by: Santhosh Sivan Murugan
<122221363+santhoshsivanhere@users.noreply.github.com>
santhoshsivanhere
force-pushed
the
combine/related-prs
branch
from
July 13, 2026 10:03
98d5c8e to
379c52b
Compare
santhoshsivanhere
marked this pull request as ready for review
July 13, 2026 10:06
santhoshsivanhere
changed the title
santhoshsivanhere
force-pushed
the
combine/related-prs
branch
2 times, most recently
from
July 13, 2026 10:12
98d5c8e to
379c52b
Compare
minminlittleshrimp
changed the title
minminlittleshrimp
approved these changes
Jul 23, 2026
Collaborator
|
Hi @SoundMatt |
Collaborator
|
Solved here #911 |
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
4 participants
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.
Combine multiple related PRs #861, #862, #864, #865, #868, #869 #873
Summary
This PR consolidates several interdependent / closely related pull requests into a single, cohesive change set. Combining them reduces merge friction, ensures consistent testing, and simplifies the review process for the team.
The following PRs are squashed / merged into this branch:
Changes included