lib: fix heap buffer overflow in dlt_with_filename_and_line_number#865
Open
SoundMatt wants to merge 1 commit into
Open
lib: fix heap buffer overflow in dlt_with_filename_and_line_number#865SoundMatt wants to merge 1 commit into
SoundMatt wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
dlt_with_filename_and_line_number()insrc/lib/dlt_user.cwritespast the end of a heap allocation when
strlen(fina) > 255:filenamelenisuint8_t(matches the V2 wire-format field widthdeclared in
include/dlt/dlt_user.h.in:235), so the cast truncatesstrlen(fina)modulo 256. The subsequentmallocallocates thetruncated length + 1, but
strcpycopies the entirefinaincludingits real terminator — writing
strlen(fina) - filenamelenbytes pastthe 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)toUINT8_MAXbefore assigningfilenamelen,allocate against the clamped length, and copy with
memcpy+ explicitnull terminator instead of
strcpy. The on-wire field still gets themaximum the V2 protocol can encode.
Notes
at a harness for
dlt_user.c(e.g. viagtest_dlt_user.cpp) I'mhappy to follow up with one.
dlt_user_param.filenamelenat line6555) appears to use the same
uint8_twidth. Worth a separateaudit pass to confirm callers can't trigger the same pattern; not
in this PR's scope.