fix(logstorage): replace unbounded strcat with snprintf in storage_dir_info#873
Open
SoundMatt wants to merge 3 commits into
Open
fix(logstorage): replace unbounded strcat with snprintf in storage_dir_info#873SoundMatt wants to merge 3 commits into
SoundMatt wants to merge 3 commits into
Conversation
The fixed-size precheck at the entry of this function validates only the 11-byte minimum GET_LOG_INFO V2 request (apidlen = ctidlen = 0). The function then reads two attacker-controlled uint8_t length fields (apidlen, ctidlen) from msg->databuffer and uses them to advance an offset, before passing pointers into the buffer to dlt_set_id_v2() (which reads up to apidlen / ctidlen bytes via dlt_strnlen_s) and finishing with a 4-byte memcpy of the trailing com field. A short message with non-zero length fields causes the variable-length reads to walk past the end of msg->databuffer. Add bounds checks after each length is parsed, freeing the calloc'd request and returning when the message cannot satisfy the next read. Note: the existing early-return paths inside this function already leak the calloc'd req pointer; left untouched here to keep this commit focused on the security fix.
Companion fix to the OOB-read fix in this same commit series. The function never assigned req->apid or req->ctid: req is calloc'd, so the char * pointer fields start NULL, and the dlt_set_id_v2(req->apid, ...) call to populate them was a no-op (dlt_set_id_v2 early-returns when its destination is NULL). req->apid / req->ctid stayed NULL and were then passed to dlt_daemon_application_find_v2 and dlt_daemon_context_find_v2 despite req->apidlen / req->ctidlen being non-zero — every non-empty lookup was silently turned into a zero-length one. Replace the no-op dlt_set_id_v2 calls with conditional pointer assignments into msg->databuffer, mirroring the surgical approach taken for set_log_level_v2 in PR COVESA#864 and unregister_context_v2 in PR COVESA#868. The bounds checks added in the previous commit ensure the pointer-into-databuffer assignments are safe. Closes COVESA#870. Related: COVESA#866.
…r_info Three chained strcat() calls build a path as dir + "/" + d_name into a fixed-size tmpfile[] buffer (DLT_OFFLINE_LOGSTORAGE_MAX_LOG_FILE_LEN+1, i.e. 121 bytes). dir comes from dirname() applied to a config field that is bounded by DLT_OFFLINE_LOGSTORAGE_MAX_FILE_NAME_LEN (100 chars), but d_name is returned by scandir() and can be up to NAME_MAX (255 chars) on Linux. dir(~100) + "/" + d_name(~255) can easily exceed 121 bytes, overflowing tmpfile on the stack. Replace with a single snprintf(..., sizeof(tmpfile), "%s/%s", dir, files[i]->d_name) which hard-limits the write to the buffer size, and use strncpy for the no-dir branch to be consistent. Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com>
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.
Summary
dlt_logstorage_storage_dir_info()indlt_offline_logstorage_behavior.cbuilds a file path from three pieces using three chainedstrcat()calls into a fixed-size stack buffer:diris derived from a config-bounded field (maxDLT_OFFLINE_LOGSTORAGE_MAX_FILE_NAME_LEN= 100 chars), so the directory component is bounded. Howeverfiles[i]->d_namecomes fromscandir()and can be up toNAME_MAX(255 bytes) on Linux.dir(~99) + "/" + d_name(~255) = ~355 bytescan easily overflow the 121-bytetmpfilebuffer, causing a stack buffer overflow. The overflow is triggerable whenever the storage directory contains a file whose name exceeds the buffer capacity, which can happen naturally or via a crafted mount-point with long filenames.Changes
Replace the three
strcat()calls with a singlesnprintf(..., sizeof(tmpfile), ...), which hard-limits the write to the buffer size. Add astrncpyfor the no-directory branch for consistency.Testing
DLT_OFFLINE_LOGSTORAGE_MAX_LOG_FILE_LENare copied unchanged.