diff --git a/internal/tools/builtin.go b/internal/tools/builtin.go index c59a578e80601cea197a42ca8e4e89e9dc5b4011..8c894a8404acec48961e18eba3094075eda47af0 100644 --- a/internal/tools/builtin.go +++ b/internal/tools/builtin.go @@ -267,6 +267,7 @@ func execDirect(ctx context.Context, command, workDir string, timeout int) (stri // Build a hardened command with extglob disabled executor := NewSecureShellExecutor("bash") hardenedCmd := executor.BuildCommand(command) + defer executor.Cleanup() // remove the temp file holding the user command (IK4RN2) // Create a new timeout context specifically for this execution. // We use a raw exec.Command (not CommandContext) so we can control diff --git a/internal/tools/secure_exec.go b/internal/tools/secure_exec.go index 27075379cfd41ee5c655bfc4b8951ad7d927b984..f2cf432c6089f77d699d27e5c4f15fa4d6946b23 100644 --- a/internal/tools/secure_exec.go +++ b/internal/tools/secure_exec.go @@ -12,7 +12,7 @@ import ( // It applies multiple layers of protection: // - Environment snapshot restoration (optional) // - extglob/extended_glob disable to prevent malicious filename expansion -// - eval wrapping to ensure aliases work after sourcing +// - User command executed via a temp-file source (avoids eval double-parse) // - Working directory tracking via pwd -P type SecureShellExecutor struct { // ShellType is the detected shell type: "bash", "zsh", "fish", "posix" @@ -29,6 +29,12 @@ type SecureShellExecutor struct { // WorkDir is the working directory for command execution. WorkDir string + + // cmdFile holds the path of the temp file written by BuildCommand to + // hold the user command. Cleanup() removes it. Empty until BuildCommand + // is called (or if temp-file creation failed and the eval fallback was + // used). + cmdFile string } // NewSecureShellExecutor creates a new SecureShellExecutor with the given shell type. @@ -38,10 +44,21 @@ func NewSecureShellExecutor(shellType string) *SecureShellExecutor { } } +// Cleanup removes the temp file created by BuildCommand to hold the user +// command. It is safe to call multiple times and when no temp file was +// created. Callers of BuildCommand should defer this. +func (s *SecureShellExecutor) Cleanup() { + if s.cmdFile != "" { + _ = os.Remove(s.cmdFile) + s.cmdFile = "" + } +} + // BuildCommand wraps a user command with security hardening measures: // 1. Source environment snapshot (optional) // 2. Disable extglob / extended_glob (security: prevent malicious filename attacks) -// 3. Wrap user command in eval (so aliases work after sourcing) +// 3. Execute user command by writing it to a temp file and sourcing it +// (avoids eval double-parse injection risk, IK4RN2) // 4. Track working directory changes via pwd -P (optional) func (s *SecureShellExecutor) BuildCommand(userCommand string) string { var parts []string @@ -60,8 +77,20 @@ func (s *SecureShellExecutor) BuildCommand(userCommand string) string { parts = append(parts, disableCmd) } - // 3. Execute user command with eval (makes aliases work after source) - parts = append(parts, fmt.Sprintf("eval %s", ShellQuote(userCommand))) + // 3. Execute user command. + // + // Previously this used `eval ShellQuote(userCommand)`, which performs a + // double parse: bash -c parses the compound (the single-quoted string is + // treated as a literal), then eval re-parses it as a shell command. If + // ShellQuote had any edge-case bug, eval could be exploited to break out + // of the quoting (IK4RN2). + // + // We now write the user command to a temp file and `source` it. Sourcing + // executes the file contents in the current shell (so aliases and sourced + // env from the snapshot still apply) without the double-parse risk. The + // temp file is created with O_NOFOLLOW (SafeCreateTemp) and 0600 perms. + cmdPart := s.buildUserCommandPart(userCommand) + parts = append(parts, cmdPart) // 4. Track working directory changes if s.CWDTrackingFile != "" { @@ -72,6 +101,34 @@ func (s *SecureShellExecutor) BuildCommand(userCommand string) string { return strings.Join(parts, " && ") } +// buildUserCommandPart returns the shell fragment that executes the user +// command. It prefers the temp-file+source path (no eval); if temp-file +// creation fails it falls back to eval ShellQuote so execution is not +// silently broken. +func (s *SecureShellExecutor) buildUserCommandPart(userCommand string) string { + f, err := SafeCreateTemp("", "ocai-cmd-*.sh") + if err != nil { + // Fallback: eval with ShellQuote. This is the pre-fix behavior and + // is still safe as long as ShellQuote is correct, but we prefer the + // temp-file path to eliminate the double-parse entirely. + return fmt.Sprintf("eval %s", ShellQuote(userCommand)) + } + if _, err := f.WriteString(userCommand); err != nil { + f.Close() + os.Remove(f.Name()) + return fmt.Sprintf("eval %s", ShellQuote(userCommand)) + } + if err := f.Close(); err != nil { + os.Remove(f.Name()) + return fmt.Sprintf("eval %s", ShellQuote(userCommand)) + } + // Restrict permissions (SafeCreateTemp already uses 0600 via CreateTemp, + // but be explicit for defense in depth). + _ = os.Chmod(f.Name(), 0600) + s.cmdFile = f.Name() + return fmt.Sprintf("source %s", ShellQuote(f.Name())) +} + // DisableExtglob returns the shell-specific command to disable extended globbing. // This prevents malicious filename patterns like ?(cmd), *(cmd), etc. func DisableExtglob(shellType string) string { diff --git a/internal/tools/secure_exec_test.go b/internal/tools/secure_exec_test.go index 5378424ca3e673efba2f485df7d3e8e139deeb8a..7f94d731eca20fdbbff37c9cc94ac7554049d6d2 100644 --- a/internal/tools/secure_exec_test.go +++ b/internal/tools/secure_exec_test.go @@ -16,26 +16,38 @@ import ( func TestSecureShellExecutor_BasicCommand(t *testing.T) { executor := NewSecureShellExecutor("bash") cmd := executor.BuildCommand("echo hello") + defer executor.Cleanup() // Should contain extglob disable if !strings.Contains(cmd, "shopt -u extglob") { t.Error("bash command should disable extglob") } - // Should contain eval-wrapped command - if !strings.Contains(cmd, "eval") { - t.Error("command should be eval-wrapped") + // User command is now executed via temp-file source (IK4RN2), not eval. + if strings.Contains(cmd, "eval ") { + t.Error("command should NOT use eval (temp-file source expected)") + } + if !strings.Contains(cmd, "source ") { + t.Error("command should source the temp file holding the user command") } - // Should contain the original command (quoted) - if !strings.Contains(cmd, "echo hello") { - t.Error("command should contain the original command") + // The temp file should contain the original command. + if executor.cmdFile == "" { + t.Fatal("cmdFile should be set after BuildCommand") + } + data, err := os.ReadFile(executor.cmdFile) + if err != nil { + t.Fatalf("failed to read cmdFile: %v", err) + } + if !strings.Contains(string(data), "echo hello") { + t.Errorf("cmdFile should contain the user command, got: %s", string(data)) } } func TestSecureShellExecutor_ZshExtglob(t *testing.T) { executor := NewSecureShellExecutor("zsh") cmd := executor.BuildCommand("ls -la") + defer executor.Cleanup() if !strings.Contains(cmd, "setopt NO_EXTENDED_GLOB") { t.Error("zsh command should disable extended glob") @@ -45,6 +57,7 @@ func TestSecureShellExecutor_ZshExtglob(t *testing.T) { func TestSecureShellExecutor_POSIXNoExtglob(t *testing.T) { executor := NewSecureShellExecutor("posix") cmd := executor.BuildCommand("ls -la") + defer executor.Cleanup() if strings.Contains(cmd, "extglob") || strings.Contains(cmd, "EXTENDED_GLOB") { t.Error("posix command should not contain extglob directives") @@ -54,6 +67,7 @@ func TestSecureShellExecutor_POSIXNoExtglob(t *testing.T) { func TestSecureShellExecutor_FishNoExtglob(t *testing.T) { executor := NewSecureShellExecutor("fish") cmd := executor.BuildCommand("ls -la") + defer executor.Cleanup() if strings.Contains(cmd, "extglob") || strings.Contains(cmd, "EXTENDED_GLOB") { t.Error("fish command should not contain extglob directives") @@ -64,6 +78,7 @@ func TestSecureShellExecutor_WithSnapshot(t *testing.T) { executor := NewSecureShellExecutor("bash") executor.SnapshotPath = "/tmp/env-snapshot.sh" cmd := executor.BuildCommand("echo test") + defer executor.Cleanup() if !strings.Contains(cmd, "source '/tmp/env-snapshot.sh'") { t.Errorf("command should source snapshot, got: %s", cmd) @@ -80,6 +95,7 @@ func TestSecureShellExecutor_WithCWDTracking(t *testing.T) { executor := NewSecureShellExecutor("bash") executor.CWDTrackingFile = "/tmp/cwd-track" cmd := executor.BuildCommand("cd /tmp && ls") + defer executor.Cleanup() if !strings.Contains(cmd, "pwd -P >|") { t.Error("command should track cwd changes") @@ -94,22 +110,27 @@ func TestSecureShellExecutor_WithAllOptions(t *testing.T) { executor.SnapshotPath = "/tmp/snapshot.sh" executor.CWDTrackingFile = "/tmp/cwd" cmd := executor.BuildCommand("make build") + defer executor.Cleanup() - // Should have 4 parts: source, extglob, eval, pwd + // Should have 4 parts: source(snapshot), extglob, source(cmdfile), pwd parts := strings.Split(cmd, " && ") if len(parts) != 4 { t.Errorf("expected 4 parts, got %d: %v", len(parts), parts) } - // Order: source → extglob → eval → pwd + // Order: source(snapshot) → extglob → source(cmdfile) → pwd if !strings.HasPrefix(parts[0], "source") { - t.Errorf("part 0 should be source, got: %s", parts[0]) + t.Errorf("part 0 should be source (snapshot), got: %s", parts[0]) } if !strings.Contains(parts[1], "extglob") { t.Errorf("part 1 should be extglob, got: %s", parts[1]) } - if !strings.HasPrefix(parts[2], "eval") { - t.Errorf("part 2 should be eval, got: %s", parts[2]) + // User command is now sourced from a temp file, not eval'd (IK4RN2). + if !strings.HasPrefix(parts[2], "source") { + t.Errorf("part 2 should be source (cmd file), got: %s", parts[2]) + } + if strings.HasPrefix(parts[2], "eval") { + t.Errorf("part 2 should NOT use eval, got: %s", parts[2]) } if !strings.HasPrefix(parts[3], "pwd") { t.Errorf("part 3 should be pwd, got: %s", parts[3]) @@ -117,40 +138,45 @@ func TestSecureShellExecutor_WithAllOptions(t *testing.T) { } func TestSecureShellExecutor_QuotingPreservation(t *testing.T) { - executor := NewSecureShellExecutor("bash") - tests := []struct { name string command string - wantIn string }{ { name: "simple command", command: "echo hello", - wantIn: "'echo hello'", }, { name: "single quotes in command", command: "echo 'hello world'", - wantIn: "'echo '\\''hello world'\\'''", }, { name: "double quotes in command", command: `echo "hello world"`, - wantIn: `'echo "hello world"'`, }, { name: "special chars", command: "ls -la | grep test", - wantIn: "'ls -la | grep test'", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + executor := NewSecureShellExecutor("bash") cmd := executor.BuildCommand(tt.command) - if !strings.Contains(cmd, tt.wantIn) { - t.Errorf("expected %q in result, got: %s", tt.wantIn, cmd) + defer executor.Cleanup() + // The user command is written verbatim to the temp file (no + // shell quoting/escaping of the command body itself), so the + // temp file content must equal the original command exactly. + if executor.cmdFile == "" { + t.Fatalf("cmdFile should be set, cmd=%s", cmd) + } + data, err := os.ReadFile(executor.cmdFile) + if err != nil { + t.Fatalf("failed to read cmdFile: %v", err) + } + if string(data) != tt.command { + t.Errorf("cmdFile content = %q, want %q", string(data), tt.command) } }) }