COVESA / COVESA/dlt-daemon

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)

Open
#866 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C
Stars
459
Forks
340
Avg merge
4d 21h
Merged PRs (30d)
2

Description

## Summary

Four V2 control-message handlers in `src/daemon/dlt_daemon_client.c`
are broken because the `DltService...V2` protocol structs they use
were defined with `char *` pointer fields (`apid`, `ctid`, `ecid`)
where inline arrays or properly-assigned pointers were intended. The
handlers either:

- cast `msg->databuffer` directly to one of these structs, in which
case the pointer fields are interpreted as 8 bytes of network input
(arbitrary-pointer dereference); or
- declare a stack/calloc'd struct, leaving the pointer fields NULL,
then call `dlt_set_id_v2(struct.apid, ...)` which silently returns
early because the destination is NULL; or
- never assign the length/pointer fields at all, leaving uninitialised
stack values to be written to the wire and used as `memcpy` lengths.

Reachable from any client able to send control messages to the
daemon's control socket.

## Affected functions

All four are in `src/daemon/dlt_daemon_client.c`:

### 1. `dlt_daemon_control_set_log_level_v2` (line ~3660)

Already filed as **#863** and patched in **#864**. Stack-allocated
`req`, NULL `req.apid` / `req.ctid` never assigned, NULL deref at
line 3723 when `apidlen != 0`.

### 2. `dlt_daemon_control_set_trace_status_v2` (line ~4020)

**Most severe.** Direct cast of `msg->databuffer` to
`DltServiceSetLogLevelV2 *`:

```c
req = (DltServiceSetLogLevelV2 *)(msg->databuffer);
...
dlt_set_id_v2(apid, req->apid, req->apidlen);
```

`req->apid` is now whatever 8 bytes happen to be at the right offset
in the wire data — an attacker-controlled pointer. `dlt_set_id_v2`
later reads from `req->apid`. **Arbitrary memory read from network
input.** Daemon either crashes (random pointer rarely valid) or leaks
memory at the attacker-supplied address.

Also has the same NULL-local-`apid`/`ctid` deref pattern as #863, and
the same `(int8_t)` cast on `req->apidlen` which produces a negative
array index when `apidlen > 127`.

### 3. `dlt_daemon_control_message_unregister_context_v2` (line ~2807)

Outbound (server-built) control message. Four bugs in one function:

- `contextSize = (uint8_t)(... + apidlen + ... + ctidlen + DLT_ID_SIZE);`
truncates when `apidlen + ctidlen ≳ 245`. `msg.datasize` then
underallocates, downstream `memcpy` overflows the heap.
- `resp.apidlen` and `resp.ctidlen` are never assigned from the
function's `apidlen` / `ctidlen` parameters, but are written
to the wire response — leaking uninitialised stack memory.
- `resp.apid` / `resp.ctid` are explicitly set to NULL, never
reassigned. `memcpy(buf, resp.apid, resp.apidlen)` is then
`memcpy(buf, NULL, garbage)` — UB.
- The intended values (the function's parameters) are never copied
into `resp`, so even when the function doesn't crash the wire
response carries wrong lengths and zero context bytes.

Triggered every time a registered application unregisters a context
(called from `dlt-daemon.c:5001`).

### 4. `dlt_daemon_control_get_log_info_v2` (line ~2098)

`req` is calloc'd, so its `char *apid` and `char *ctid` start NULL.
The `dlt_set_id_v2(req->apid, ...)` calls at lines 2154/2158 are
no-ops because the destination pointer is NULL. `req->apid` is then
passed to `dlt_daemon_context_find_v2` (line 2199) and
`dlt_daemon_application_find_v2` (line 2186) while still NULL.

**The OOB-read fix in #861 addressed the bounds-check problem but
did not address this deeper parsing bug.** Will follow up with a
separate PR.

## Root cause

The shared `DltService...V2` structs (`include/dlt/dlt_common.h`)
declare:

```c
char *apid; // pointer
char *ctid; // pointer
```

…where the AUTOSAR DLT V2 wire format places the bytes inline:

```
[apidlen:1][apid:apidlen][ctidlen:1][ctid:ctidlen]
```

A `char *` cannot be filled directly from inline wire data, so each
handler has to either copy bytes out (which the existing
`dlt_set_id_v2` calls would do *if* the destination were a real
buffer rather than a NULL pointer) or point the field into the
message buffer. Most handlers do neither.

## Suggested approach

Per-handler, parse the wire bytes incrementally into local variables
(or point `req.apid` / `req.ctid` into `msg->databuffer` when each
length is non-zero, as #864 does). Don't cast `msg->databuffer`
directly to these structs.

Alternative (more invasive): redefine the structs with inline arrays
of `DLT_V2_ID_SIZE` to make casting safe — but this changes the ABI
for any external user of the headers.

I will follow up with a separate Issue + PR for each affected
function. The first follow-up
(`dlt_daemon_control_message_unregister_context_v2`) is in flight.

## Related

- #861 — partial fix to `dlt_daemon_control_get_log_info_v2` (OOB
read only).
- #862 — OOB read fix in `dlt_daemon_control_set_log_level_v2`.
- #863 — function-specific issue for #1 above.
- #864 — PR fixing #863.
- #865 — unrelated heap overflow in `dlt_with_filename_and_line_number`.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.