From 0b142cf7b18e8886353d8b244453a40e3642a194 Mon Sep 17 00:00:00 2001 From: stack72 Date: Sat, 21 Feb 2026 21:07:08 +0100 Subject: [PATCH] fix: escape $ and ` in vault secrets to prevent shell command substitution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Vault secrets containing $(cmd) or `cmd` sequences were passed unescaped through CEL evaluation and executed as shell commands when used in command/shell models via sh -c. The fix applies a double-backslash prefix (\\$ and \\`) in the CEL string literal so that: - CEL sees \\ → single backslash, followed by a plain literal char - The shell then receives \$ or \` → treats as literal characters with no command or variable substitution Changes: - src/domain/expressions/model_resolver.ts: replace broken \$ / \` escaping with the correct \\$ / \\` prefix approach - src/domain/vaults/vault_expression_test.ts: update 7 existing tests to match new escaping and add 2 new injection-prevention tests - integration/vault_cel_integration_test.ts: update special-characters integration test to expect \$ in the CEL-evaluated value - design/expressions.md: document the shell safety escaping contract --- design/expressions.md | 21 +++++++++ integration/vault_cel_integration_test.ts | 9 ++-- src/domain/expressions/model_resolver.ts | 8 +++- src/domain/vaults/vault_expression_test.ts | 55 +++++++++++++++++----- 4 files changed, 75 insertions(+), 18 deletions(-) diff --git a/design/expressions.md b/design/expressions.md index 20ef0b9..6f7ada3 100644 --- a/design/expressions.md +++ b/design/expressions.md @@ -165,6 +165,27 @@ attributes: keyData: ${{ vault.get(aws, machineKeyData) }} ``` +### Shell Safety + +When vault secrets are injected into the `run` field of a `command/shell` model via CEL +string concatenation, shell metacharacters in the secret value are automatically escaped so +that the value is always treated as **literal data**, never as shell syntax. + +Specifically, `$` and `` ` `` are escaped so that `$(cmd)` and `` `cmd` `` in a secret +value do not trigger command substitution: + +```yaml +# Secret value: $(cat /etc/passwd) +# Shell receives: \$(cat /etc/passwd) → outputs literally: $(cat /etc/passwd) +attributes: + run: '"echo " + vault.get(''my-vault'', ''SECRET'') + '' done''' +``` + +This means: +- `$VAR_NAME` in a secret is **not** expanded as a shell variable — it appears literally. +- `$(cmd)` and `` `cmd` `` in a secret are **not** executed — they appear literally. +- Prices, connection strings, and other data containing `$` are safe to store and use. + ## Environment Variables All process environment variables are available in CEL expressions via the `env` diff --git a/integration/vault_cel_integration_test.ts b/integration/vault_cel_integration_test.ts index 86a2dac..26600d1 100644 --- a/integration/vault_cel_integration_test.ts +++ b/integration/vault_cel_integration_test.ts @@ -726,9 +726,12 @@ Deno.test("Vault CEL: handles secrets with special characters", async () => { ]); // Store secret with special characters - // Note: Avoiding characters that require escaping in CEL strings - // (backslashes, quotes, newlines) as these are escaped for CEL parsing + // Note: $ and ` are escaped with a \\ prefix so that after CEL evaluation + // the shell receives \$ and \` (literal characters, no command substitution). + // Backslashes, quotes, and newlines are also escaped for CEL string safety. const specialSecret = "p@ssw0rd!#$%^&*()_+-=[]{}|;:.<>?/"; + // After escaping, $ becomes \$ so the resolved CEL value includes the backslash + const expectedResolved = "p@ssw0rd!#\\$%^&*()_+-=[]{}|;:.<>?/"; const vaultServiceForPut = await VaultService.fromRepository(repoDir); await vaultServiceForPut.put( "special-chars-vault", @@ -760,7 +763,7 @@ Deno.test("Vault CEL: handles secrets with special characters", async () => { result.definition, ); - assertEquals(resolved.globalArguments.password, specialSecret); + assertEquals(resolved.globalArguments.password, expectedResolved); }); }); diff --git a/src/domain/expressions/model_resolver.ts b/src/domain/expressions/model_resolver.ts index 4bd4b2e..e95bc5d 100644 --- a/src/domain/expressions/model_resolver.ts +++ b/src/domain/expressions/model_resolver.ts @@ -788,12 +788,16 @@ export class ModelResolver { const [fullMatch, , vaultName, , secretKey] = match; try { const secretValue = await vaultService.get(vaultName, secretKey); - // Escape special characters to prevent CEL parsing issues and injection attacks + // Escape special characters to prevent CEL parsing issues and injection attacks. + // For $ and `, we use a \\X prefix (two backslashes + char) so that: + // - CEL sees \\ → produces one \, then the char is a plain literal + // - Shell receives \$ or \` → literal character, no command substitution const escapedValue = secretValue .replace(/\\/g, "\\\\") + .replace(/\$/g, "\\\\$") // $ → \\$ so CEL produces \$, shell treats as literal $ + .replace(/`/g, "\\\\`") // ` → \\` so CEL produces \`, shell treats as literal ` .replace(/"/g, '\\"') .replace(/'/g, "\\'") - .replace(/`/g, "\\`") .replace(/\n/g, "\\n") .replace(/\r/g, "\\r") .replace(/\t/g, "\\t"); diff --git a/src/domain/vaults/vault_expression_test.ts b/src/domain/vaults/vault_expression_test.ts index a8634c9..4ce19b8 100644 --- a/src/domain/vaults/vault_expression_test.ts +++ b/src/domain/vaults/vault_expression_test.ts @@ -199,7 +199,7 @@ Deno.test("ModelResolver.resolveVaultExpressions", async (t) => { const result = await resolver.resolveVaultExpressions( "vault.get(test-vault, backtick-secret)", ); - assertEquals(result, '"value\\`with\\`backticks"'); + assertEquals(result, '"value\\\\`with\\\\`backticks"'); }); await t.step( @@ -255,7 +255,7 @@ Deno.test("ModelResolver.resolveVaultExpressions", async (t) => { }); await t.step( - "should preserve $& pattern in secret values", + "should escape $ in secret values to prevent shell variable expansion", async () => { const resolver = createResolverWithMockVault({ "dollar-amp": "my$&secret", @@ -263,12 +263,12 @@ Deno.test("ModelResolver.resolveVaultExpressions", async (t) => { const result = await resolver.resolveVaultExpressions( "vault.get(test-vault, dollar-amp)", ); - assertEquals(result, '"my$&secret"'); + assertEquals(result, '"my\\\\$&secret"'); }, ); await t.step( - "should preserve $` pattern in secret values", + "should escape $` pattern in secret values", async () => { const resolver = createResolverWithMockVault({ "dollar-backtick": "prefix$`suffix", @@ -276,12 +276,12 @@ Deno.test("ModelResolver.resolveVaultExpressions", async (t) => { const result = await resolver.resolveVaultExpressions( "vault.get(test-vault, dollar-backtick)", ); - assertEquals(result, '"prefix$\\`suffix"'); + assertEquals(result, '"prefix\\\\$\\\\`suffix"'); }, ); await t.step( - "should preserve $' pattern in secret values", + "should escape $' pattern in secret values", async () => { const resolver = createResolverWithMockVault({ "dollar-quote": "prefix$'suffix", @@ -289,12 +289,12 @@ Deno.test("ModelResolver.resolveVaultExpressions", async (t) => { const result = await resolver.resolveVaultExpressions( "vault.get(test-vault, dollar-quote)", ); - assertEquals(result, '"prefix$\\\'suffix"'); + assertEquals(result, '"prefix\\\\$\\\'suffix"'); }, ); await t.step( - "should preserve $$ pattern in secret values", + "should escape $$ pattern in secret values", async () => { const resolver = createResolverWithMockVault({ "dollar-dollar": "cost: $$100", @@ -302,12 +302,12 @@ Deno.test("ModelResolver.resolveVaultExpressions", async (t) => { const result = await resolver.resolveVaultExpressions( "vault.get(test-vault, dollar-dollar)", ); - assertEquals(result, '"cost: $$100"'); + assertEquals(result, '"cost: \\\\$\\\\$100"'); }, ); await t.step( - "should preserve numbered capture group patterns in secret values", + "should escape numbered dollar patterns in secret values", async () => { const resolver = createResolverWithMockVault({ "dollar-numbers": "$1$2$3", @@ -315,12 +315,12 @@ Deno.test("ModelResolver.resolveVaultExpressions", async (t) => { const result = await resolver.resolveVaultExpressions( "vault.get(test-vault, dollar-numbers)", ); - assertEquals(result, '"$1$2$3"'); + assertEquals(result, '"\\\\$1\\\\$2\\\\$3"'); }, ); await t.step( - "should preserve multiple dollar patterns in same secret", + "should escape multiple dollar and backtick patterns in same secret", async () => { const resolver = createResolverWithMockVault({ "multi-dollar": "a]$&b$`c$'d$$e$1f", @@ -328,7 +328,36 @@ Deno.test("ModelResolver.resolveVaultExpressions", async (t) => { const result = await resolver.resolveVaultExpressions( "vault.get(test-vault, multi-dollar)", ); - assertEquals(result, '"a]$&b$\\`c$\\\'d$$e$1f"'); + assertEquals( + result, + '"a]\\\\$&b\\\\$\\\\`c\\\\$\\\'d\\\\$\\\\$e\\\\$1f"', + ); + }, + ); + + 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)", + ); + assertEquals(result, '"\\\\$(echo injected)"'); + }, + ); + + await t.step( + "should escape backticks to prevent shell command substitution", + async () => { + const resolver = createResolverWithMockVault({ + "backtick-cmd": "`echo injected`", + }); + const result = await resolver.resolveVaultExpressions( + "vault.get(test-vault, backtick-cmd)", + ); + assertEquals(result, '"\\\\`echo injected\\\\`"'); }, );