From 53309ea2d748d6a4c938ab1793a386ba5c701c18 Mon Sep 17 00:00:00 2001 From: Xavier Delaruelle Date: Thu, 13 Aug 2026 07:12:57 +0000 Subject: [PATCH 1/3] init: fix command injection in bash Tab completion (compgen -W expansion) _module_comgen_words_and_files() fed untrusted text (module names read off disk, LOADEDMODULES, MODULEPATH) straight into `compgen -W`, which performs a full unquoted-word expansion on its wordlist -- including command substitution -- as a normal, documented part of its behavior. A module name, loaded-module entry, or MODULEPATH component containing e.g. `$(...)` therefore ran arbitrary shell code the moment a user pressed Tab. Fix: never hand candidate text to `compgen -W`. The new _module_comgen_words() splits the candidate list with `read -r -d ''` (pure IFS word-splitting, no expansion of any kind) and does the prefix match itself; _module_comgen_words_and_files() now layers the nospace-for-directory-entries behavior on top of it. Every call site that previously built a compgen -W wordlist from LOADEDMODULES, MODULEPATH, or a stash/save collection name now goes through one of these two functions instead. Fixes CVE-2026-85013 Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Xavier Delaruelle --- .hunspell.en.dic | 3 ++ NEWS.rst | 6 ++++ init/bash_completion.in | 63 +++++++++++++++++++++++++---------------- 3 files changed, 47 insertions(+), 25 deletions(-) diff --git a/.hunspell.en.dic b/.hunspell.en.dic index 7482eb4aa..de08ab5ac 100644 --- a/.hunspell.en.dic +++ b/.hunspell.en.dic @@ -344,6 +344,7 @@ compA compB compat compdef +compgen completionhome compilerTag compopt @@ -922,6 +923,7 @@ unloadable unprefixed unsetConf unsetModuleDependency +untrusted unsetState unsetenv unsets @@ -1617,3 +1619,4 @@ cuda lindex defineModStartNbProc isIcase +CVE diff --git a/NEWS.rst b/NEWS.rst index b30b07019..a3c848ae9 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -251,6 +251,12 @@ Modules 5.7.0 (not yet released) * Doc: add the :ref:`user-guide` document that explains a selection of useful but lesser known features through practical examples and common use cases. +* Init: fix command injection in Bash completion when module names contain + shell meta-characters. Completion candidates were passed to ``compgen -W`` + which evaluates command substitution syntax. (fix `CVE-2026-85013`_ found + by AISLE in partnership with Red Hat) + +.. _CVE-2026-85013: https://github.com/envmodules/modules/security/advisories/GHSA-8hrw-p88g-qhmg .. _5.6 release notes: diff --git a/init/bash_completion.in b/init/bash_completion.in index 160aed0ad..138c0e2d1 100644 --- a/init/bash_completion.in +++ b/init/bash_completion.in @@ -2,17 +2,30 @@ # # Bash commandline completion # +_module_comgen_words() { + local -a words + local val + # split candidate list on IFS without triggering shell expansion (command + # substitution, arithmetic, ...): candidate words may come from module + # names read off disk, which must never be treated as executable code + IFS=$' \t\n' read -r -d '' -a words <<<"$1" + for val in "${words[@]}"; do + case "$val" in + "$2"*) COMPREPLY[${#COMPREPLY[@]}]="$val" ;; + esac + done +} + _module_comgen_words_and_files() { - local k=0 - local setnospace=1 + local start=${#COMPREPLY[@]} i + _module_comgen_words "$1" "$2" # do not append space to word completed if it is a directory (ends with /) - for val in $(compgen -W "$1" -- "$2"); do - if [ $setnospace -eq 1 ] && [ "${val: -1:1}" = '/' ]; then + for ((i = start; i < ${#COMPREPLY[@]}; i++)); do + if [ "${COMPREPLY[i]: -1:1}" = '/' ]; then # Bash >=4.0 is required for compopt type compopt &>/dev/null && compopt -o nospace - setnospace=0 + break fi - COMPREPLY[k++]="$val" done } @@ -64,7 +77,7 @@ _module_long_arg_list() { _module_comgen_words_and_files "$(_module_not_yet_loaded "$cur")" "$cur" break;; rm|delete|remove|unload|switch|swap) - COMPREPLY=( $(IFS=: compgen -W "${LOADEDMODULES}" -- "$cur") ) + _module_comgen_words "${LOADEDMODULES//:/ }" "$cur" break;; esac done @@ -85,19 +98,19 @@ _module() { list) COMPREPLY=( $(compgen -W "@comp_list_opts@" -- "$cur") );; savelist) COMPREPLY=( $(compgen -W "@comp_savelist_opts@" -- "$cur") );; stashlist) COMPREPLY=( $(compgen -W "@comp_stashlist_opts@" -- "$cur") );; - stashpop) COMPREPLY=( $(compgen -W "@comp_stashpop_opts@ $(_module_stashlist)" -- "$cur") );; + stashpop) _module_comgen_words "@comp_stashpop_opts@ $(_module_stashlist)" "$cur";; stashshow|stashrm) - COMPREPLY=( $(compgen -W "$(_module_stashlist)" -- "$cur") );; + _module_comgen_words "$(_module_stashlist)" "$cur";; clear) COMPREPLY=( $(compgen -W "@comp_clear_opts@" -- "$cur") );; - restore) COMPREPLY=( $(compgen -W "@comp_restore_opts@ $(_module_savelist)" -- "$cur") );; + restore) _module_comgen_words "@comp_restore_opts@ $(_module_savelist)" "$cur";; save|saveshow|describe|saverm|disable|is-saved) - COMPREPLY=( $(compgen -W "$(_module_savelist)" -- "$cur") );; + _module_comgen_words "$(_module_savelist)" "$cur";; rm|delete|remove|unload) - COMPREPLY=( $(compgen -W "@comp_unload_opts@ ${LOADEDMODULES//:/ }" -- "$cur") );; - switch|swap) COMPREPLY=( $(compgen -W "@comp_load_opts@ ${LOADEDMODULES//:/ }" -- "$cur") );; - unuse|is-used) COMPREPLY=( $(IFS=: compgen -W "${MODULEPATH}" -- "$cur") );; + _module_comgen_words "@comp_unload_opts@ ${LOADEDMODULES//:/ }" "$cur";; + switch|swap) _module_comgen_words "@comp_load_opts@ ${LOADEDMODULES//:/ }" "$cur";; + unuse|is-used) _module_comgen_words "${MODULEPATH//:/ }" "$cur";; use) case "$cur" in - -*) COMPREPLY=( $(compgen -W "@comp_use_opts@" -- "$cur") );; + -*) _module_comgen_words "@comp_use_opts@" "$cur";; *) ;; # let readline handle the completion esac;; -a|--append|cachebuild) ;; # let readline handle the completion @@ -161,19 +174,19 @@ if type -t ml >/dev/null; then list) COMPREPLY=( $(compgen -W "@comp_list_opts@" -- "$cur") );; savelist) COMPREPLY=( $(compgen -W "@comp_savelist_opts@" -- "$cur") );; stashlist) COMPREPLY=( $(compgen -W "@comp_stashlist_opts@" -- "$cur") );; - stashpop) COMPREPLY=( $(compgen -W "@comp_stashpop_opts@ $(_module_stashlist)" -- "$cur") );; + stashpop) _module_comgen_words "@comp_stashpop_opts@ $(_module_stashlist)" "$cur";; stashshow|stashrm) - COMPREPLY=( $(compgen -W "$(_module_stashlist)" -- "$cur") );; + _module_comgen_words "$(_module_stashlist)" "$cur";; clear) COMPREPLY=( $(compgen -W "@comp_clear_opts@" -- "$cur") );; - restore) COMPREPLY=( $(compgen -W "@comp_restore_opts@ $(_module_savelist)" -- "$cur") );; + restore) _module_comgen_words "@comp_restore_opts@ $(_module_savelist)" "$cur";; save|saveshow|describe|saverm|disable|is-saved) - COMPREPLY=( $(compgen -W "$(_module_savelist)" -- "$cur") );; + _module_comgen_words "$(_module_savelist)" "$cur";; rm|delete|remove|unload) - COMPREPLY=( $(compgen -W "@comp_unload_opts@ ${LOADEDMODULES//:/ }" -- "$cur") );; - switch|swap) COMPREPLY=( $(compgen -W "@comp_load_opts@ ${LOADEDMODULES//:/ }" -- "$cur") );; - unuse|is-used) COMPREPLY=( $(IFS=: compgen -W "${MODULEPATH}" -- "$cur") );; + _module_comgen_words "@comp_unload_opts@ ${LOADEDMODULES//:/ }" "$cur";; + switch|swap) _module_comgen_words "@comp_load_opts@ ${LOADEDMODULES//:/ }" "$cur";; + unuse|is-used) _module_comgen_words "${MODULEPATH//:/ }" "$cur";; use) case "$cur" in - -*) COMPREPLY=( $(compgen -W "@comp_use_opts@" -- "$cur") );; + -*) _module_comgen_words "@comp_use_opts@" "$cur";; *) ;; # let readline handle the completion esac;; -a|--append|cachebuild) ;; # let readline handle the completion @@ -218,14 +231,14 @@ if type -t ml >/dev/null; then for i in ${LOADEDMODULES//:/ }; do loaded_modules+="-${i} " done - COMPREPLY=( "${COMPREPLY[@]}" $(compgen -W "@comp_load_opts@ $loaded_modules" -- "$cur") );; + _module_comgen_words "@comp_load_opts@ $loaded_modules" "$cur";; *) _module_comgen_words_and_files "@comp_load_opts@ $(_module_not_yet_loaded "$cur")" "$cur" COMPREPLY=( "${COMPREPLY[@]}" $(compgen -W "@comp_opts@ @comp_cmds@" -- "$cur") ) loaded_modules="" for i in ${LOADEDMODULES//:/ }; do loaded_modules+="-${i} " done - COMPREPLY=( "${COMPREPLY[@]}" $(compgen -W "@comp_load_opts@ $loaded_modules" -- "$cur") );; + _module_comgen_words "@comp_load_opts@ $loaded_modules" "$cur";; esac fi;; esac From bd44f57ff689b50931017bec31286882fb7b426b Mon Sep 17 00:00:00 2001 From: Xavier Delaruelle Date: Thu, 13 Aug 2026 07:13:06 +0000 Subject: [PATCH 2/3] ts: add bash non-regression cases for the completion injection fix Adds injection-safety cases to the completion DejaGnu tool (testsuite/completion.00-init/021-bash.exp) covering the fix in the previous commit: a crafted module name, LOADEDMODULES entry, and MODULEPATH entry embedding shell code must be listed as an inert candidate string and never executed, verified by checking a marker file the payload would touch if it ran (completion_assert_no_exec, new in 006-procs.exp). Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Xavier Delaruelle --- .gitignore | 2 + doc/source/devel/testsuite.rst | 12 +-- testsuite/completion.00-init/005-init_ts.exp | 4 + testsuite/completion.00-init/006-procs.exp | 16 ++++ testsuite/completion.00-init/021-bash.exp | 78 +++++++++++++++++++- 5 files changed, 105 insertions(+), 7 deletions(-) diff --git a/.gitignore b/.gitignore index 11d0642c2..6260fc02f 100644 --- a/.gitignore +++ b/.gitignore @@ -61,6 +61,8 @@ /testsuite/completion-use-fixture /testsuite/completion-home /testsuite/cookbook-sandbox +/testsuite/completion-inject-fixture +/testsuite/completion-injection-marker /doc/build /doc/source/version.py /doc/demo/*/*.svg diff --git a/doc/source/devel/testsuite.rst b/doc/source/devel/testsuite.rst index 30181e63e..a9805bbf6 100644 --- a/doc/source/devel/testsuite.rst +++ b/doc/source/devel/testsuite.rst @@ -46,8 +46,10 @@ DejaGnu tools: Drives a real, interactive shell process (via Expect ``spawn``/``send``/ ``expect``, not just a captured non-interactive run like the other three tools) to press Tab against the built shell completion script and check - that the resulting candidate list holds the expected module names and - option flags. Driven by the :file:`completion.00-init` directory; + the resulting candidate list -- both that expected module names/option + flags show up, and that a candidate word built from untrusted text (a + module name, ``LOADEDMODULES``, ``MODULEPATH``, ...) can never reach a + shell expansion step. Driven by the :file:`completion.00-init` directory; currently covers bash, zsh, fish and tcsh, see `completion.00-init layout`_. @@ -181,9 +183,9 @@ shell-specific in a way none of the other three tools are: :file:`init/zsh-functions/_module`, :file:`init/fish_completion` and :file:`init/tcsh_completion` scripts, a clean fixture modulepath), the shell-agnostic assert procedures (``completion_assert_contains``, - ``completion_assert_not_contains``, ``completion_assert_eq``), and the - clean baseline environment/``save_test_env`` checkpoint, exactly as for - the other tools. + ``completion_assert_not_contains``, ``completion_assert_eq``, + ``completion_assert_no_exec``), and the clean baseline + environment/``save_test_env`` checkpoint, exactly as for the other tools. - ``0NN--procs.exp`` defines one ``completion__start`` / ``completion__raw`` / ``completion__list`` / ``completion__inline`` / ``completion__close`` set per diff --git a/testsuite/completion.00-init/005-init_ts.exp b/testsuite/completion.00-init/005-init_ts.exp index 461b88f56..877ef6a31 100644 --- a/testsuite/completion.00-init/005-init_ts.exp +++ b/testsuite/completion.00-init/005-init_ts.exp @@ -121,4 +121,8 @@ set out [open $tcshcompletion w] puts -nonewline $out $tcshcompletion_content close $out +# scratch area for modulefiles created on the fly by completion tests +set injectdir "$env(TESTSUITEDIR)/completion-inject-fixture" +file delete -force $injectdir + # vim:set tabstop=3 shiftwidth=3 expandtab autoindent: diff --git a/testsuite/completion.00-init/006-procs.exp b/testsuite/completion.00-init/006-procs.exp index 7a499e422..e00ca07b1 100644 --- a/testsuite/completion.00-init/006-procs.exp +++ b/testsuite/completion.00-init/006-procs.exp @@ -130,4 +130,20 @@ proc completion_assert_eq {got expected} { } } +# assert a marker file was not created (used to prove no shell code +# injected through a candidate word got executed); the test label is built +# from the cmdline last passed to completion__list plus the marker +# path +proc completion_assert_no_exec {markerpath} { + global completion_last_cmdline + + set label "'$completion_last_cmdline' completion does not execute injected code ($markerpath)" + if {[file exists $markerpath]} { + fail "$label (marker file was created: injected code ran)" + file delete -force $markerpath + } else { + pass $label + } +} + # vim:set tabstop=3 shiftwidth=3 expandtab autoindent: diff --git a/testsuite/completion.00-init/021-bash.exp b/testsuite/completion.00-init/021-bash.exp index db285fa47..f5378e3aa 100644 --- a/testsuite/completion.00-init/021-bash.exp +++ b/testsuite/completion.00-init/021-bash.exp @@ -10,7 +10,7 @@ # Authors: Xavier Delaruelle, xavier.delaruelle@cea.fr # # Description: Testuite testsequence -# Command: avail, load, unload, ml, use, unuse, restore +# Command: avail, load, unload, ml, use, unuse, restore, switch # Sub-Command: # # Comment: %C{ @@ -26,7 +26,12 @@ # 'ml' Tab-completion proposes is checked against the # ground truth fetched from 'module help' (see # 007-module_help.exp) and 'module config' (see -# 008-module_config.exp) -- neither less nor more +# 008-module_config.exp) -- neither less nor more, and +# completion candidate words built from untrusted text +# (module names read off disk, LOADEDMODULES, +# MODULEPATH) cannot reach a shell expansion step -- a +# candidate embedding shell code must be listed as an +# inert string, never executed # }C% # ############################################################################## @@ -480,6 +485,75 @@ completion_bash_close unsetenv_loaded_module +# +# candidate words built from untrusted text cannot reach a shell expansion +# step (see security fix for the bash-completion command injection) +# + +set marker "$env(TESTSUITEDIR)/completion-injection-marker" +file delete -force $marker + +# module/spider avail: malicious module name read off disk + +# the marker path is passed through as an env var rather than embedded +# directly in the filename, since a filename cannot itself contain '/' +setenv_var INJMARKER $marker +set evilname {evil$(touch>$INJMARKER)} +file mkdir $injectdir +set fd [open "$injectdir/$evilname" w] +puts $fd {#%Module1.0} +close $fd + +setenv_path_var MODULEPATH $injectdir + +completion_bash_start + +set got [completion_bash_list {module load }] +completion_assert_no_exec $marker +completion_assert_contains $got [list $evilname] + +completion_bash_close +unsetenv_path_var MODULEPATH +unsetenv_var INJMARKER +file delete -force $injectdir + +# LOADEDMODULES: malicious already-loaded module name + +set evilmod [string map [list MARKERPATH $marker] {python$(touch>MARKERPATH)}] +# _LMFILES_ is set consistently alongside LOADEDMODULES (matching entry +# count) purely to avoid an unrelated "inconsistent state" warning; its +# content is not used by the completion script +setenv_loaded_module [list gcc $evilmod] [list /fake/gcc /fake/evilmod] + +completion_bash_start + +set got [completion_bash_list {module unload }] +completion_assert_no_exec $marker +completion_assert_contains $got [list $evilmod] + +completion_bash_close +unsetenv_loaded_module + +# MODULEPATH: malicious path entry (module unuse) + +set evilpath [string map [list MARKERPATH $marker] {/opt$(touch>MARKERPATH)}] +setenv_path_var MODULEPATH /tmp $evilpath + +completion_bash_start + +## both entries share a leading '/', so type it explicitly: otherwise +## readline auto-inserts that common prefix on the first Tab and the +## double-Tab below would only ring the bell instead of listing +set got [completion_bash_list {module unuse /}] +completion_assert_no_exec $marker +completion_assert_contains $got [list $evilpath] + +completion_bash_close +unsetenv_path_var MODULEPATH + +file delete -force $marker + + # # Cleanup # From 02ec3317fc9ec5617cf3d8ab03f0ca110efed3ac Mon Sep 17 00:00:00 2001 From: Xavier Delaruelle Date: Thu, 13 Aug 2026 07:13:37 +0000 Subject: [PATCH 3/3] ts: add zsh/fish/tcsh non-regression cases for the completion injection fix The bash fix in a previous commit only applied to bash: the unquoted- word expansion compgen -W performs on its candidate string is specific to that one builtin. init/zsh-functions/_module.in hands candidates to 'compadd -a ', init/fish_completion to 'complete -a "(...)"' (newline-split command output), and init/tcsh_completion.in to a plain backtick command's word list -- in all three, each element becomes a literal candidate string with no further shell expansion, so none of them were ever vulnerable to this bug class. Add the same three injection-safety cases (a malicious module name, LOADEDMODULES entry, and MODULEPATH entry, each embedding shell code) to 031-zsh.exp/041-fish.exp/051-tcsh.exp, to guard against a future regression rather than a known vulnerability. Each shell has its own candidate-display conventions, which change what the literal "contains" check needs to look for: zsh backslash-escapes special characters before display/insertion, fish single-quotes a candidate containing a special character on single-candidate inline completion and (on a multi-candidate pager listing) strips a "(...)" suffix as if it were a candidate's own description, colliding with the embedded parentheses in the crafted candidate itself. fish has no MODULEPATH case: 'unuse' does not list modulepaths there at all. None of this affects the actual security check (completion_assert_no_exec, confirming that the crafted candidate's 'touch' side effect never ran), which needs no such per-shell handling. Also widens the completion timeout for the new section in 051-tcsh.exp: each case there spawns a cold tcsh session with nothing having already warmed up the pty/subprocess pipeline before listing candidates, which occasionally ran past the default 10 second bound under a loaded machine. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Xavier Delaruelle --- .hunspell.en.dic | 4 + testsuite/completion.00-init/031-zsh.exp | 82 +++++++++++++++++++++ testsuite/completion.00-init/041-fish.exp | 74 +++++++++++++++++++ testsuite/completion.00-init/051-tcsh.exp | 89 +++++++++++++++++++++++ 4 files changed, 249 insertions(+) diff --git a/.hunspell.en.dic b/.hunspell.en.dic index de08ab5ac..0c0f60a4e 100644 --- a/.hunspell.en.dic +++ b/.hunspell.en.dic @@ -263,6 +263,7 @@ autotools availabilities avx ba +backtick backticks badcommand baf @@ -285,6 +286,7 @@ boolvariantname boolvr bourne bugfix +builtin cachebuild cacheclear cachefile @@ -341,6 +343,7 @@ comgen commandexp commandname compA +compadd compB compat compdef @@ -842,6 +845,7 @@ subdir subdirectories subdirectory submodule +subprocess subprojects subshell substring diff --git a/testsuite/completion.00-init/031-zsh.exp b/testsuite/completion.00-init/031-zsh.exp index 63f1ed05b..1ba9a4764 100644 --- a/testsuite/completion.00-init/031-zsh.exp +++ b/testsuite/completion.00-init/031-zsh.exp @@ -38,6 +38,15 @@ # @comp_*_opts@ flags into the candidate set # regardless of prefix, so a bare trailing-space # listing here only ever shows one set at a time +# +# Also checks, like 021-bash.exp, that a candidate word +# built from untrusted text (a module name, +# LOADEDMODULES, MODULEPATH) cannot reach a shell +# expansion step: init/zsh-functions/_module.in only ever +# hands such text to 'compadd -a ', where each +# array element is used as a literal candidate string, so +# this is expected to already be safe -- these cases guard +# against a regression, not a known vulnerability # }C% # ############################################################################## @@ -393,6 +402,79 @@ completion_zsh_close unsetenv_loaded_module +# +# candidate words built from untrusted text cannot reach a shell expansion +# step (see this file's header, and the security fix for the bash-completion +# command injection this guards against a regression of) +# + +set marker "$env(TESTSUITEDIR)/completion-injection-marker" +file delete -force $marker + +# module avail: malicious module name read off disk + +# the marker path is passed through as an env var rather than embedded +# directly in the filename, since a filename cannot itself contain '/' +setenv_var INJMARKER $marker +set evilname {evil$(touch>$INJMARKER)} +file mkdir $injectdir +set fd [open "$injectdir/$evilname" w] +puts $fd {#%Module1.0} +close $fd + +setenv_path_var MODULEPATH $injectdir + +completion_zsh_start + +## _module_avail_mods hands this candidate to 'compadd', which backslash- +## escapes every shell metacharacter in it before display/insertion (the +## same protection this file's header notes _module.in relies on instead of +## bash's compgen -W): the listing shows 'evil\$\(touch\>\$INJMARKER\)', not +## the raw string +set got [completion_zsh_list {module load }] +completion_assert_no_exec $marker +completion_assert_contains $got [list [regsub -all {[$()>]} $evilname {\\&}]] + +completion_zsh_close +unsetenv_path_var MODULEPATH +unsetenv_var INJMARKER +file delete -force $injectdir + +# LOADEDMODULES: malicious already-loaded module name + +set evilmod [string map [list MARKERPATH $marker] {python$(touch>MARKERPATH)}] +setenv_loaded_module [list gcc $evilmod] [list /fake/gcc /fake/evilmod] + +completion_zsh_start + +set got [completion_zsh_list {module unload }] +completion_assert_no_exec $marker +completion_assert_contains $got [list $evilmod] + +completion_zsh_close +unsetenv_loaded_module + +# MODULEPATH: malicious path entry (module unuse) + +set evilpath [string map [list MARKERPATH $marker] {/opt$(touch>MARKERPATH)}] +setenv_path_var MODULEPATH /tmp $evilpath + +completion_zsh_start + +## both entries share a leading '/', so type it explicitly: otherwise zle +## auto-inserts that common prefix on the first Tab and the double-Tab below +## would only ring the bell instead of listing (same trap as the "bar"/"ba" +## cases earlier in this file) +set got [completion_zsh_list {module unuse /}] +completion_assert_no_exec $marker +completion_assert_contains $got [list $evilpath] + +completion_zsh_close +unsetenv_path_var MODULEPATH + +file delete -force $marker + + # # Cleanup # diff --git a/testsuite/completion.00-init/041-fish.exp b/testsuite/completion.00-init/041-fish.exp index a131f4fc8..54f4178d2 100644 --- a/testsuite/completion.00-init/041-fish.exp +++ b/testsuite/completion.00-init/041-fish.exp @@ -66,6 +66,17 @@ # the word being completed starts with '-' -- so a bare # trailing-space listing only ever shows one set, same # as 031-zsh.exp +# +# Also checks, like 021-bash.exp/031-zsh.exp, that a +# candidate word built from untrusted text (a module +# name, LOADEDMODULES) cannot reach a shell expansion +# step: init/fish_completion only ever hands such text to +# 'complete -a "(...)"', where the parenthesized command's +# output is split into literal candidate lines, so this is +# expected to already be safe -- these cases guard against +# a regression, not a known vulnerability. No MODULEPATH +# case here: 'unuse' does not list modulepaths at all in +# fish (see this file's header) # }C% # ############################################################################## @@ -371,6 +382,69 @@ completion_fish_close unsetenv_loaded_module +# +# candidate words built from untrusted text cannot reach a shell expansion +# step (see this file's header, and the security fix for the bash-completion +# command injection this guards against a regression of) +# + +set marker "$env(TESTSUITEDIR)/completion-injection-marker" +file delete -force $marker + +# module avail: malicious module name read off disk + +# the marker path is passed through as an env var rather than embedded +# directly in the filename, since a filename cannot itself contain '/' +setenv_var INJMARKER $marker +set evilname {evil$(touch>$INJMARKER)} +file mkdir $injectdir +set fd [open "$injectdir/$evilname" w] +puts $fd {#%Module1.0} +close $fd + +setenv_path_var MODULEPATH $injectdir + +completion_fish_start + +## this fixture has only one module, so a single Tab completes it inline +## rather than showing a pager listing (see completion_fish_list's own +## single-candidate fallback) -- and fish, like zsh, protects a candidate +## containing shell metacharacters before inserting it: fish 4 wraps it in +## single quotes whereas fish 3 backslash-escapes each metacharacter +set got [completion_fish_list {module load }] +completion_assert_no_exec $marker +completion_assert_any $got [list "'$evilname'"\ + [regsub -all {[$()>]} $evilname {\\&}]] + +completion_fish_close +unsetenv_path_var MODULEPATH +unsetenv_var INJMARKER +file delete -force $injectdir + +# LOADEDMODULES: malicious already-loaded module name + +set evilmod [string map [list MARKERPATH $marker] {python$(touch>MARKERPATH)}] +setenv_loaded_module [list gcc $evilmod] [list /fake/gcc /fake/evilmod] + +completion_fish_start + +## two candidates here (gcc and evilmod) means a real pager listing, which +## completion_fish_list runs through completion_fish_strip_descriptions to +## drop each candidate's "(description)" suffix -- the injected +## '(touch>...)' substring is indistinguishable from that convention and +## gets stripped the same way, so the surviving candidate is the text up to +## its own first '(', not the full raw string (still proven never executed +## by completion_assert_no_exec above) +set got [completion_fish_list {module unload }] +completion_assert_no_exec $marker +completion_assert_contains $got [list [completion_fish_strip_descriptions $evilmod]] + +completion_fish_close +unsetenv_loaded_module + +file delete -force $marker + + # # Cleanup # diff --git a/testsuite/completion.00-init/051-tcsh.exp b/testsuite/completion.00-init/051-tcsh.exp index b36fc3d25..da1f484e5 100644 --- a/testsuite/completion.00-init/051-tcsh.exp +++ b/testsuite/completion.00-init/051-tcsh.exp @@ -78,6 +78,18 @@ # worked around here, since 'help' otherwise dispatches # exactly like 'show'/'display'/'test'/... which all # already carry that macro) +# +# Also checks, like 021-bash.exp/031-zsh.exp, that a +# candidate word built from untrusted text (a module +# name, LOADEDMODULES, MODULEPATH) cannot reach a shell +# expansion step: init/tcsh_completion.in only ever hands +# such text to a plain 'n/.../`...`/' backtick rule, where +# each output line becomes a literal candidate string, so +# this is expected to already be safe -- these cases guard +# against a regression, not a known vulnerability. The +# MODULEPATH case uses a bare 'module unuse ' (no typed +# prefix), same restriction as this file's own 'unuse' +# section above # }C% # ############################################################################## @@ -455,6 +467,83 @@ completion_tcsh_close unsetenv_loaded_module +# +# candidate words built from untrusted text cannot reach a shell expansion +# step (see this file's header, and the security fix for the bash-completion +# command injection this guards against a regression of) +# + +# each case below spawns a fresh tcsh session and, unlike every other +# section in this file, its Ctrl-D listing goes through _module_loaded/ +# _module_avail/_module_modulepath cold (no completion call already primed +# the pty/subprocess pipeline): comfortably inside the default 10s +# 'timeout' most of the time, but occasionally not under a loaded machine, +# so it is raised for just this section rather than risking a false +# UNRESOLVED +set old_timeout $timeout +set timeout 30 + +set marker "$env(TESTSUITEDIR)/completion-injection-marker" +file delete -force $marker + +# module avail: malicious module name read off disk + +# the marker path is passed through as an env var rather than embedded +# directly in the filename, since a filename cannot itself contain '/' +setenv_var INJMARKER $marker +set evilname {evil$(touch>$INJMARKER)} +file mkdir $injectdir +set fd [open "$injectdir/$evilname" w] +puts $fd {#%Module1.0} +close $fd + +setenv_path_var MODULEPATH $injectdir + +completion_tcsh_start + +set got [completion_tcsh_list {module load }] +completion_assert_no_exec $marker +completion_assert_contains $got [list $evilname] + +completion_tcsh_close +unsetenv_path_var MODULEPATH +unsetenv_var INJMARKER +file delete -force $injectdir + +# LOADEDMODULES: malicious already-loaded module name + +set evilmod [string map [list MARKERPATH $marker] {python$(touch>MARKERPATH)}] +setenv_loaded_module [list gcc $evilmod] [list /fake/gcc /fake/evilmod] + +completion_tcsh_start + +set got [completion_tcsh_list {module unload }] +completion_assert_no_exec $marker +completion_assert_contains $got [list $evilmod] + +completion_tcsh_close +unsetenv_loaded_module + +# MODULEPATH: malicious path entry (module unuse); like this file's own +# 'unuse' section above, only a bare, empty word reaches the modulepath +# listing at all (see this file's header) + +set evilpath [string map [list MARKERPATH $marker] {/opt$(touch>MARKERPATH)}] +setenv_path_var MODULEPATH /tmp $evilpath + +completion_tcsh_start + +set got [completion_tcsh_list {module unuse }] +completion_assert_no_exec $marker +completion_assert_contains $got [list $evilpath] + +completion_tcsh_close +unsetenv_path_var MODULEPATH + +set timeout $old_timeout +file delete -force $marker + + # # Cleanup #