security: escape shell metacharacters in vault secrets used in shell model run fields

[stack72]

Summary

Vault secret values are resolved before CEL evaluation in model_resolver.ts:resolveVaultExpressions(). The current escaping chain prevents breaking out of a CEL string literal (backslash, double quote, single quote, backtick, whitespace), but does not escape shell metacharacters.

When a vault secret is injected via CEL into the run field of a command/shell model, characters such as $, &, |, ;, (, ) pass through unescaped into the shell command.

Attack Path

A user writes a model definition like:

run: "echo " + vault.get('my-vault', 'SECRET') + " done"

If SECRET contains $(cat /etc/passwd), the vault resolution step produces the CEL expression:

"echo " + "$(cat /etc/passwd)" + " done"

CEL evaluates this to the string echo $(cat /etc/passwd) done. This string is then executed as:

sh -c 'echo $(cat /etc/passwd) done'

The $(...) command substitution executes cat /etc/passwd. Similar issues exist with &, |, ;.

Relevant Code

src/domain/expressions/model_resolver.ts lines 775–782:

const escapedValue = secretValue
  .replace(/\\/g, "\\\\")
  .replace(/"/g, '\\"')
  .replace(/'/g, "\\'")
  .replace(/`/g, "\\`")
  .replace(/\n/g, "\\n")
  .replace(/\r/g, "\\r")
  .replace(/\t/g, "\\t");

$ is not escaped.

Mitigating Factors

• This requires the user to store a value in their own vault and write a CEL expression that injects it unquoted into a shell command. It is within the user's own trust domain.
• The vault is local and under the user's control.
• No remote attacker can influence vault contents without already having local write access.

However, this is a real foot-gun: users who write parameterised shell commands driven by vault secrets have no indication that special shell characters in secret values will be interpreted by the shell.

Recommended Fix

Add $ to the escaping chain in resolveVaultExpressions():

.replace(/\$/g, "\\$")

Add a test case: store a secret value containing $(echo injected) and verify it renders literally in shell output rather than being executed.

Also add documentation that vault values injected into shell run fields via CEL string concatenation are escaped for shell safety, so users understand the contract.

References

• src/domain/expressions/model_resolver.ts — resolveVaultExpressions()
• src/domain/models/command/shell/shell_model.ts — sh -c args.run
• Related fix: fix: escape single quotes and backticks in vault secret values #407 (added ' and backtick escaping to CEL layer, but shell metacharacters not addressed)

[stack72]

Implementation Plan

After investigating the cel-js source, the naive fix suggested in this issue (.replace(/\$/g, "\\$")) would not work — \$ is not a valid escape sequence in cel-js and would throw ParseError: Invalid escape sequence: \$.

Root cause

cel-js's STRING_ESCAPES map (lib/parser.js) defines only: \\, ?, ", ', `, a, b, f, n, r, t, v. Any other escape character throws a parse error.

Correct fix: .replace(/\$/g, "\\\\$")

This inserts \\$ (two actual backslashes + dollar) into escapedValue, which threads through two escape layers correctly:

Layer	Value	Notes
Secret	$(cat /etc/passwd)	raw
escapedValue	\\$(cat /etc/passwd)	after .replace(/\$/g, "\\\\$")
CEL string literal	"\\$(cat /etc/passwd)"	wrapped in "..."
After CEL eval	\$(cat /etc/passwd)	\\ → \; $ is literal in CEL
Shell sh -c	echo \$(cat /etc/passwd) done
Shell output	$(cat /etc/passwd) done	\$ = literal $, no substitution ✓

Changes required

src/domain/expressions/model_resolver.ts — add one line to the escaping chain, immediately after backslash escaping:

const escapedValue = secretValue
  .replace(/\\/g, "\\\\")
  .replace(/\$/g, "\\\\$")   // ← NEW
  .replace(/"/g, '\\"')
  .replace(/'/g, "\\'")
  .replace(/`/g, "\\`")
  .replace(/\n/g, "\\n")
  .replace(/\r/g, "\\r")
  .replace(/\t/g, "\\t");

src/domain/vaults/vault_expression_test.ts — update the 6 existing dollar-pattern tests (they verify the .split().join() approach preserves $&, $`, $', $$, $1… patterns). Each $ in a secret now maps to \\$ in the CEL expression, so expected values change:

Test	Secret	Old expected	New expected
$& pattern	my$&secret	'"my$&secret"'	'"my\\\\$&secret"'
$` pattern	prefix$`suffix	'"prefix$\\`suffix"'	'"prefix\\\\$\\`suffix"'
$' pattern	prefix$'suffix	'"prefix$\\\'suffix"'	'"prefix\\\\$\\\'suffix"'
$$ pattern	cost: $$100	'"cost: $$100"'	'"cost: \\\\$\\\\$100"'
$1$2$3 pattern	$1$2$3	'"$1$2$3"'	'"\\\\$1\\\\$2\\\\$3"'
multiple	a]$&b$\c$'d$$e$1f`	(complex)	update all $ to \\\\$

Add one new test:

await t.step(
  "should escape $ to prevent shell command substitution",
  async () => {
    const resolver = createResolverWithMockVault({
      "cmd-secret": "$(echo injected)",
    });
    const result = await resolver.resolveVaultExpressions(
      "vault.get(test-vault, cmd-secret)",
    );
    // CEL string contains \\$ so CEL evaluates to \$(echo injected)
    // Shell sees \$ = literal $, no command substitution
    assertEquals(result, '"\\\\$(echo injected)"');
  },
);

Out of scope

• Backtick shell injection: the existing \`` CEL escaping causes cel-js to unescape back to `` before reaching the shell, so cmd `` in a secret would still execute — separate issue.
• Other metacharacters (|, &, ;, (, )): not addressed per the recommendation in this issue.

[stack72]

closed via #419