Skip to content

fix: prevent stack overflow and /tmp race in dlt-convert workspace (#793) #833

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aki1770-del wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: prevent stack overflow and /tmp race in dlt-convert workspace (#793) #833

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions src/console/dlt-convert.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ int main(int argc, char *argv[])
memset(&st, 0, sizeof(struct stat));
struct dirent **files = { 0 };
int n = 0;
int original_argc = argc;

struct iovec iov[2];
int bytes_written = 0;
Expand Down Expand Up @@ -358,12 +359,13 @@ int main(int argc, char *argv[])
}
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);
Comment on lines +362 to +365
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copilot uses AI. Check for mistakes.
Comment on lines +362 to +365
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copilot uses AI. Check for mistakes.
}
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++) {
Expand Down Expand Up @@ -398,6 +400,9 @@ int main(int argc, char *argv[])

/* 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;
Comment on lines 401 to +405
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}
}

Copilot uses AI. Check for mistakes.
}

for (index = optind; index < argc; index++) {
Expand Down
Loading