Skip to content

lib: fix heap buffer overflow in dlt_with_filename_and_line_number#865

Open
SoundMatt wants to merge 1 commit into
COVESA:masterfrom
SoundMatt:fix/dlt-with-filename-heap-overflow
Open

lib: fix heap buffer overflow in dlt_with_filename_and_line_number#865
SoundMatt wants to merge 1 commit into
COVESA:masterfrom
SoundMatt:fix/dlt-with-filename-heap-overflow

Conversation

@SoundMatt
Copy link
Copy Markdown

Problem

dlt_with_filename_and_line_number() in src/lib/dlt_user.c writes
past the end of a heap allocation when strlen(fina) > 255:

dlt_user.filenamelen = (uint8_t)strlen(fina);          // truncates
...
dlt_user.filename = malloc(dlt_user.filenamelen + 1);  // small
strcpy(dlt_user.filename, fina);                       // full

filenamelen is uint8_t (matches the V2 wire-format field width
declared in include/dlt/dlt_user.h.in:235), so the cast truncates
strlen(fina) modulo 256. The subsequent malloc allocates the
truncated length + 1, but strcpy copies the entire fina including
its real terminator — writing strlen(fina) - filenamelen bytes past
the end of the heap chunk.

Reachable in practice via long compile-time __FILE__ paths
(monorepo-rooted absolute paths regularly exceed 255 chars) and any
application that forwards a user-controlled filename through this
function. Heap-write primitive in the linked-into-everything client
library.

Fix

Clamp strlen(fina) to UINT8_MAX before assigning filenamelen,
allocate against the clamped length, and copy with memcpy + explicit
null terminator instead of strcpy. The on-wire field still gets the
maximum the V2 protocol can encode.

Notes

  • I have not added a unit test in this PR. If maintainers can point me
    at a harness for dlt_user.c (e.g. via gtest_dlt_user.cpp) I'm
    happy to follow up with one.
  • Per-call filename length (dlt_user_param.filenamelen at line
    6555) appears to use the same uint8_t width. Worth a separate
    audit pass to confirm callers can't trigger the same pattern; not
    in this PR's scope.

When the source filename string is longer than 255 bytes,
dlt_with_filename_and_line_number(fina, linr) overflows the heap
buffer it allocates for dlt_user.filename:

  dlt_user.filenamelen = (uint8_t)strlen(fina);          // truncates
  ...
  dlt_user.filename = malloc(dlt_user.filenamelen + 1);  // small
  strcpy(dlt_user.filename, fina);                       // full

filenamelen is uint8_t (matching the V2 wire-format field width
declared in include/dlt/dlt_user.h.in), so the cast truncates
strlen(fina) modulo 256. The subsequent malloc allocates the
truncated length + 1, but strcpy copies the entire fina including
its real terminator, writing strlen(fina) - filenamelen bytes past
the end of the heap chunk.

Reachable in practice via long compile-time __FILE__ paths
(monorepo-rooted absolute paths) and any application that forwards
a user-controlled filename through this function.

Fix: clamp strlen(fina) to UINT8_MAX before assigning filenamelen,
allocate against the clamped length, and copy with memcpy + explicit
null terminator instead of strcpy. The on-wire field still gets the
truncated length, which matches the maximum the V2 protocol can
encode.
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