Skip to content

daemon: fix out-of-bounds read in dlt_daemon_control_set_log_level_v2#862

Open
SoundMatt wants to merge 1 commit into
COVESA:masterfrom
SoundMatt:fix/set-log-level-v2-oob-read
Open

daemon: fix out-of-bounds read in dlt_daemon_control_set_log_level_v2#862
SoundMatt wants to merge 1 commit into
COVESA:masterfrom
SoundMatt:fix/set-log-level-v2-oob-read

Conversation

@SoundMatt
Copy link
Copy Markdown

Problem

Same shape as #861, applied to the parallel SET_LOG_LEVEL V2 handler.

dlt_daemon_control_set_log_level_v2 validates only the 11-byte fixed
portion of the SET_LOG_LEVEL V2 request before reading two attacker-
controlled uint8_t length fields (apidlen, ctidlen) and using
them to advance a buffer offset. A short message with non-zero length
fields causes dlt_set_id_v2(), the trailing 1-byte log_level read,
and the final 4-byte memcpy() of the com field to read past the
end of msg->databuffer. Reachable from any client that can talk to
the daemon's control socket. Bounded OOB read; no OOB write.

Fix

Two bounds checks:

  • After apidlen is parsed, verify the message contains
    apidlen + 1 more bytes (apid + the upcoming ctidlen byte).
  • After ctidlen is parsed, verify the message contains
    ctidlen + 1 + DLT_ID_SIZE more bytes (ctid + log_level + com).

Unlike dlt_daemon_control_get_log_info_v2 in #861, the request struct
here lives on the stack, so no allocation cleanup is required on the
early returns.

Notes

  • Natural follow-up to daemon: fix OOB read and parse bug in dlt_daemon_control_get_log_info_v2 (closes #870) #861. Functionally independent — can be merged
    in either order.
  • I noticed but did not address a separate, independent bug in this
    function: at lines 3719/3721 the local apid/ctid pointers
    (declared NULL at 3687/3688) are passed to dlt_set_id_v2() while
    still NULL, so the call is a no-op, yet line 3723 (and later
    branches) dereferences them: apid[apid_length - 1]. This NULL-
    derefs whenever apid_length != 0. Looks like the intent was
    apid = req.apid; (pointer assignment). Filing separately since
    it's an orthogonal bug class.

Mirror of COVESA#861 for the parallel SET_LOG_LEVEL V2 control handler. The
fixed-size precheck validates only the 11-byte minimum SET_LOG_LEVEL V2
request (apidlen = ctidlen = 0). The function then reads two attacker-
controlled uint8_t length fields (apidlen, ctidlen) and uses them to
advance an offset into msg->databuffer, before passing pointers into
the buffer to dlt_set_id_v2() (which reads up to apidlen / ctidlen
bytes), a 1-byte log_level read, and 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; on failure return
without further processing (req is on the stack, no allocation cleanup
required).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant