diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a55787..bea8956 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,42 @@ All notable changes to the Vim Plugin Manager will be documented in this file. +## [Unreleased] + +### Security +- **Option injection via `.gitmodules` branch** (#9, HIGH, proven RCE): + `submodule..branch` is passed to `git pull origin ` - a + leading-dash value is parsed by git as an option + (`--upload-pack=` executes the command, demonstrated locally with + git 2.43 despite shell quoting). New `core#util#sanitize_branch` + (first char may not be `-`, charset `^[A-Za-z0-9._/-]+$`) applied at + the `.gitmodules` read (update pull flow) and at + `git submodule add -b`; refused values warn and fall back to the + default branch resolution. +- **Pathspec injection via `.gitmodules` path** (#9, HIGH): module paths + reach `git rm -f` / `git submodule deinit -f` as pathspecs - `path = *` + would remove every tracked file. New `core#util#validate_module_path` + (refuses `* ? [ ]`, `..`, leading `-`, absolute paths; requires a + repo-relative path under `plugins_dir`) applied in `remove` (covers + `:PluginManager remove` and `gc`); refused values warn and abort the + removal. +- **Directory traversal via the `dir` option** (#9, MED): clone/install + targets accepted `..`, absolute paths and nested paths. New + `core#util#validate_dir_name` (plain basename only) applied to both + the dict and the deprecated positional forms; refused values warn and + fall back to the default plugin name. +- **Credential leak** (#9, MED): `https://user:token@host` userinfo no + longer reaches pushed commit messages ("Add/Remove plugin"), + error messages (REPO_NOT_FOUND), command traces (git#execute, + run_in_dir) or sidebar output. New `core#util#sanitize_url` / + `sanitize_cmd` strip the userinfo at every display point; the raw + value is still used for the actual git call. + +### Added +- `tests/security.vader`: unit tests for the four validators plus an + integration test proving a hostile `.gitmodules` branch never + propagates to the pull flow. + ## [2.2.8] - 2026-09-20 ### Fixed diff --git a/autoload/plugin_manager/cmd/add.vim b/autoload/plugin_manager/cmd/add.vim index 7fa7e8e..98158e5 100644 --- a/autoload/plugin_manager/cmd/add.vim +++ b/autoload/plugin_manager/cmd/add.vim @@ -26,7 +26,8 @@ function! plugin_manager#cmd#add#execute(...) abort " For remote plugins, check repository exists if !l:is_local && !plugin_manager#git#repository_exists(l:module_url) - call plugin_manager#core#throw('add', 'REPO_NOT_FOUND', 'Repository not found: ' . l:module_url) + call plugin_manager#core#throw('add', 'REPO_NOT_FOUND', + \ 'Repository not found: ' . plugin_manager#core#util#sanitize_url(l:module_url)) endif " Install diff --git a/autoload/plugin_manager/cmd/declare.vim b/autoload/plugin_manager/cmd/declare.vim index 3e3860a..181aebc 100644 --- a/autoload/plugin_manager/cmd/declare.vim +++ b/autoload/plugin_manager/cmd/declare.vim @@ -227,7 +227,8 @@ function! s:on_clone_done(ctx, name, url, options, target, result) abort else call plugin_manager#ui#complete_operation(l:op_id, 'fail', 'Failed') call plugin_manager#ui#log_detail('declare', - \ 'submodule add failed for ' . a:name . ' (' . a:url . ')', + \ 'submodule add failed for ' . a:name . ' (' + \ . plugin_manager#core#util#sanitize_url(a:url) . ')', \ 'warn') let a:ctx.errors += 1 endif diff --git a/autoload/plugin_manager/cmd/remove.vim b/autoload/plugin_manager/cmd/remove.vim index a260089..15919b2 100644 --- a/autoload/plugin_manager/cmd/remove.vim +++ b/autoload/plugin_manager/cmd/remove.vim @@ -167,6 +167,16 @@ function! s:remove_module(module_name, module_path) abort " absolute path (filesystem search): normalize so the vim_dir prefix is " never applied twice. let l:rel_path = plugin_manager#core#util#make_relative_path(a:module_path) + " .gitmodules is user-writable content: its path value is passed as a + " pathspec to `git rm` / `git submodule deinit` - a glob ('*') would match + " every tracked file (issue #9). Refuse + abort the removal on invalid. + let l:rel_path = plugin_manager#core#util#validate_module_path(l:rel_path) + if empty(l:rel_path) + call plugin_manager#ui#complete_operation(l:op_id, 'fail', + \ 'Refusing to remove: untrusted module path (see log)') + call plugin_manager#git#refresh_modules_cache() + return 0 + endif let l:deinit_result = plugin_manager#git#execute( \ 'git submodule deinit -f ' . shellescape(l:rel_path), l:vim_dir, 0, 0) @@ -239,7 +249,8 @@ function! s:commit_removal(module_name, module_info) abort let l:vim_dir = plugin_manager#core#util#get_config('vim_dir', '') if !empty(a:module_info) && has_key(a:module_info, 'url') - let l:commit_msg .= " (" . a:module_info.url . ")" + " Pushed to remotes: strip credentials from the URL first. + let l:commit_msg .= " (" . plugin_manager#core#util#sanitize_url(a:module_info.url) . ")" endif " Stage .gitmodules (updated by git rm); run in vim_dir for repo-root scope. diff --git a/autoload/plugin_manager/core/util.vim b/autoload/plugin_manager/core/util.vim index cc0522e..29e9c1e 100644 --- a/autoload/plugin_manager/core/util.vim +++ b/autoload/plugin_manager/core/util.vim @@ -115,7 +115,8 @@ function! plugin_manager#core#util#run_in_dir(cmd, dir) abort let l:full_cmd = empty(a:dir) ? a:cmd : 'cd ' . shellescape(a:dir) . ' && ' . a:cmd if plugin_manager#core#util#get_config('trace_commands', 0) - call plugin_manager#core#log#trace('util', 'run_in_dir: ' . l:full_cmd) + call plugin_manager#core#log#trace('util', + \ 'run_in_dir: ' . plugin_manager#core#util#sanitize_cmd(l:full_cmd)) endif let l:output = system(l:full_cmd) @@ -154,6 +155,76 @@ function! plugin_manager#core#util#get_plugin_dir(type) abort return l:plugins_dir . '/' . l:start_dir endfunction +" ------------------------------------------------------------------------------ +" UNTRUSTED INPUT VALIDATION (issue #9) +" +" Contract: warn + safe fallback (empty string), never a hard throw. The +" degraded state is visible in the log, the operation stays usable. +" ------------------------------------------------------------------------------ + +" Strip https/http userinfo (user:token@) from a URL: logs, error messages +" and commit texts must never carry credentials. +function! plugin_manager#core#util#sanitize_url(url) abort + return substitute(a:url, '\(https\?://\)[^/@]*@', '\1', '') +endfunction + +" Strip userinfo from every https/http URL inside a command string (traces +" and error messages embed whole commands). +function! plugin_manager#core#util#sanitize_cmd(cmd) abort + return substitute(a:cmd, '\(https\?://\)[^/@]*@', '\1', 'g') +endfunction + +" Validate a branch name coming from .gitmodules or user declarations. +" Returns the value when it is a plain refname, '' otherwise (callers fall +" back to default branch resolution). A leading dash is parsed by git as an +" option (--upload-pack = remote code execution, proven in issue #9); +" anything outside ^[A-Za-z0-9._/-]+$ is not a refname git would track. +function! plugin_manager#core#util#sanitize_branch(branch) abort + " First character may not be '-': a leading dash is parsed by git as an + " option (--upload-pack = remote code execution, proven in issue #9). + if a:branch =~# '^[A-Za-z0-9._/][A-Za-z0-9._/-]*$' + return a:branch + endif + call plugin_manager#core#log#warn('util', 'untrusted branch ignored: ' . a:branch) + return '' +endfunction + +" Validate a module path coming from .gitmodules before it is used as a +" pathspec (git rm / git submodule deinit) or a filesystem target. Returns +" the value when it is a repo-relative path under plugins_dir, '' otherwise. +" Glob characters as a pathspec expand to unrelated files ('*' removes +" everything); '..' and absolute paths escape the config repo. +function! plugin_manager#core#util#validate_module_path(path) abort + if empty(a:path) || a:path =~# '^-' || a:path =~# '^[/~]' + \ || a:path =~# '[*?\[]' || a:path =~# '\.\.' + call plugin_manager#core#log#warn('util', + \ 'untrusted module path ignored: ' . a:path) + return '' + endif + " plugins_dir may be configured absolute: compare in repo-relative form. + let l:plugins_dir = plugin_manager#core#util#make_relative_path( + \ plugin_manager#core#util#get_config('plugins_dir', '')) + if stridx(a:path, l:plugins_dir . '/') != 0 + call plugin_manager#core#log#warn('util', + \ 'untrusted module path ignored: ' . a:path) + return '' + endif + return a:path +endfunction + +" Validate a `dir` option (custom install name). Returns the value when it +" is a plain basename, '' otherwise (callers fall back to the default plugin +" name). '..' and absolute paths would clone outside plugins_dir. +function! plugin_manager#core#util#validate_dir_name(dir) abort + if empty(a:dir) || a:dir =~# '\.\.' || a:dir =~# '^[/~]' + \ || fnamemodify(a:dir, ':t') !=# a:dir + call plugin_manager#core#log#warn('util', + \ 'untrusted dir option ignored: ' . a:dir) + return '' + endif + return a:dir +endfunction + " ------------------------------------------------------------------------------ " URL AND PLUGIN NAME UTILITIES " ------------------------------------------------------------------------------ @@ -268,6 +339,9 @@ function! plugin_manager#core#util#process_plugin_options(args) abort echohl WarningMsg echomsg "Invalid 'load' value: " . l:val . ". Using default: 'start'" echohl None + elseif l:key ==# 'dir' + " Traversal guard: dir is a clone/install target (issue #9). + let l:options.dir = plugin_manager#core#util#validate_dir_name(l:val) else let l:options[l:key] = l:val endif @@ -278,7 +352,7 @@ function! plugin_manager#core#util#process_plugin_options(args) abort endif endfor elseif len(a:args) >= 1 && type(a:args[0]) == v:t_string - let l:options.dir = a:args[0] + let l:options.dir = plugin_manager#core#util#validate_dir_name(a:args[0]) if len(a:args) >= 2 && a:args[1] ==# 'opt' let l:options.load = 'opt' endif diff --git a/autoload/plugin_manager/git.vim b/autoload/plugin_manager/git.vim index a62ecd5..c08bd86 100644 --- a/autoload/plugin_manager/git.vim +++ b/autoload/plugin_manager/git.vim @@ -302,13 +302,17 @@ function! plugin_manager#git#execute(cmd, dir, ...) abort let l:full_cmd = 'git -C ' . shellescape(a:dir) . strpart(a:cmd, 3) endif + " Display/traced variant: commands embed user URLs which may carry + " credentials - never write the userinfo to logs, UI or error messages. + let l:display_cmd = plugin_manager#core#util#sanitize_cmd(l:full_cmd) + " Trace the command to the debug log if enabled if get(g:, 'plugin_manager_trace_commands', 0) - call plugin_manager#core#log#trace('git', 'exec: ' . l:full_cmd) + call plugin_manager#core#log#trace('git', 'exec: ' . l:display_cmd) endif if l:output_to_ui && exists('*plugin_manager#ui#update_sidebar') - call plugin_manager#ui#update_sidebar(['Executing: ' . a:cmd], 1) + call plugin_manager#ui#update_sidebar(['Executing: ' . l:display_cmd], 1) endif let l:output = system(l:full_cmd) @@ -322,8 +326,9 @@ function! plugin_manager#git#execute(cmd, dir, ...) abort endif if !l:success && l:throw_on_error - " Standardized error handling - call plugin_manager#core#throw('git', 'COMMAND_FAILED', 'Command failed: ' . a:cmd . ' - ' . l:output) + " Standardized error handling (sanitized: no credentials in messages) + call plugin_manager#core#throw('git', 'COMMAND_FAILED', + \ 'Command failed: ' . l:display_cmd . ' - ' . l:output) endif return {'success': l:success, 'output': l:output} @@ -390,6 +395,13 @@ function! plugin_manager#git#collect_status_local(module_path) abort \ ' submodule.' . shellescape(l:rel_path) . '.branch', \ '', 0, 0) let l:remote_branch = l:res.success ? substitute(l:res.output, '\n', '', 'g') : '' + " .gitmodules is user-writable config content: its branch value is passed + " to `git pull origin ` - a leading dash would be parsed as a git + " option (proven RCE via --upload-pack, issue #9). Refuse + fall back to + " the default branch resolution. + if !empty(l:remote_branch) + let l:remote_branch = plugin_manager#core#util#sanitize_branch(l:remote_branch) + endif " If not found in .gitmodules, try to determine from the current branch's upstream if empty(l:remote_branch) && l:result.branch !=# 'detached' @@ -530,9 +542,15 @@ function! plugin_manager#git#add_submodule(url, install_dir, options) abort let l:cmd = 'git -c protocol.file.allow=always submodule add' endif - " Add branch option if specified + " Add branch option if specified. The value may come from the vimrc (or + " an API caller): refuse option-like values instead of passing them to + " `git submodule add -b` (issue #9). + let l:branch = '' if !empty(a:options.branch) - let l:cmd .= ' -b ' . shellescape(a:options.branch) + let l:branch = plugin_manager#core#util#sanitize_branch(a:options.branch) + if !empty(l:branch) + let l:cmd .= ' -b ' . shellescape(l:branch) + endif endif " Add URL and path @@ -565,10 +583,11 @@ function! plugin_manager#git#add_submodule(url, install_dir, options) abort " Commit changes (must run at repo root). A failed pointer commit " leaves the submodule unrecorded: never swallow it - warn and fail the - " add so the caller reports the degraded outcome. - let l:commit_msg = 'Add ' . a:url . ' plugin' - if !empty(a:options.branch) - let l:commit_msg .= ' (branch: ' . a:options.branch . ')' + " add so the caller reports the degraded outcome. The URL may embed + " credentials: strip userinfo before it lands in a pushed commit. + let l:commit_msg = 'Add ' . plugin_manager#core#util#sanitize_url(a:url) . ' plugin' + if !empty(l:branch) + let l:commit_msg .= ' (branch: ' . l:branch . ')' elseif !empty(a:options.tag) let l:commit_msg .= ' (tag: ' . a:options.tag . ')' endif @@ -596,7 +615,8 @@ function! plugin_manager#git#add_remote(url, name) abort " Check if the repository exists if !plugin_manager#git#repository_exists(a:url) - call plugin_manager#core#throw('remote', 'REPO_NOT_FOUND', 'Repository not found: ' . a:url) + call plugin_manager#core#throw('remote', 'REPO_NOT_FOUND', + \ 'Repository not found: ' . plugin_manager#core#util#sanitize_url(a:url)) endif " Generate remote name if not provided diff --git a/tests/security.vader b/tests/security.vader new file mode 100644 index 0000000..ece3e2d --- /dev/null +++ b/tests/security.vader @@ -0,0 +1,110 @@ +" Security validators (issue #9): untrusted values must never reach git +" commands, filesystem targets, or logs unvalidated. Contract: warn + safe +" fallback (empty string), never a hard throw. Unit-level tests; the +" hostile-branch case exercises the full collect_status_local path. + +Before: + let g:_pm_sec_saved = { + \ 'vim_dir': get(g:, 'plugin_manager_vim_dir', ''), + \ 'plugins_dir': get(g:, 'plugin_manager_plugins_dir', ''), + \ 'logging': get(g:, 'plugin_manager_enable_logging', 0), + \ } + let g:plugin_manager_vim_dir = '/tmp/pm-sec-test/vim' + let g:plugin_manager_plugins_dir = '/tmp/pm-sec-test/vim/pack/plugins' + call delete('/tmp/pm-sec-test', 'rf') + call mkdir('/tmp/pm-sec-test/vim', 'p') + let g:plugin_manager_enable_logging = 1 + unlet! g:plugin_manager_debug_mode + function! g:_pm_sec_log() abort + let l:p = '/tmp/pm-sec-test/vim/logs/plugin_manager.log' + return filereadable(l:p) ? join(readfile(l:p), "\n") : '' + endfunction + +After: + call delete('/tmp/pm-sec-test', 'rf') + let g:plugin_manager_vim_dir = g:_pm_sec_saved.vim_dir + let g:plugin_manager_plugins_dir = g:_pm_sec_saved.plugins_dir + let g:plugin_manager_enable_logging = g:_pm_sec_saved.logging + unlet g:_pm_sec_saved + +Execute (sanitize_branch accepts ordinary refnames): + AssertEqual 'main', plugin_manager#core#util#sanitize_branch('main') + AssertEqual 'feature/x.y-z', plugin_manager#core#util#sanitize_branch('feature/x.y-z') + +Execute (sanitize_branch refuses option-like and hostile values): + AssertEqual '', plugin_manager#core#util#sanitize_branch('--upload-pack=/bin/true') + AssertEqual '', plugin_manager#core#util#sanitize_branch('-b') + AssertEqual '', plugin_manager#core#util#sanitize_branch('main;rm -rf /') + AssertEqual '', plugin_manager#core#util#sanitize_branch('main$(x)') + AssertEqual '', plugin_manager#core#util#sanitize_branch('ma in') + Assert g:_pm_sec_log() =~# 'untrusted branch', + \ 'a refused branch must be logged, got: ' . g:_pm_sec_log() + +Execute (validate_module_path accepts repo-relative plugin paths): + AssertEqual 'pack/plugins/start/myplugin', + \ plugin_manager#core#util#validate_module_path('pack/plugins/start/myplugin') + AssertEqual 'pack/plugins/opt/lazy', + \ plugin_manager#core#util#validate_module_path('pack/plugins/opt/lazy') + +Execute (validate_module_path refuses glob chars, traversal and escapes): + AssertEqual '', plugin_manager#core#util#validate_module_path('*') + AssertEqual '', plugin_manager#core#util#validate_module_path('my*') + AssertEqual '', plugin_manager#core#util#validate_module_path('pack/plugins/start/[a]b') + AssertEqual '', plugin_manager#core#util#validate_module_path('pack/plugins/start/../evil') + AssertEqual '', plugin_manager#core#util#validate_module_path('evil') + AssertEqual '', plugin_manager#core#util#validate_module_path('-x') + AssertEqual '', plugin_manager#core#util#validate_module_path('') + Assert g:_pm_sec_log() =~# 'untrusted module path', + \ 'a refused path must be logged, got: ' . g:_pm_sec_log() + +Execute (validate_dir_name accepts plain basenames only): + AssertEqual 'myplugin', plugin_manager#core#util#validate_dir_name('myplugin') + AssertEqual '', plugin_manager#core#util#validate_dir_name('../evil') + AssertEqual '', plugin_manager#core#util#validate_dir_name('/abs/path') + AssertEqual '', plugin_manager#core#util#validate_dir_name('a/b') + AssertEqual '', plugin_manager#core#util#validate_dir_name('') + Assert g:_pm_sec_log() =~# 'untrusted dir option', + \ 'a refused dir must be logged, got: ' . g:_pm_sec_log() + +Execute (sanitize_url strips https userinfo, keeps the rest): + AssertEqual 'https://github.com/a/b.git', + \ plugin_manager#core#util#sanitize_url('https://user:token@github.com/a/b.git') + AssertEqual 'https://github.com/a/b.git', + \ plugin_manager#core#util#sanitize_url('https://token@github.com/a/b.git') + AssertEqual 'http://host/x', + \ plugin_manager#core#util#sanitize_url('http://u@host/x') + AssertEqual 'https://github.com/a/b.git', + \ plugin_manager#core#util#sanitize_url('https://github.com/a/b.git') + AssertEqual 'git@github.com:a/b.git', + \ plugin_manager#core#util#sanitize_url('git@github.com:a/b.git') + +Execute (a hostile .gitmodules branch never reaches git pull): + " Minimal module: a real repo with a main branch, a crafted .gitmodules + " whose branch value is a git option. + let g:_pm_sec_mod = '/tmp/pm-sec-test/vim/pack/plugins/start/hostile' + call mkdir(g:_pm_sec_mod, 'p') + call system('git init -q ' . shellescape(g:_pm_sec_mod)) + call system('git -C ' . shellescape(g:_pm_sec_mod) . ' config user.email "t@t.com"') + call system('git -C ' . shellescape(g:_pm_sec_mod) . ' config user.name "T"') + call writefile(['x'], g:_pm_sec_mod . '/f') + call system('git -C ' . shellescape(g:_pm_sec_mod) . ' add .') + call system('git -C ' . shellescape(g:_pm_sec_mod) . ' commit -qm init') + call writefile([ + \ '[submodule "pack/plugins/start/hostile"]', + \ "\tpath = pack/plugins/start/hostile", + \ "\turl = https://example.com/a/b.git", + \ "\tbranch = --upload-pack=/bin/true", + \ ], '/tmp/pm-sec-test/vim/.gitmodules') + call plugin_manager#git#refresh_modules_cache() + let g:_pm_sec_exc = '' + try + let g:_pm_sec_st = plugin_manager#git#collect_status_local(g:_pm_sec_mod) + catch + let g:_pm_sec_exc = v:exception . ' @ ' . v:throwpoint + endtry + AssertEqual '', g:_pm_sec_exc, 'collect_status_local must not throw: ' . g:_pm_sec_exc + Assert stridx(g:_pm_sec_st.remote_branch, '--') == -1, + \ 'the hostile branch value must not propagate, got: ' . g:_pm_sec_st.remote_branch + Assert g:_pm_sec_log() =~# 'untrusted branch', + \ 'the refused branch must be logged, got: ' . g:_pm_sec_log() + unlet g:_pm_sec_exc g:_pm_sec_st