fix: prevent stack overflow and /tmp race in dlt-convert workspace (#793)#833
Open
aki1770-del wants to merge 1 commit into
Open
fix: prevent stack overflow and /tmp race in dlt-convert workspace (#793)#833aki1770-del wants to merge 1 commit into
aki1770-del wants to merge 1 commit into
Conversation
…OVESA#793) Two related bugs in the -t (compressed file) path of dlt-convert: 1. Workspace not cleared when directory already exists (CWE-377): mkdir() failing with EEXIST did not call empty_dir(). The struct stat was zero-initialised but never populated via stat(), so S_ISDIR() was always false and empty_dir() was never reached. An attacker who pre-creates /tmp/dlt_convert_workspace/ and fills it with files controls the workspace contents before dlt-convert runs. Fix: call stat() before S_ISDIR() in the EEXIST path so the check is meaningful; always call empty_dir() in the fresh-mkdir path (removing the now-dead fprintf branch). 2. argc inflated past original argv bounds (CWE-121, stack overflow): argc = optind + (n - 2) can exceed the original argc when n is large (e.g. attacker planted many files). The subsequent loop assigns argv[index] = tmp_filename for indices beyond the original argv allocation, writing past the stack-allocated argument vector. Fix: save original_argc before the tflag block and cap the inflated argc at that value. CWE-377 (Insecure Temporary File), CWE-121 (Stack-based Buffer Overflow), CWE-20 (Improper Input Validation). Signed-off-by: Akihiko Komada <aki1770@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses two security/robustness bugs in dlt-convert’s -t (tar workspace) path by hardening workspace directory handling under /tmp and preventing argc from being inflated beyond the original argv bounds.
Changes:
- Track
original_argcand clamp the computedargcafter scanning the workspace directory. - Fix the
EEXISTmkdir path by callingstat()before checkingS_ISDIRand cleaning the workspace. - Always call
empty_dir()on the freshly created workspace directory.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+362
to
+365
| /* Directory exists — clear any pre-existing contents (including | ||
| * attacker-planted files) before we use it as a workspace. */ | ||
| if (stat(DLT_CONVERT_WS, &st) == 0 && S_ISDIR(st.st_mode)) | ||
| empty_dir(DLT_CONVERT_WS); |
There was a problem hiding this comment.
The new EEXIST handling uses stat() and then calls empty_dir() when S_ISDIR is true. Because stat() follows symlinks, an attacker can pre-create /tmp/dlt_convert_workspace as a symlink to another directory; running dlt-convert could then delete files outside of /tmp. Use lstat() and explicitly reject symlinks (and ideally verify ownership/permissions) before cleaning or using the workspace path.
Suggested change
| /* Directory exists — clear any pre-existing contents (including | |
| * attacker-planted files) before we use it as a workspace. */ | |
| if (stat(DLT_CONVERT_WS, &st) == 0 && S_ISDIR(st.st_mode)) | |
| empty_dir(DLT_CONVERT_WS); | |
| /* Directory exists — ensure it is a safe, non-symlink directory | |
| * owned by the current user before clearing its contents. */ | |
| if (lstat(DLT_CONVERT_WS, &st) != 0) { | |
| fprintf(stderr,"ERROR: Cannot stat existing temp dir %s!\n", DLT_CONVERT_WS); | |
| if (ovalue) { | |
| close(ohandle); | |
| ohandle = -1; | |
| } | |
| return -1; | |
| } | |
| if (S_ISLNK(st.st_mode) || !S_ISDIR(st.st_mode)) { | |
| fprintf(stderr,"ERROR: Temp path %s is not a directory or is a symlink!\n", DLT_CONVERT_WS); | |
| if (ovalue) { | |
| close(ohandle); | |
| ohandle = -1; | |
| } | |
| return -1; | |
| } | |
| if (st.st_uid != geteuid() || (st.st_mode & 0777) != 0700) { | |
| fprintf(stderr,"ERROR: Temp dir %s has unsafe owner or permissions!\n", DLT_CONVERT_WS); | |
| if (ovalue) { | |
| close(ohandle); | |
| ohandle = -1; | |
| } | |
| return -1; | |
| } | |
| empty_dir(DLT_CONVERT_WS); |
Comment on lines
+362
to
+365
| /* Directory exists — clear any pre-existing contents (including | ||
| * attacker-planted files) before we use it as a workspace. */ | ||
| if (stat(DLT_CONVERT_WS, &st) == 0 && S_ISDIR(st.st_mode)) | ||
| empty_dir(DLT_CONVERT_WS); |
There was a problem hiding this comment.
If mkdir() returns EEXIST and stat() fails or the path is not a directory, the code currently proceeds and later runs tar/cp/scandir against DLT_CONVERT_WS. This should fail fast with a clear error (and close ohandle) rather than continuing with an invalid workspace path.
Suggested change
| /* Directory exists — clear any pre-existing contents (including | |
| * attacker-planted files) before we use it as a workspace. */ | |
| if (stat(DLT_CONVERT_WS, &st) == 0 && S_ISDIR(st.st_mode)) | |
| empty_dir(DLT_CONVERT_WS); | |
| /* Directory exists — verify it is a directory and usable, then | |
| * clear any pre-existing contents (including attacker-planted | |
| * files) before we use it as a workspace. */ | |
| if (stat(DLT_CONVERT_WS, &st) != 0) { | |
| fprintf(stderr,"ERROR: Cannot stat existing temp path %s!\n", DLT_CONVERT_WS); | |
| if (ovalue) { | |
| close(ohandle); | |
| ohandle = -1; | |
| } | |
| return -1; | |
| } | |
| if (!S_ISDIR(st.st_mode)) { | |
| fprintf(stderr,"ERROR: Temp path %s exists but is not a directory!\n", DLT_CONVERT_WS); | |
| if (ovalue) { | |
| close(ohandle); | |
| ohandle = -1; | |
| } | |
| return -1; | |
| } | |
| empty_dir(DLT_CONVERT_WS); |
Comment on lines
401
to
+405
| /* do not include ./ and ../ in the files */ | ||
| argc = optind + (n - 2); | ||
| /* Guard: never exceed the original argv bounds. */ | ||
| if (argc > original_argc) | ||
| argc = original_argc; |
There was a problem hiding this comment.
Clamping argc to original_argc prevents the overflow but also changes behavior: archives that expand into more than (original_argc - optind) files will have the extra extracted files silently ignored. Instead of mutating/clamping argc, consider iterating over the scandir() results directly or allocating a new argv-like array on the heap sized to the discovered file count.
Suggested change
| /* do not include ./ and ../ in the files */ | |
| argc = optind + (n - 2); | |
| /* Guard: never exceed the original argv bounds. */ | |
| if (argc > original_argc) | |
| argc = original_argc; | |
| /* do not include ./ and ../ in the files; n includes "." and ".." */ | |
| if (n > 2) { | |
| int new_argc = optind + (n - 2); | |
| if (new_argc > original_argc) { | |
| char **new_argv; | |
| int i; | |
| new_argv = (char **)malloc((size_t)new_argc * sizeof(char *)); | |
| if (new_argv == NULL) { | |
| fprintf(stderr, | |
| "ERROR: Out of memory while expanding argument list for %s!\n", | |
| DLT_CONVERT_WS); | |
| if (ovalue) { | |
| close(ohandle); | |
| ohandle = -1; | |
| } | |
| return -1; | |
| } | |
| /* Preserve existing arguments. */ | |
| for (i = 0; i < original_argc; i++) { | |
| new_argv[i] = argv[i]; | |
| } | |
| /* Initialize any newly added slots. They will be populated as needed. */ | |
| for (i = original_argc; i < new_argc; i++) { | |
| new_argv[i] = NULL; | |
| } | |
| argv = new_argv; | |
| argc = new_argc; | |
| } else { | |
| argc = new_argc; | |
| } | |
| } |
Comment on lines
351
to
+405
| if (tflag) { | ||
| /* Prepare the temp dir to untar compressed files */ | ||
| if (mkdir(DLT_CONVERT_WS, 0700) != 0) { | ||
| if (errno != EEXIST) { | ||
| fprintf(stderr,"ERROR: Cannot create temp dir %s!\n", DLT_CONVERT_WS); | ||
| if (ovalue) { | ||
| close(ohandle); | ||
| ohandle = -1; | ||
| } | ||
| return -1; | ||
| } | ||
| /* Directory exists — clear any pre-existing contents (including | ||
| * attacker-planted files) before we use it as a workspace. */ | ||
| if (stat(DLT_CONVERT_WS, &st) == 0 && S_ISDIR(st.st_mode)) | ||
| empty_dir(DLT_CONVERT_WS); | ||
| } | ||
| else { | ||
| if (S_ISDIR(st.st_mode)) | ||
| empty_dir(DLT_CONVERT_WS); | ||
| else | ||
| fprintf(stderr, "ERROR: %s is not a directory", DLT_CONVERT_WS); | ||
| empty_dir(DLT_CONVERT_WS); | ||
| } | ||
|
|
||
| for (index = optind; index < argc; index++) { | ||
| /* Check extension of input file | ||
| * If it is a compressed file, uncompress it | ||
| */ | ||
| const char *file_ext = get_filename_ext(argv[index]); | ||
| if (file_ext && strcmp(file_ext, DLT_EXTENSION) != 0) { | ||
| syserr = dlt_execute_command(NULL, "tar", "xf", argv[index], "-C", DLT_CONVERT_WS, NULL); | ||
| if (syserr != 0) | ||
| fprintf(stderr, "ERROR: Failed to uncompress %s to %s with error [%d]\n", | ||
| argv[index], DLT_CONVERT_WS, WIFEXITED(syserr)); | ||
| } | ||
| else { | ||
| syserr = dlt_execute_command(NULL, "cp", argv[index], DLT_CONVERT_WS, NULL); | ||
| if (syserr != 0) | ||
| fprintf(stderr, "ERROR: Failed to copy %s to %s with error [%d]\n", | ||
| argv[index], DLT_CONVERT_WS, WIFEXITED(syserr)); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| n = scandir(DLT_CONVERT_WS, &files, NULL, alphasort); | ||
| if (n == -1) { | ||
| fprintf(stderr,"ERROR: Cannot scan temp dir %s!\n", DLT_CONVERT_WS); | ||
| if (ovalue) { | ||
| close(ohandle); | ||
| ohandle = -1; | ||
| } | ||
| return -1; | ||
| } | ||
|
|
||
| /* do not include ./ and ../ in the files */ | ||
| argc = optind + (n - 2); | ||
| /* Guard: never exceed the original argv bounds. */ | ||
| if (argc > original_argc) | ||
| argc = original_argc; |
There was a problem hiding this comment.
There are existing integration tests that run dlt-convert, but none appear to cover the -t (tar workspace) path. Adding a regression test for an archive with multiple extracted .dlt files (and for an attacker-controlled pre-existing workspace path) would help prevent reintroducing the overflow/tmp issues.
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
Fixes two bugs in
dlt-convert.creported in issue #793:Bug 1 — Stack overflow via inflated
argcdlt-convertcreates a workspace directory atDLT_CONVERT_WS(/tmp/dlt_convert_workspace/) and builds a dynamicargv[]array from its contents. The countnis read from the directory (attacker-controllable via/tmpworld-write). The computedargc = optind + (n - 2)can exceed the originalargc, causingargv[index] = tmp_filenameto write past the original stack-allocatedargvbounds — CWE-121 (stack-based buffer overflow).Fix: record
original_argcbefore workspace processing; cap the computedargcatoriginal_argc.Bug 2 —
/tmprace:empty_dir()never called on EEXIST pathOn
mkdir(DLT_CONVERT_WS)returningEEXIST, the code callsstat(DLT_CONVERT_WS, &st)butstwas zero-initialized and never populated by the precedingmkdir.S_ISDIR(st.st_mode)always evaluates false on a zeroedst_mode, soempty_dir()is never invoked — stale files accumulate and feed the inflatedargccount. CWE-377 (insecure temporary file).Fix: add explicit
stat()call on the EEXIST path before checkingS_ISDIR; remove the deadS_ISDIRcheck in the fresh-mkdir branch (newly created directory is always a directory).Changes
src/console/dlt-convert.c— 1 file, +9/-4 linesint original_argc = argc;before workspace processingstat(DLT_CONVERT_WS, &st)explicitly, then guardempty_dir()with realS_ISDIRresultempty_dir()unconditionally (directory was just created)argcinflation:if (argc > original_argc) argc = original_argc;Safety framing
Closes #793
Signed-off-by: Akihiko Komada aki1770@gmail.com