diff --git a/cmd/nerd/cmd_advanced.go b/cmd/nerd/cmd_advanced.go index 4c0251bb..95454cb4 100644 --- a/cmd/nerd/cmd_advanced.go +++ b/cmd/nerd/cmd_advanced.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "os/exec" + "path/filepath" "strings" "time" @@ -454,8 +455,35 @@ func runToolCommand(cmd *cobra.Command, args []string) error { return fmt.Errorf("tool '%s' not found. Run 'nerd tool list' to see available tools", toolName) } + // SECURITY FIX: Prevent command injection and path traversal + // Ensure the binary is executed from the .nerd/tools/.compiled directory + ws := workspace + if ws == "" { + ws, _ = os.Getwd() + } + expectedDir := filepath.Join(ws, ".nerd", "tools", ".compiled") + + absExpectedDir, err := filepath.Abs(expectedDir) + if err != nil { + return fmt.Errorf("failed to resolve expected tools directory: %w", err) + } + + absBinaryPath, err := filepath.Abs(binaryPath) + if err != nil { + return fmt.Errorf("failed to resolve binary path: %w", err) + } + + relPath, err := filepath.Rel(absExpectedDir, absBinaryPath) + if err != nil || strings.HasPrefix(relPath, "..") || relPath == ".." || filepath.IsAbs(relPath) { + return fmt.Errorf("security error: binary path '%s' is outside expected directory '%s'", binaryPath, expectedDir) + } + + if _, err := os.Stat(absBinaryPath); os.IsNotExist(err) { + return fmt.Errorf("tool binary not found at '%s'", absBinaryPath) + } + // Execute tool binary directly - toolCmd := exec.CommandContext(ctx, binaryPath) + toolCmd := exec.CommandContext(ctx, absBinaryPath) if toolInput != "" { toolCmd.Stdin = strings.NewReader(toolInput) } diff --git a/pr_description.txt b/pr_description.txt index b4306963..a9d9df18 100644 --- a/pr_description.txt +++ b/pr_description.txt @@ -1,11 +1,6 @@ -**Title**: ⚡ Optimize AtomLoader.ReplaceAtoms with batch inserts to fix N+1 issue +**Title**: 🔒 Fix potential command injection/path traversal in nerd tool run **Description**: -💡 **What:** Refactored `AtomLoader.ReplaceAtoms` to use a true chunked batch insert approach (using `INSERT INTO ... VALUES (...), (...), ...`) and moved embedding generation out of the database transaction. - -🎯 **Why:** The previous implementation called `storeAtomTx` within a loop over atoms inside a database transaction. This resulted in an N+1 query problem, parsing and executing `INSERT` and redundant `DELETE` statements on a per-atom basis. Also, holding an open transaction while waiting on external network calls for embeddings generation is a significant anti-pattern that can lead to exhausted database connections and locks. - -📊 **Measured Improvement:** -Before optimization, replacing 10,000 atoms took about 700ms, and 1,000 took about 84ms. -After optimization using bulk chunk inserts, 1,000 atoms took about 23ms and 10,000 atoms took roughly 199ms. -This represents a solid 3.5x to 3.6x measured performance improvement, alongside drastically improving the transaction health by removing network dependencies inside locks. +🎯 **What**: Prevented unauthorized execution by validating the tool binary path before running it in `cmd/nerd/cmd_advanced.go`. +⚠️ **Risk**: The previous code pulled `binaryPath` from a Mangle kernel fact `tool_binary_path` without checking if the binary actually belonged to the internal compiled tools directory. If an attacker managed to assert a malicious `tool_binary_path` (e.g. `../../../../bin/bash`), they could execute arbitrary binaries with the same privileges as the agent. +🛡️ **Solution**: Ensure absolute path resolution for both the target binary and the expected directory (`.nerd/tools/.compiled`). Use `filepath.Rel` to explicitly verify that the binary resides strictly within the compiled directory, effectively preventing path traversal escapes and executing untrusted/unexpected binaries on the host system.