fix(convert): prevent argv out-of-bounds write and use PID-specific workspace#842
Open
aki1770-del wants to merge 1 commit into
Open
fix(convert): prevent argv out-of-bounds write and use PID-specific workspace#842aki1770-del wants to merge 1 commit into
aki1770-del wants to merge 1 commit into
Conversation
…orkspace
Two security issues in dlt-convert's -t (tarball) mode:
1. ARGV OUT-OF-BOUNDS WRITE (CVE-class: CWE-131)
After extracting files to the workspace, argc was overwritten:
argc = optind + (n - 2); // n from scandir of /tmp workspace
A subsequent loop then wrote:
argv[index] = tmp_filename; // index may exceed original argc
If an attacker pre-populated /tmp/dlt_convert_workspace/ with
extra files before execution, 'n' would be inflated, argc
extended beyond the original argv array bounds, and subsequent
argv[index] writes would corrupt the stack.
Fix: introduce 'current_file' and set it to tmp_filename (tflag)
or argv[index] (no tflag) without ever writing back to argv[].
2. PREDICTABLE WORKSPACE PATH (CWE-377)
The fixed path /tmp/dlt_convert_workspace/ allowed any local user
to pre-create the directory with arbitrary files before execution,
triggering the overflow above or injecting unexpected .dlt files.
Fix: use a PID-specific path /tmp/dlt_convert_ws_<pid>/ which
is unique per invocation and not guessable by other processes.
Fixes COVESA#793
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-convert -t <tarball>has two security issues:1. argv out-of-bounds write (CWE-131 / stack corruption)
After extracting files to the workspace directory, the code overwrites
argc:A subsequent loop then writes to
argv[index]using the inflatedargc:If a malicious local user pre-populates
/tmp/dlt_convert_workspace/with extra files before execution,nis inflated,argcis set beyond the original argv array bounds, andargv[index]writes corrupt the stack — leading to a stack overflow as demonstrated with AddressSanitizer in #793.2. Predictable workspace path (CWE-377)
The fixed path
/tmp/dlt_convert_workspace/is world-writable and predictable. Any local user can pre-create it with arbitrary contents beforedlt-convertis run, triggering the overflow or injecting unexpected.dltfiles.Fixes #793.
Fix
For (1): Introduce a
current_filepointer. In the processing loop, set it totmp_filenamein tarball mode orargv[index]otherwise. Removeargv[index] = tmp_filenameentirely — the originalargvarray is never written to.For (2): Use a PID-specific workspace path
/tmp/dlt_convert_ws_<pid>/(format stringDLT_CONVERT_WS_FMT). The path is unique per invocation and not guessable by other local processes.Testing
dlt-convert.c#793 (pre-populated/tmp/dlt_convert_ws_*/) — no stack overflow occurs-toperation with a valid tar file continues to work correctly