dlt_daemon_control_message_unregister_context_v2 underallocates response, leaks uninitialised stack to wire, and may segfault on every context unregistration

[SoundMatt]

Function-specific follow-up to #866.

Summary

dlt_daemon_control_message_unregister_context_v2 in
src/daemon/dlt_daemon_client.c (around line 2807) has four
distinct bugs, all stemming from the V2-control-handler pattern
documented in #866. The function builds an UNREGISTER_CONTEXT V2
notification message that is broadcast to clients whenever an
application unregisters a context. It is invoked every time a
registered application disconnects (from dlt-daemon.c:5001).

The function as written

int dlt_daemon_control_message_unregister_context_v2(int sock,
                                                  DltDaemon *daemon,
                                                  DltDaemonLocal *daemon_local,
                                                  uint8_t apidlen,
                                                  char *apid,
                                                  uint8_t ctidlen,
                                                  char *ctid,
                                                  char *comid,
                                                  int verbose)
{
    DltMessageV2 msg;
    DltServiceUnregisterContextV2 resp;          // not zero-initialised
    uint8_t contextSize = 0;
    resp.apid = NULL;                            // explicit
    resp.ctid = NULL;                            // explicit
    int offset = 0;
    /* resp.apidlen, resp.ctidlen, resp.service_id, resp.status,
     * resp.comid all UNINITIALISED at this point. */

    PRINT_FUNCTION_VERBOSE(verbose);

    if (daemon == 0)
        return -1;

    if (dlt_message_init_v2(&msg, 0) == DLT_RETURN_ERROR)
        return -1;

    contextSize = (uint8_t)(sizeof(uint32_t) + sizeof(uint8_t) + sizeof(uint8_t)
                          + apidlen + sizeof(uint8_t) + ctidlen + DLT_ID_SIZE);
    msg.datasize = contextSize;
    ...
    msg.databuffer = (uint8_t *)malloc((size_t)msg.datasize);
    msg.databuffersize = msg.datasize;
    ...

    resp.service_id = DLT_SERVICE_ID_UNREGISTER_CONTEXT;
    resp.status = DLT_SERVICE_RESPONSE_OK;
    dlt_set_id_v2(resp.apid, apid, resp.apidlen);   // NULL dest → no-op
    dlt_set_id_v2(resp.ctid, ctid, resp.ctidlen);   // NULL dest → no-op
    dlt_set_id(resp.comid, comid);

    memcpy(msg.databuffer + offset, &(resp.service_id), sizeof(uint32_t));
    offset = offset + (int)sizeof(uint32_t);
    memcpy(msg.databuffer + offset, &(resp.status), sizeof(uint8_t));
    offset = offset + (int)sizeof(uint8_t);
    memcpy(msg.databuffer + offset, &(resp.apidlen), sizeof(uint8_t));   // ← uninit
    offset = offset + (int)sizeof(uint8_t);
    memcpy(msg.databuffer + offset, resp.apid, resp.apidlen);            // ← NULL src
    offset = offset + (int)resp.apidlen;
    memcpy(msg.databuffer + offset, &(resp.ctidlen), sizeof(uint8_t));   // ← uninit
    offset = offset + (int)sizeof(uint8_t);
    memcpy(msg.databuffer + offset, resp.ctid, resp.ctidlen);            // ← NULL src
    offset = offset + (int)resp.ctidlen;
    memcpy(msg.databuffer + offset, resp.comid, DLT_ID_SIZE);
    ...
}

The four bugs

1. Heap underallocation via (uint8_t) truncation (line 2834)

contextSize is uint8_t. The summed expression evaluates to up to
4 + 1 + 1 + 255 + 1 + 255 + 4 = 521 bytes, but the cast wraps mod
256. With apidlen + ctidlen ≳ 245, contextSize is much smaller
than the actual write footprint. msg.datasize then underallocates,
and the subsequent memcpy chain writes past the heap allocation.

2. Stack-memory disclosure to the wire (lines 2861, 2865)

resp.apidlen and resp.ctidlen are never assigned from the
function's apidlen / ctidlen parameters. The stack-allocated
resp is not zero-initialised. The memcpy of &(resp.apidlen) /
&(resp.ctidlen) into the response thus copies whatever happened to
be on the stack. Two bytes of recent stack contents leak to every
client receiving the unregistration message, every time an
application disconnects. Function-pointer addresses, recently-used
secrets, and arbitrary other stack memory could be exposed.

3. memcpy from NULL source with arbitrary length (lines 2863, 2867)

resp.apid is explicitly NULL; never reassigned. resp.apidlen
is uninitialised stack memory (see #2 above). memcpy(dst, NULL, random_length) is undefined behaviour — typically segfaults if
random_length happens to be non-zero. Same for resp.ctid /
resp.ctidlen. Whether the daemon survives an unregistration depends
on stack-state luck.

4. The intended values are never written to the response

The parameters apidlen, apid, ctidlen, ctid are passed to the
function but never copied into resp. Even when bugs #2 and #3
happen to be benign (uninitialised len = 0, lucky), the wire response
carries apidlen = <uninit byte>, apid = <missing>,
ctidlen = <uninit byte>, ctid = <missing>. Clients receive
malformed unregister notifications that don't identify which
context unregistered.

Reproduction

Connect to a running dlt-daemon as a client, register an app and
context with apidlen / ctidlen near 4 (typical), then disconnect.
The daemon will execute this function. With apidlen + ctidlen well
under 245 the heap-underalloc bug doesn't bite immediately, but bug
#3 (NULL memcpy) will fire if the stack state produces non-zero
resp.apidlen. Triggering #1 reliably requires an
attacker-controlled context register with very large apid/ctid (the
length validation upstream of this function would need separate
review to confirm reachability).

Proposed fix

Assign all resp fields explicitly before serialisation:

DltServiceUnregisterContextV2 resp = {0};   /* zero everything by default */

resp.service_id = DLT_SERVICE_ID_UNREGISTER_CONTEXT;
resp.status     = DLT_SERVICE_RESPONSE_OK;
resp.apidlen    = apidlen;
resp.apid       = apid;
resp.ctidlen    = ctidlen;
resp.ctid       = ctid;
dlt_set_id(resp.comid, comid);

Replace the (uint8_t) contextSize with a wider type that matches
msg.datasize:

uint32_t contextSize = sizeof(uint32_t) + sizeof(uint8_t) + sizeof(uint8_t)
                     + apidlen + sizeof(uint8_t) + ctidlen + DLT_ID_SIZE;
msg.datasize = (int32_t)contextSize;

The serialisation memcpy chain then writes the correct lengths
and the actual apid/ctid bytes (from the resp.apid / resp.ctid
pointers, which now point at the function's parameters).

PR with this fix is in flight.