daemon: fix OOB read and parse bug in dlt_daemon_control_get_log_info_v2 (closes #870)#861
Open
SoundMatt wants to merge 2 commits into
Open
daemon: fix OOB read and parse bug in dlt_daemon_control_get_log_info_v2 (closes #870)#861SoundMatt wants to merge 2 commits into
SoundMatt wants to merge 2 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.
8a8a078 to
cc31ed3
Compare
This was referenced May 5, 2026
Companion fix to the OOB-read fix in this same commit series. 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. Replace the no-op dlt_set_id_v2 calls with conditional pointer assignments into msg->databuffer, mirroring the surgical approach taken for set_log_level_v2 in PR COVESA#864 and unregister_context_v2 in PR COVESA#868. The bounds checks added in the previous commit ensure the pointer-into-databuffer assignments are safe. Closes COVESA#870. Related: COVESA#866.
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.
Problem
dlt_daemon_control_get_log_info_v2had two compounding bugs, eachaddressed by one commit in this PR.
1. Out-of-bounds read parsing apidlen / ctidlen
The fixed-size precheck only validated the 11-byte minimum
GET_LOG_INFO V2 request before the function read two attacker-
controlled
uint8_tlength fields (apidlen,ctidlen) and usedthem to advance an offset into
msg->databuffer. A short messagewith non-zero length fields caused the variable-length reads to walk
past the end of
msg->databuffer(up to ~514 bytes). Reachable fromany client able to talk to the daemon's control socket. Bounded OOB
read; no OOB write.
2. Function never actually parses apid / ctid (#870)
reqis calloc'd, so itschar *apid/char *ctidpointer fieldsstart NULL. The
dlt_set_id_v2(req->apid, ...)calls intended topopulate them are no-ops (the helper early-returns when its
destination is NULL).
req->apid/req->ctidtherefore stayedNULL and were passed to
dlt_daemon_application_find_v2/dlt_daemon_context_find_v2with non-zero corresponding lengths —turning every non-empty apid / ctid lookup into a zero-length one.
Full analysis in #870.
Fix
Two commits:
freeing the calloc'd
reqand returning when the message cannotsatisfy the next read.
dlt_set_id_v2(req->apid, ...)calls withconditional pointer assignments into
msg->databuffer,mirroring the surgical approach taken for
set_log_level_v2indaemon: parse apid/ctid in dlt_daemon_control_set_log_level_v2 #864 and
unregister_context_v2in daemon: fix multi-bug response builder in dlt_daemon_control_message_unregister_context_v2 (closes #867) #868.reqshares storage with the message buffer for the duration ofthis function call, so the pointer-into-databuffer assignments are
lifetime-safe.
Notes
control-handler family cleanup enumerated in Multiple V2 control-message handlers are broken by char* fields in protocol structs (set_log_level_v2 [#863], set_trace_status_v2, unregister_context_v2, get_log_info_v2) #866.
(the calloc'd
reqis freed only on the normal exit ~line 2532).Not addressed here to keep scope focused — could be a separate
cleanup PR.
Closes #870.
Related: #866 (META).