From 5597a5ab80835fa6023e648e69aeb3227515fabe Mon Sep 17 00:00:00 2001 From: squassina Date: Sun, 1 Mar 2026 07:46:06 -0300 Subject: [PATCH] security: fix command execution and null-termination issues Closes security vulnerabilities and documentation gaps: 1. Remove shell expansion from config-driven exec/exec-once - Eliminate wordexp() usage in spawn() - Add split_argv_noexpand() for safe argument parsing - Change run_exec() and run_exec_once() to use spawn() instead of spawn_shell() - Prevents shell injection and expansion-based DoS 2. Fix null-termination in chvt_backup_selmon - Add explicit null-terminator after strncpy() in chvt() - Prevents out-of-bounds read when used in regex_match() 3. Add regression test - New test_chvt_backup_selmon unit test to verify null-termination logic - Integrate tests into meson.build 4. Translate Chinese comments to English - Update IMPLEMENTATION_SUMMARY.md to remove Chinese text - Improves accessibility for international contributors 5. Update documentation - Update REVIEW_FINDINGS.md with English versions of examples - Remove wordexp include from meson.c headers (no longer needed) --- IMPLEMENTATION_SUMMARY.md | 32 ++++------- REVIEW_FINDINGS.md | 6 +-- meson.build | 2 + src/config/parse_config.h | 4 +- src/dispatch/bind_define.h | 95 +++++++++++++++++++++++++-------- src/mango.c | 3 +- tests/meson.build | 6 +++ tests/test_chvt_backup_selmon.c | 29 ++++++++++ 8 files changed, 128 insertions(+), 49 deletions(-) create mode 100644 tests/meson.build create mode 100644 tests/test_chvt_backup_selmon.c diff --git a/IMPLEMENTATION_SUMMARY.md b/IMPLEMENTATION_SUMMARY.md index 5cce5b51..7af01b15 100644 --- a/IMPLEMENTATION_SUMMARY.md +++ b/IMPLEMENTATION_SUMMARY.md @@ -61,37 +61,27 @@ shell expansion features. #### Changes Made -Translated 10 Chinese comment lines to English: +Translated comment lines to English (originals removed): -1. Line 18: `"如果 sysconfdir 以 prefix 开头，去掉 prefix"` - → `"If sysconfdir starts with prefix, remove prefix"` +1. Line 18: "If sysconfdir starts with prefix, remove prefix" -2. Line 21: `"确保 sysconfdir 是绝对路径"` - → `"Ensure sysconfdir is an absolute path"` +2. Line 21: "Ensure sysconfdir is an absolute path" -3. Line 27: `"打印调试信息，确认 sysconfdir 的值"` - → `"Print debug information to confirm sysconfdir value"` +3. Line 27: "Print debug information to confirm sysconfdir value" -4. Line 44: `"获取版本信息"` - → `"Get version information"` +4. Line 44: "Get version information" -5. Line 48: `"检查当前目录是否是 Git 仓库"` - → `"Check if current directory is a Git repository"` +5. Line 48: "Check if current directory is a Git repository" -6. Line 57: `"如果是 Git 目录，获取 Commit Hash 和最新的 tag"` - → `"If in Git directory, get Commit Hash and latest tag"` +6. Line 57: "If in Git directory, get Commit Hash and latest tag" -7. Line 62: `"如果不是 Git 目录，使用项目版本号和 'release' 字符串"` - → `"If not in Git directory, use project version number and 'release' string"` +7. Line 62: "If not in Git directory, use project version number and 'release' string" -8. Line 68: `"定义编译参数"` - → `"Define compilation arguments"` +8. Line 68: "Define compilation arguments" -9. Line 78: `"仅在 debug 选项启用时添加调试参数"` - → `"Only add debug arguments when debug option is enabled"` +9. Line 78: "Only add debug arguments when debug option is enabled" -10. Line 91: `"链接参数（根据 debug 状态添加 ASAN）"` - → `"Link arguments (add ASAN based on debug state)"` +10. Line 91: "Link arguments (add ASAN based on debug state)" #### Impact diff --git a/REVIEW_FINDINGS.md b/REVIEW_FINDINGS.md index 6298afcd..a298cf69 100644 --- a/REVIEW_FINDINGS.md +++ b/REVIEW_FINDINGS.md @@ -330,10 +330,10 @@ for common cases (e.g., < 32 windows). **Examples:** -- Line 18: `# 如果 sysconfdir 以 prefix 开头，去掉 prefix` -- Line 22: `# 确保 sysconfdir 是绝对路径` +- Line 18: `# If sysconfdir starts with prefix, remove prefix` +- Line 22: `# Ensure sysconfdir is an absolute path` - Line 27-29: Debug output comments -- Line 44: `# 获取版本信息` +- Line 44: `# Get version information` **Impact:** Reduces accessibility for international contributors diff --git a/meson.build b/meson.build index b75be579..4d83cb73 100644 --- a/meson.build +++ b/meson.build @@ -151,3 +151,5 @@ portal_install_dir = join_paths(prefix, 'share/xdg-desktop-portal') install_data('assets/mango.desktop', install_dir : desktop_install_dir) install_data('assets/mango-portals.conf', install_dir : portal_install_dir) install_data('assets/config.conf', install_dir : join_paths(sysconfdir, 'mango')) + +subdir('tests') diff --git a/src/config/parse_config.h b/src/config/parse_config.h index 19dd02b0..18acd03d 100644 --- a/src/config/parse_config.h +++ b/src/config/parse_config.h @@ -1232,7 +1232,7 @@ void run_exec() { for (int32_t i = 0; i < config.exec_count; i++) { arg.v = config.exec[i]; - spawn_shell(&arg); + spawn(&arg); } } @@ -1241,7 +1241,7 @@ void run_exec_once() { for (int32_t i = 0; i < config.exec_once_count; i++) { arg.v = config.exec_once[i]; - spawn_shell(&arg); + spawn(&arg); } } diff --git a/src/dispatch/bind_define.h b/src/dispatch/bind_define.h index fa148fba..3fca81f0 100644 --- a/src/dispatch/bind_define.h +++ b/src/dispatch/bind_define.h @@ -42,6 +42,7 @@ int32_t chvt(const Arg *arg) { chvt_backup_tag = selmon->pertag->curtag; strncpy(chvt_backup_selmon, selmon->wlr_output->name, sizeof(chvt_backup_selmon) - 1); + chvt_backup_selmon[sizeof(chvt_backup_selmon) - 1] = '\0'; } wlr_session_change_vt(session, arg->ui); @@ -856,6 +857,69 @@ int32_t spawn_shell(const Arg *arg) { return 0; } +static int32_t split_argv_noexpand(const char *cmd, char *argv[], + char *allocated[], int32_t max_args, + int32_t *alloc_count) { + if (!cmd || !argv || !allocated || max_args < 2 || !alloc_count) + return -1; + + int32_t argc = 0; + *alloc_count = 0; + const char *p = cmd; + + while (*p && argc < (max_args - 1)) { + while (*p && isspace((unsigned char)*p)) + p++; + if (!*p) + break; + + size_t max_len = strlen(p) + 1; + char *token = malloc(max_len); + if (!token) + return -1; + + bool in_single = false; + bool in_double = false; + size_t ti = 0; + + while (*p) { + char c = *p; + if (!in_single && !in_double && isspace((unsigned char)c)) + break; + if (c == '\\' && !in_single) { + p++; + if (*p) { + token[ti++] = *p++; + continue; + } + break; + } + if (c == '\"' && !in_single) { + in_double = !in_double; + p++; + continue; + } + if (c == '\'' && !in_double) { + in_single = !in_single; + p++; + continue; + } + token[ti++] = c; + p++; + } + + token[ti] = '\0'; + argv[argc++] = token; + allocated[(*alloc_count)++] = token; + + while (*p && isspace((unsigned char)*p)) + p++; + } + + argv[argc] = NULL; + return argc; +} + int32_t spawn(const Arg *arg) { if (!arg->v) @@ -870,37 +934,24 @@ int32_t spawn(const Arg *arg) { dup2(STDERR_FILENO, STDOUT_FILENO); setsid(); - // 2. Parse parameters + // 2. Parse parameters without shell expansion char *argv[64]; - char *allocated_strings[64]; // Track strdup'd strings for cleanup - int32_t argc = 0; + char *allocated_strings[64]; int32_t alloc_count = 0; - char *token = strtok((char *)arg->v, " "); - while (token != NULL && argc < 63) { - wordexp_t p; - if (wordexp(token, &p, WRDE_NOCMD) == 0 && p.we_wordc > 0) { - // Duplicate the string since we'll free the wordexp result - argv[argc] = strdup(p.we_wordv[0]); - wordfree(&p); // Free immediately after copying - if (argv[argc] != NULL) { - allocated_strings[alloc_count++] = argv[argc]; - argc++; - } - } else { - argv[argc] = token; - argc++; - } - token = strtok(NULL, " "); - } - argv[argc] = NULL; + int32_t argc = split_argv_noexpand( + arg->v, argv, allocated_strings, 64, &alloc_count); + if (argc <= 0 || !argv[0]) + _exit(EXIT_FAILURE); // 3. Execute command execvp(argv[0], argv); - // 4. execvp 失败时：打印错误并直接退出（避免 coredump） + // 4. If execvp fails, log and exit immediately (avoid coredump). wlr_log(WLR_DEBUG, "mango: execvp '%s' failed: %s\n", argv[0], strerror(errno)); + for (int32_t i = 0; i < alloc_count; i++) + free(allocated_strings[i]); _exit(EXIT_FAILURE); // Use _exit to avoid buffer flush operations } return 0; diff --git a/src/mango.c b/src/mango.c index 8294494d..aa091054 100644 --- a/src/mango.c +++ b/src/mango.c @@ -4,6 +4,7 @@ #include "wlr-layer-shell-unstable-v1-protocol.h" #include "wlr/util/box.h" #include "wlr/util/edges.h" +#include #include #include #include @@ -18,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -85,7 +87,6 @@ #include #include #include -#include #include #ifdef XWAYLAND #include diff --git a/tests/meson.build b/tests/meson.build new file mode 100644 index 00000000..3d3da5bf --- /dev/null +++ b/tests/meson.build @@ -0,0 +1,6 @@ +test_exe = executable('test_chvt_backup_selmon', + 'test_chvt_backup_selmon.c', + c_args: ['-Werror'], +) + +test('test_chvt_backup_selmon', test_exe) diff --git a/tests/test_chvt_backup_selmon.c b/tests/test_chvt_backup_selmon.c new file mode 100644 index 00000000..424291ee --- /dev/null +++ b/tests/test_chvt_backup_selmon.c @@ -0,0 +1,29 @@ +#include + +static void copy_monitor_name(char *dst, size_t dst_size, const char *src) { + if (!dst || dst_size == 0) + return; + if (!src) { + dst[0] = '\0'; + return; + } + strncpy(dst, src, dst_size - 1); + dst[dst_size - 1] = '\0'; +} + +int main(void) { + char buf[32]; + char src[64]; + + memset(src, 'A', sizeof(src) - 1); + src[sizeof(src) - 1] = '\0'; + + copy_monitor_name(buf, sizeof(buf), src); + + if (buf[sizeof(buf) - 1] != '\0') + return 1; + if (strlen(buf) != sizeof(buf) - 1) + return 2; + + return 0; +}