COVESA / COVESA/dlt-daemon

V2 control handlers narrow uint8_t apid/ctid length to int8_t, causing negative buffer index for lengths > 127

Open
#884 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

Several V2 control-message handlers in `src/daemon/dlt_daemon_client.c` store the application/context id **length** (a `uint8_t`, range 0–255, parsed straight from the wire) into an `int8_t` local variable, then use it to index the id buffer as `id[length - 1]`. For any length in the range **128–255** the `int8_t` value is **negative**, so:

- `id[length - 1]` performs a **negative (out-of-bounds) array index**, reading stack memory before the buffer, and
- the negative length is passed on to the lookup/wildcard helpers, corrupting the comparison logic.

This is independent of (and pre-dates) the `char *` / `dlt_set_id_v2` no-op family tracked in #866 — it remains even after those handlers are fixed to populate the ids correctly.

## Affected code

`dlt_daemon_control_set_log_level_v2`:
```c
int8_t apid_length = 0;
int8_t ctid_length = 0;
...
apid_length = (int8_t) req.apidlen; /* req.apidlen is uint8_t (0..255) */
ctid_length = (int8_t) req.ctidlen;
...
if ((apid_length != 0) && (apid[apid_length - 1] == '*') && (ctid == NULL)) { ... }
```

`dlt_daemon_control_set_trace_status_v2` has the same `int8_t apid_length / ctid_length` pattern.

The downstream helpers compound the problem by also taking signed 8-bit lengths:
```c
void dlt_daemon_find_multiple_context_and_send_log_level_v2(int sock, DltDaemon *daemon,
DltDaemonLocal *daemon_local, int8_t app_flag, char *str, int8_t len, ...);
```
and the handler then casts back with `(uint8_t)apid_length` when calling `dlt_daemon_context_find_v2`, so a value like 200 round-trips as 200 → -56 → 200 inconsistently depending on the path taken.

## Worked example

`apidlen = 200` (0xC8):
- `apid_length = (int8_t)200 = -56`
- `apid_length != 0` is true
- `apid[apid_length - 1]` = `apid[-57]` → **OOB read before the buffer**

## Impact

Attacker-influenced `apidlen` / `ctidlen` values above 127 trigger an out-of-bounds stack read and broken id matching in the SET_LOG_LEVEL and SET_TRACE_STATUS V2 handlers. (With the #866 fixes, the ids are copied into `char[DLT_V2_ID_SIZE]` buffers, so the negative index reads adjacent stack rather than wild memory — still undefined behaviour and a logic bug.)

## Suggested fix

Use an unsigned width consistently for the id lengths end to end:
- declare `apid_length` / `ctid_length` as `uint8_t` (or `int` / `size_t`) in the handlers;
- widen the `int8_t len` parameter of `dlt_daemon_find_multiple_context_and_send_log_level_v2` / `..._trace_status_v2` accordingly;
- drop the `(int8_t)` casts.

A wildcard length of 255 is legitimately representable as `uint8_t`, so no value in the protocol range should ever go negative.

## Notes

Flagged in passing while addressing the #866 handler family (PRs #864 / #869); deliberately left out of those PRs to keep them focused, since the proper fix touches the shared helper signatures.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in src/daemon/dlt_daemon_client.c with dlt_daemon_control_set_log_level_v2 and dlt_daemon_control_set_trace_status_v2, then trace their calls to the shared find_multiple_context_and_send_log_level_v2 and trace-status helpers. Check every affected length parameter and index path against the 0–255 protocol range. Done means lengths remain nonnegative throughout both handlers and helpers, including wildcard handling, without changing the separate #866 work.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
backend, security
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
70/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.