daemon: parse incrementally in dlt_daemon_control_set_trace_status_v2 (V2 control-handler family, ref #866)#869
Open
SoundMatt wants to merge 1 commit into
Conversation
This handler had three compounding bugs from the V2 control-handler pattern documented in COVESA#866: 1. Direct cast of msg->databuffer to DltServiceSetLogLevelV2 *. That struct contains `char *apid` and `char *ctid` pointer fields, so under the cast those fields took attacker-controlled pointer values from network input. dlt_set_id_v2(apid, req->apid, ...) then read from req->apid — an arbitrary memory read. 2. Same NULL-deref pattern as COVESA#863 / COVESA#864: locals apid / ctid declared NULL, dlt_set_id_v2(apid, ...) silently no-ops on the NULL destination, then apid[apid_length - 1] dereferences NULL when apid_length != 0. 3. (int8_t) cast on uint8_t req->apidlen / ctidlen produces a negative value when the length exceeds 127, then apid[apid_length - 1] underflows to a negative array index. Fix: parse the wire bytes incrementally into local primitives, point apid / ctid into msg->databuffer when their lengths are non-zero, add bounds checks (mirroring COVESA#862 for set_log_level_v2), and use unsigned widths in this function. Downstream helpers still take int8_t lengths — that narrowing is a separate latent issue, called out in a code comment, out of scope here. Related: COVESA#866. Mirrors the surgical approach taken in COVESA#864.
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_set_trace_status_v2was the most severe of theV2 control-handler bugs documented in #866 — it cast
msg->databufferdirectly to
DltServiceSetLogLevelV2 *:DltServiceSetLogLevelV2containschar *apidandchar *ctidpointer fields. Under the direct cast, those fields were filled with
8 bytes of network input each (interpreted as pointer values), then
dereferenced by
dlt_set_id_v2. Arbitrary memory read from networkinput. The daemon either crashed (most pointers invalid) or read
from an attacker-chosen address.
Plus two compounding bugs:
apid/ctiddeclared NULL, never reassigned, dereferenced at
apid[apid_length - 1]etc.(int8_t)cast onuint8_tapidlen / ctidlen produced a negativevalue for lengths > 127, leading to a negative array-index
underflow at the same dereference points.
Reachable from any client able to send control messages (dispatched
from
dlt_daemon_handle_control_messages_v2, line 1278).Fix
Replace the struct cast with incremental parsing, mirroring the
surgical approach already taken for
set_log_level_v2in #864:memcpyinto a local primitive.apid/ctidintomsg->databufferonly when theircorresponding length is non-zero.
(int8_t)casts onapid_length/ctid_length.Notes
dlt_daemon_find_multiple_context_and_send_trace_status_v2,dlt_daemon_send_trace_status_v2) still takeint8_tlengthparameters. That narrowing remains a separate latent issue — a
caller passing
apidlen > 127(legal in the wire protocol'suint8_trange) still gets a sign-flipped length passeddownstream. Called out in a code comment; out of scope for this
PR. Will follow up if maintainers want.
only" / "exact match" branches) — only the parsing and the local
variable widths changed.
size constant
DLT_SERVICE_SET_LOG_LEVEL_FIXED_SIZE_V2 = 11. Havenot added a regression test.
Related: #866 (META — V2 control-handler systemic problem).
Mirrors: #864 (same surgical approach for
set_log_level_v2).