From 2b0b2f960903974c95eed3da84059507bc2896da Mon Sep 17 00:00:00 2001 From: dipto0321 Date: Fri, 3 Jul 2026 07:22:48 +0600 Subject: [PATCH] fix(platform): single-quote paths in QuotePath to neutralize NVM_DIR injection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `platform.QuotePath` wrapped POSIX paths in double quotes and only escaped `"` and `\`, leaving `$`, `$(...)`, and backticks exposed to bash expansion. `NVM.Version()` builds a `source && nvm --version` script using whatever `NVM_DIR` (or `~/.nvm`) provides, so an attacker who can set `NVM_DIR` (CI runner env, direnv `.envrc`, devcontainer, compromised build step) gets arbitrary command execution the moment nvm is detected or queried. Switch QuotePath's POSIX branch to single-quote wrapping with `'\''` to escape embedded `'`. Inside single quotes bash performs no expansion — `$`, `$(...)`, backticks, and `\\` all become literal. The Windows (cmd.exe) branch is unchanged: it still wraps paths-with-spaces in double quotes; full injection-safety on Windows would need a different approach but nvm-windows is its own detector with its own quoting story. The existing test table's expectations are updated to the new contract; a new `TestQuotePathNeutralizesShellInjection` exercises a payload path with `$(touch /tmp/PWND_…)` through `printf '%s'` and asserts both that the round-trip is byte-identical AND that the sentinel file is never created. A new detector-level `TestNVM_Version_NVMDirShellInjectionIsNeutralized` points `NVM_DIR` at a payload string and asserts no unquoted `$(touch` occurrence in the emitted script. Refs #43 Co-Authored-By: puku-ai-2.8 --- CHANGELOG.md | 18 ++++ internal/detector/nvm_test.go | 79 +++++++++++++++ internal/platform/platform_test.go | 151 +++++++++++++++++++++-------- internal/platform/shell.go | 54 +++++++++-- 4 files changed, 250 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fd341c0..331a5d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -139,6 +139,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 carrying the user's globals forward to the new default exactly once. Added a regression test pinning the `RestoreFromSnapshot`-reads-source-path contract. Closes #42. +- `internal/platform/shell.go` (`QuotePath`): on POSIX the quoting + rule for paths embedded in `bash -c` scripts now uses **single + quotes** with `'\''` to escape embedded `'` bytes. The previous + rule used double quotes and only escaped `"` and `\\`, so a path + containing `$USER`, `$(...)`, or backticks was still subject to + shell expansion. `NVM.Version()` builds a `source && nvm + --version` script using whatever `NVM_DIR` (or `~/.nvm`) provides, + so setting `NVM_DIR='/tmp/$(touch /tmp/PWNED)'` would expand + inside bash's double quotes before reaching nvm. Single-quote + wrapping disables bash variable expansion, command substitution, + and backtick substitution entirely. The Windows branch + (cmd.exe-unaware double-quote wrapping for paths-with-spaces) is + unchanged. The test table for `QuotePath` now expects single quotes + on POSIX, plus a dedicated `TestQuotePathNeutralizesShellInjection` + regression test, plus a `TestNVM_Version_NVMDirShellInjectionIsNeutralized` + detector-level test that points `NVM_DIR` at a payload string and + asserts no unquoted `$(touch` occurrence in the emitted script. + Closes #43. ## [0.0.0] - 2024-07-01 diff --git a/internal/detector/nvm_test.go b/internal/detector/nvm_test.go index 6c4756a..91f85e6 100644 --- a/internal/detector/nvm_test.go +++ b/internal/detector/nvm_test.go @@ -5,6 +5,7 @@ import ( "errors" "os" "path/filepath" + "runtime" "strings" "testing" @@ -333,6 +334,84 @@ func TestNVM_Version_NoNVMScript(t *testing.T) { } } +// TestNVM_Version_NVMDirShellInjectionIsNeutralized is the regression +// test for #43. Even when NVM_DIR contains shell metacharacters like +// `$(touch …)` or backticks, the script that runs through runScript +// must keep those characters LITERAL — bash must not perform command +// substitution inside the single-quoted region that QuotePath emits. +// +// The test sets NVM_DIR to a string that contains a command-substitution +// payload but does NOT require a real directory to exist (nvm.sh is +// stubbed via runScript), captures the script that NVM.Version() builds, +// and asserts that the payload characters appear inside a single-quoted +// span rather than as bare command substitution. +func TestNVM_Version_NVMDirShellInjectionIsNeutralized(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("cmd.exe unquote rules differ; QuotePath's Windows branch is covered in internal/platform tests") + } + + // Pick a payload directory NAME that QuotePath must absorb + // verbatim — we never create it on disk, but nvmDir()/nvmScriptPath() + // only return the strings, never validate them, and the runScript + // stub returns canned stdout without touching the filesystem. + // + // The bytes `$(touch /tmp/PWND_BY_NVM_DIR)` are just path bytes, + // not actual shell substitution. NVM_DIR is set to a path that + // looks like `/tmp//$(touch /tmp/PWND_BY_NVM_DIR)`. + maliciousDirName := "$(touch /tmp/PWND_BY_NVM_DIR)" + t.Setenv("NVM_DIR", "/tmp/anything-at-all/"+maliciousDirName) + // Make sure no leftover sentinel from a previous run. + _ = os.Remove("/tmp/PWND_BY_NVM_DIR") + defer os.Remove("/tmp/PWND_BY_NVM_DIR") + + rec := withStubScript(t, "0.40.5\n", nil) + if _, err := NewNVM().Version(); err != nil { + t.Fatalf("Version(): %v", err) + } + + // The script MUST contain the payload inside single quotes, not + // as bare `$(touch …)`. Otherwise bash would expand it the moment + // the script runs in production. + script := rec.script + + // 1. The malicious substring must be inside a single-quoted span. + // nvmScriptPath() appends `/nvm.sh`, so the full quoted form + // is `…/$(touch /tmp/PWND_BY_NVM_DIR)/nvm.sh`. We find the + // payload's byte offset and walk backwards/forward to the + // nearest `'` on each side; both must surround the payload, + // proving it lives inside a single-quoted region. + idx := strings.Index(script, "$(touch") + if idx < 0 { + t.Fatalf("payload substring %q not present in script — the malicious NVM_DIR was not used. script=%q", + "$(touch", script) + } + leftQuote := strings.LastIndex(script[:idx], "'") + rightQuote := strings.Index(script[idx:], "'") + if leftQuote < 0 || rightQuote < 0 { + t.Fatalf("payload not surrounded by single quotes on both sides. leftQuote=%d rightQuote=%d script=%q", + leftQuote, rightQuote, script) + } + // If the script also contains double-quotes around the payload, + // bash would still perform command-substitution inside them. + // We tolerate overall double-quotes elsewhere in the script; + // the regression check is specifically that the payload bytes are + // not in an unquoted region. + + // 2. The payload substring `$(touch` (if present in the script + // as bare bytes) must appear only inside a single-quoted span. + // If QuotePath passed it through bare, that's a regression. + if idx := strings.Index(script, "$(touch"); idx >= 0 { + // Count single quotes BEFORE the payload occurrence: odd + // count means we are inside a single-quoted span. + upTo := script[:idx] + singleQuoteCount := strings.Count(upTo, "'") + if singleQuoteCount%2 == 0 { + t.Errorf("payload substring %q at offset %d is OUTSIDE single quotes — bash would expand it. script=%q", + "$(touch", idx, script) + } + } +} + // --- NVM.ListInstalled ------------------------------------------------- func TestNVM_ListInstalled_HappyPath(t *testing.T) { diff --git a/internal/platform/platform_test.go b/internal/platform/platform_test.go index 4b0eb09..79cfd08 100644 --- a/internal/platform/platform_test.go +++ b/internal/platform/platform_test.go @@ -2,6 +2,7 @@ package platform import ( "context" + "os" "path/filepath" "runtime" "testing" @@ -83,12 +84,21 @@ func TestPlatformHelpers(t *testing.T) { } } -// TestQuotePathNoSpecials verifies that paths without spaces or shell -// metacharacters pass through unchanged (no spurious quoting). +// TestQuotePathNoSpecials documents that all paths now go through +// single-quote wrapping (POSIX) or selective double-quote wrapping +// (Windows). The output is always a shell-safe literal of the input. func TestQuotePathNoSpecials(t *testing.T) { in := "/usr/local/bin" - if got := QuotePath(in); got != in { - t.Errorf("QuotePath(%q) = %q, want unchanged", in, got) + got := QuotePath(in) + if runtime.GOOS == "windows" { + if got != in { + t.Errorf("QuotePath(%q) = %q, want unchanged on Windows", in, got) + } + return + } + want := "'/usr/local/bin'" + if got != want { + t.Errorf("QuotePath(%q) = %q, want %q", in, got, want) } } @@ -106,20 +116,29 @@ func TestQuotePathWithSpace(t *testing.T) { if got == in { t.Error("QuotePath left a space-containing path unquoted") } - // Must start with a quote. - if got[0] != '"' { - t.Errorf("QuotePath did not wrap in double quotes: %q", got) + // POSIX: must start with a single quote (and end with one). + // Windows: must start with a double quote. + if runtime.GOOS == "windows" { + if got[0] != '"' { + t.Errorf("QuotePath did not wrap in double quotes (Windows): %q", got) + } + } else { + if got[0] != '\'' { + t.Errorf("QuotePath did not wrap in single quotes (POSIX): %q", got) + } } } // TestQuotePathTable exercises QuotePath across the cases that matter for // cross-platform support: spaces, embedded quotes, backslashes (Windows -// path separators), shell metacharacters on unix ($, `, |), and edges +// path separators), shell metacharacters on unix ($, `, |, '), and edges // (empty, single space, trailing slash). // // Expected output is computed by branching on runtime.GOOS because -// QuotePath uses a different unsafe-char set on Windows vs unix: on -// Windows only `"` is unsafe; on unix ` "$`<>|'` are all unsafe. +// QuotePath uses a different quoting strategy on Windows vs unix: on +// Windows we wrap paths-with-spaces in double quotes (the cmd.exe +// unquote rules); on unix we ALWAYS wrap in single quotes so that `$`, +// backticks, and `\\` are not interpreted by bash. See #43. // // We run the same table on every OS so CI catches regressions on all // three platforms (ubuntu, macos, windows in our matrix). @@ -130,26 +149,32 @@ func TestQuotePathTable(t *testing.T) { want string } - // Unix-style cases (forward slashes, no backslashes). On Windows the - // unsafe-char set is narrower — backslash is treated as a regular - // char and not escaped — so paths containing backslashes are only - // valid inputs on Windows. We split the table by OS to keep the - // expectations unambiguous. + // Unix-style cases. On POSIX we always single-quote; embedded `'` + // becomes `'\''`. We deliberately do NOT preserve `$USER` or + // backticks verbatim — that was the command-injection bug. unixCases := []tc{ {"empty", "", `""`}, - {"plain_unix_path", "/usr/local/bin", "/usr/local/bin"}, - {"single_space", " ", `" "`}, - {"trailing_space", "/Users/dipto/My App ", `"/Users/dipto/My App "`}, - {"embedded_space", "/Users/dipto/My App/bin", `"/Users/dipto/My App/bin"`}, - {"multiple_spaces", "/Users/My App/bin", `"/Users/My App/bin"`}, - {"dollar_sign", "/home/$USER/foo", `"/home/$USER/foo"`}, - {"backtick", "/tmp/`whoami`/x", "\"/tmp/`whoami`/x\""}, - {"pipe", "/tmp/a|b/c", `"/tmp/a|b/c"`}, - {"redirect", "/tmp/a>b/c", `"/tmp/a>b/c"`}, - {"single_quote", "/tmp/it's/x", `"/tmp/it's/x"`}, - {"embedded_dquote", `/tmp/oops"quote/x`, `"/tmp/oops\"quote/x"`}, - {"trailing_slash", "/var/log/", "/var/log/"}, + {"plain_unix_path", "/usr/local/bin", "'/usr/local/bin'"}, + {"single_space", " ", `' '`}, + {"trailing_space", "/Users/dipto/My App ", "'/Users/dipto/My App '"}, + {"embedded_space", "/Users/dipto/My App/bin", "'/Users/dipto/My App/bin'"}, + {"multiple_spaces", "/Users/My App/bin", "'/Users/My App/bin'"}, + // Regression cases for #43 — these previously passed through + // unchanged (or with bare double quotes that bash would expand). + {"dollar_sign", "/home/$USER/foo", "'/home/$USER/foo'"}, + {"backtick", "/tmp/`whoami`/x", "'/tmp/`whoami`/x'"}, + {"command_subst", "/tmp/$(id)/x", "'/tmp/$(id)/x'"}, + {"pipe", "/tmp/a|b/c", "'/tmp/a|b/c'"}, + {"redirect", "/tmp/a>b/c", "'/tmp/a>b/c'"}, + // Single quote inside the path becomes the canonical + // close/reopen/escape dance. + {"single_quote", "/tmp/it's/x", `'/tmp/it'\''s/x'`}, + {"embedded_dquote", `/tmp/oops"quote/x`, `'/tmp/oops"quote/x'`}, + {"trailing_slash", "/var/log/", "'/var/log/'"}, } + // Windows: same logic as before — only `"` (and spaces) trigger + // double-quote wrapping; `\\` inside the path gets escaped for + // cmd.exe double-quote semantics. windowsCases := []tc{ {"empty", "", `""`}, {"plain_windows_path", `C:\node`, `C:\node`}, @@ -177,30 +202,66 @@ func TestQuotePathTable(t *testing.T) { } } +// TestQuotePathNeutralizesShellInjection is the regression test for #43: +// a path containing `$(touch /tmp/PWNED)` or `\`whoami\“ must NOT be +// expanded when fed back through bash. We embed the quoted form in a +// shell script that touches a side-channel file if (and only if) +// expansion happens. +func TestQuotePathNeutralizesShellInjection(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("cmd.exe unquote rules differ; Windows path is covered by TestQuotePathTable") + } + // A path that, if expanded, would `touch` a sentinel file. After + // running the test, the sentinel MUST NOT exist. + malicious := "/tmp/nodeup-quote-path-test-$$(touch /tmp/nodeup-pwned-$$)" + sentinel := "/tmp/nodeup-pwned-$$" + // Clean up any leftover from a previous failed run. + _ = os.Remove(sentinel) + defer func() { + // `$$` is process-id; multiple test runs may target the same + // sentinel if the test reuses one. Clean using the literal + // basename pattern. + _ = os.Remove("/tmp/nodeup-pwned-$$") + }() + + // Compose a shell script that runs `id -un` after sourcing a + // non-existent file (a no-op for our purposes) but that, if the + // path were expanded by bash, would touch the sentinel. We then + // compare the printed value to the literal input — they must match. + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + // `printf '%s'` echoes the literal value of the quoted form; if + // bash expanded `$(touch …)`, the touch side-effect would run AND + // the printed value would be the empty string (touch produces no + // stdout). + script := "printf '%s' " + QuotePath(malicious) + res, err := RunShellScript(ctx, script) + if err != nil { + t.Fatalf("RunShellScript: %v", err) + } + if res.Stdout != malicious { + t.Errorf("round-trip mismatch: in=%q quoted=%q got=%q (bash expanded the quoted path)", + malicious, QuotePath(malicious), res.Stdout) + } + if _, err := os.Stat("/tmp/nodeup-pwned-$$"); err == nil { + t.Errorf("sentinel file /tmp/nodeup-pwned-$$ exists — bash expanded a metacharacter from the path, command injection is NOT neutralized") + } +} + // TestQuotePathRoundTrip confirms that the output of QuotePath, when // embedded in a shell script, re-evaluates to the original path. We // shell out via RunShellScript and `printf '%s'` the quoted form, then // compare to the original input. // -// We only test cases that QuotePath is *designed* to protect: paths -// with spaces, embedded double-quotes, and shell metacharacters that -// are not subject to variable expansion (|, <, >). Paths containing -// literal `$` or backticks are intentionally NOT tested here because -// QuotePath uses double-quotes (preserving shell variable expansion), -// so a `$` in the path will still be expanded by the shell. See the -// comment on QuotePath itself for this trade-off. +// On POSIX we now expect round-trip success for the full set of +// metacharacters ($, `, |) because QuotePath uses single quotes that +// disable shell expansion entirely. On Windows, the round-trip is +// not meaningful for cmd.exe (different unquote rules) — TestQuotePath +// covers per-OS expected output instead. func TestQuotePathRoundTrip(t *testing.T) { if runtime.GOOS == "plan9" { t.Skip("plan9 shell semantics differ; skip") } - // The round-trip uses RunShellScript, which on Windows invokes - // cmd.exe rather than bash. cmd.exe's unquote rules differ from - // bash (notably, cmd.exe drops the leading backslash from a - // double-quoted path and treats `\"` as a literal `"` followed by - // the next char). The test's purpose is to verify QuotePath's - // bash-mode quoting, so it is not meaningful on Windows. The - // table-driven TestQuotePath already covers the per-OS quoting - // logic for both unix and Windows. if runtime.GOOS == "windows" { t.Skip("cmd.exe unquote rules differ from bash; TestQuotePath covers per-OS expected output") } @@ -208,6 +269,12 @@ func TestQuotePathRoundTrip(t *testing.T) { "/Users/dipto/My App", "/Users/Double Space/bin", "/tmp/oops\"quote/x", + // Regression cases for #43: these now round-trip correctly + // because single-quote wrapping disables expansion. + "/home/$USER/foo", + "/tmp/`whoami`/x", + "/tmp/it's/a/path", + "/tmp/$(id -u)/x", } for _, p := range paths { p := p diff --git a/internal/platform/shell.go b/internal/platform/shell.go index def5c09..1be330b 100644 --- a/internal/platform/shell.go +++ b/internal/platform/shell.go @@ -141,21 +141,56 @@ func EnvWithSource() []string { // contains paths from the user (e.g., ~/.nvm/nvm.sh). Spaces are common // on Windows ("C:\Program Files\..."). // -// We use double-quotes on unix (preserving $variables) and double-quotes -// on Windows inside cmd.exe — which won't expand %FOO% inside double -// quotes the same way, but it's still safer than leaving the path bare. +// Quoting rules by platform: +// +// - On unix (bash): we wrap in single quotes, escaping any embedded +// single quote as `'\”` (close-quote, literal-quote, reopen-quote). +// Inside single quotes, bash performs NO expansion — `$`, “ ` “, +// `\\`, `"`, `;`, `|` all become literal. This is the only quoting +// mode that fully neuters command injection, including for values +// like NVM_DIR that are read from the environment without further +// validation. See #43. +// - On Windows (cmd.exe): we wrap in double quotes. cmd.exe's unquote +// rules differ from bash (notably, `\` and `"` have special +// handling); the goal here is just to keep paths-with-spaces +// together, not to defeat injection (the nvm path on Windows goes +// through the nvm-windows manager, which has its own quoting story). func QuotePath(p string) string { if p == "" { return `""` } - // If the path contains no characters that the shell would interpret, - // return it as-is. Otherwise wrap in double quotes and escape embedded - // double quotes / backslashes appropriately per platform. - safe := true - unsafeChars := " \"$`<>|'" if runtime.GOOS == "windows" { - unsafeChars = " \"" + return quotePathWindows(p) } + return quotePathPOSIX(p) +} + +// quotePathPOSIX wraps p in single quotes, escaping any embedded `'` as +// `'\”`. The result is safe to embed verbatim in a bash script — bash +// will not expand any variable, command substitution, or backtick +// inside the quoted region. +func quotePathPOSIX(p string) string { + // The set of characters that are unconditionally safe to leave + // inside a single-quoted string WITHOUT triggering any quoting + // requirement: every byte except `'` itself. We still always wrap + // (rather than only-on-demand) to keep the function's contract + // "result is always a shell-safe literal of the input" — callers + // can rely on the output being safe regardless of the input. + // + // Always wrap, then handle the single-quote escape inside. + return `'` + strings.ReplaceAll(p, `'`, `'\''`) + `'` +} + +// quotePathWindows wraps p for cmd.exe. On Windows, only `"` forces +// quoting (cmd.exe requires the path to be wrapped in double quotes if +// it contains spaces or tabs, and `"` inside the path must be escaped). +// Other characters — including `\\`, `%`, `&`, `|`, `<`, `>` — have +// cmd.exe-specific handling that we do NOT attempt to fully neutralize +// here; the use case is filesystem paths that may have spaces +// ("C:\Program Files\…"), not arbitrary user-controlled strings. +func quotePathWindows(p string) string { + safe := true + unsafeChars := " \"" for _, c := range p { if strings.ContainsRune(unsafeChars, c) { safe = false @@ -165,7 +200,6 @@ func QuotePath(p string) string { if safe { return p } - // Escape backslashes and double quotes for double-quoted string. escaped := strings.ReplaceAll(p, `\`, `\\`) escaped = strings.ReplaceAll(escaped, `"`, `\"`) return `"` + escaped + `"`