diff --git a/cmd/generate_changelog/incoming/2152.txt b/cmd/generate_changelog/incoming/2152.txt new file mode 100644 index 00000000..5808eccf --- /dev/null +++ b/cmd/generate_changelog/incoming/2152.txt @@ -0,0 +1,3 @@ +### PR [#2152](https://github.com/danielmiessler/Fabric/pull/2152) by [AUTHENSOR](https://github.com/AUTHENSOR): fix: shell-escape extension values to prevent command injection + +- **Fix:** Shell-escape extension values to prevent command injection in the extension executor, which previously ran commands via `sh -c` with unescaped, user-controlled values interpolated into the command string. All user-controlled values are now wrapped in single quotes with embedded-single-quote escaping prior to interpolation, ensuring the shell treats them as literal arguments. A regression test (`ShellInjectionBlocked`) has been added to verify that malicious input (e.g., `hello; touch /marker`) does not execute unintended shell commands. diff --git a/internal/plugins/template/extension_executor.go b/internal/plugins/template/extension_executor.go index de4d6895..5d4073d2 100644 --- a/internal/plugins/template/extension_executor.go +++ b/internal/plugins/template/extension_executor.go @@ -78,20 +78,32 @@ func (e *ExtensionExecutor) formatCommand(ext *ExtensionDefinition, operation st return "", fmt.Errorf("%s", fmt.Sprintf(i18n.T("extension_operation_not_found"), operation, ext.Name)) } + // Shell-escape all user-controlled values to prevent command injection. + // The command string is ultimately passed to "sh -c", so any shell + // metacharacters (;, |, $(), backticks, etc.) in the value would be + // executed. Wrapping each value in single quotes and escaping embedded + // single quotes ensures the value is treated as a literal argument. vars := make(map[string]string) vars["executable"] = ext.Executable vars["operation"] = operation - vars["value"] = value + vars["value"] = shellEscape(value) // Split on pipe for numbered variables values := strings.Split(value, "|") for i, val := range values { - vars[fmt.Sprintf("%d", i+1)] = val + vars[fmt.Sprintf("%d", i+1)] = shellEscape(val) } return ApplyTemplate(opConfig.CmdTemplate, vars, "") } +// shellEscape wraps a string in single quotes for safe use in a shell command, +// escaping any embedded single quotes. This prevents command injection when +// untrusted input is passed as an argument to "sh -c". +func shellEscape(s string) string { + return "'" + strings.ReplaceAll(s, "'", "'\"'\"'") + "'" +} + // executeStdout runs the command and captures its stdout func (e *ExtensionExecutor) executeStdout(cmd *exec.Cmd, ext *ExtensionDefinition) (string, error) { var stdout bytes.Buffer diff --git a/internal/plugins/template/extension_executor_test.go b/internal/plugins/template/extension_executor_test.go index 44940828..ee9774ec 100644 --- a/internal/plugins/template/extension_executor_test.go +++ b/internal/plugins/template/extension_executor_test.go @@ -21,6 +21,9 @@ case "$1" in "stdout") echo "Hello, $2!" ;; + "echo") + echo "$2" + ;; "file") echo "Hello, $2!" > "$3" echo "$3" # Print the filename for path_from_stdout @@ -39,6 +42,54 @@ esac` registry := NewExtensionRegistry(tmpDir) executor := NewExtensionExecutor(registry) + // Test that shell metacharacters in user input are neutralized. + // Before the fix, value flowed unescaped into "sh -c", so input + // like "; touch /tmp/pwned" would execute arbitrary commands. + t.Run("ShellInjectionBlocked", func(t *testing.T) { + // Use a marker file to detect if injection succeeded. + markerFile := filepath.Join(tmpDir, "injection-marker") + _ = os.Remove(markerFile) + + configPath := filepath.Join(tmpDir, "inject-test.yaml") + configContent := `name: inject-test +executable: ` + testScript + ` +type: executable +timeout: 5s +operations: + echo: + cmd_template: "{{executable}} echo {{value}}" +config: + output: + method: stdout` + + if err := os.WriteFile(configPath, []byte(configContent), 0644); err != nil { + t.Fatalf("Failed to create config: %v", err) + } + + if err := registry.Register(configPath); err != nil { + t.Fatalf("Failed to register extension: %v", err) + } + + // Malicious input: attempt to run a separate command after echo. + maliciousValue := "hello; touch " + markerFile + + output, err := executor.Execute("inject-test", "echo", maliciousValue) + if err != nil { + t.Fatalf("Execute failed: %v", err) + } + + // The output should contain the full malicious string as a literal + // argument (proving the shell did NOT interpret the semicolon). + if !strings.Contains(output, "hello; touch") { + t.Errorf("Expected literal value in output, got: %q", output) + } + + // The marker file must NOT exist (injection was blocked). + if _, err := os.Stat(markerFile); !os.IsNotExist(err) { + t.Error("SECURITY: command injection succeeded — marker file was created") + } + }) + // Test stdout-based extension t.Run("StdoutExecution", func(t *testing.T) { configPath := filepath.Join(tmpDir, "stdout-extension.yaml")