Merge pull request #29 from NVIDIA-NeMo/security/shelltools-path-containment

Contain ShellTools file operations within cwd

Author: rdasilveiracabral

security/shelltools-file-operation-path-containment/README.md

@@ -0,0 +1,27 @@
+# ShellTools File-Operation Path Containment
+
+## Security issue
+
+`ShellTools.read()`, both forms of `replace()`, and `write_file()` resolved
+user-controlled paths but did not verify that the result remained beneath
+`ShellTools.cwd`. Absolute paths and `..` components could therefore read or
+modify files elsewhere on the host. Match harvesting used the same unsafe
+resolution when creating editable file matches.
+
+## Patch
+
+All non-shell filesystem access now goes through a shared `_resolve_path()`
+helper. It resolves both the working directory and candidate path, then requires
+`resolved.relative_to(cwd)` to succeed before any file is opened, created, or
+modified. A path outside the current working directory raises `ValueError`.
+
+Normal relative paths, nested directories, overwrites, and both existing
+`replace()` forms retain their behavior. Match harvesting fails closed and
+attaches no editable matches when a reported path is outside the working
+directory.
+
+## Boundary
+
+This change confines the explicit Python file-operation methods. `run()` is an
+intentional shell capability and must be separately sandboxed or withheld when
+arbitrary shell commands are outside the deployment's trust model.

src/nooa/tools/shell_tools.py

@@ -339,6 +339,16 @@ async def close(self) -> None:
         """Terminate the underlying bash session owned by this shell."""
         await self._session.close()
 
+    def _resolve_path(self, path: str) -> Path:
+        """Resolve a file-operation path and require it to remain inside cwd."""
+        root = self.cwd.resolve()
+        resolved = (root / path).resolve()
+        try:
+            resolved.relative_to(root)
+        except ValueError as exc:
+            raise ValueError(f"path escapes ShellTools cwd: {path}") from exc
+        return resolved
+
     async def run(
         self,
         command: Annotated[str, spec(description="Shell command to execute")],
@@ -538,10 +548,10 @@ async def _harvest_matches(self, command: str, displayed_stdout: str) -> list[Ma
         out: list[Match] = []
         for mpath, line_no in keep:
             if mpath not in file_cache:
-                resolved = (self.cwd / mpath).resolve()
                 try:
+                    resolved = self._resolve_path(mpath)
                     file_cache[mpath] = resolved.read_text().splitlines(keepends=True)
-                except OSError:
+                except (OSError, ValueError):
                     return None
             lines = file_cache[mpath]
             if not (1 <= line_no <= len(lines)):
@@ -656,7 +666,7 @@ async def read(
         Returns:
             Match with .text, .numbered, .path, .start, .end.
         """
-        resolved = (self.cwd / path).resolve()
+        resolved = self._resolve_path(path)
         content = resolved.read_text()
         all_lines = content.splitlines(keepends=True)
         total = len(all_lines)
@@ -698,7 +708,7 @@ async def replace(
         """
         if isinstance(target, Match):
             new_text = old_or_new
-            resolved = (self.cwd / target.path).resolve()
+            resolved = self._resolve_path(target.path)
             content = resolved.read_text()
             all_lines = content.splitlines(keepends=True)
 
@@ -724,7 +734,7 @@ async def replace(
                     "Did you mean replace(match, new_text)?"
                 )
             old_text = old_or_new
-            resolved = (self.cwd / target).resolve()
+            resolved = self._resolve_path(target)
             content = resolved.read_text()
 
             count = content.count(old_text)
@@ -762,7 +772,7 @@ async def write_file(
             path: File path (relative to cwd).
             content: Full file content.
         """
-        resolved = (self.cwd / path).resolve()
+        resolved = self._resolve_path(path)
         resolved.parent.mkdir(parents=True, exist_ok=True)
         resolved.write_text(content)
         line_count = content.count("\n") + (1 if content and not content.endswith("\n") else 0)

tests/tools/test_shell_tools_modern_behavior.py

@@ -9,7 +9,7 @@
 
 import pytest
 
-from nooa.tools.shell_tools import ShellTools
+from nooa.tools.shell_tools import Match, ShellTools
 
 
 @pytest.fixture
@@ -71,6 +71,23 @@ async def test_write_file_is_overwrite(sh, tmp_path):
     assert (tmp_path / "f.txt").read_text() == "new"
 
 
+@pytest.mark.asyncio
+async def test_file_operations_reject_paths_outside_cwd(sh, tmp_path):
+    outside = tmp_path.parent / f"{tmp_path.name}-outside.txt"
+    outside.write_text("secret")
+
+    with pytest.raises(ValueError, match="escapes ShellTools cwd"):
+        await sh.read(f"../{outside.name}")
+    with pytest.raises(ValueError, match="escapes ShellTools cwd"):
+        await sh.replace(f"../{outside.name}", "secret", "changed")
+    with pytest.raises(ValueError, match="escapes ShellTools cwd"):
+        await sh.replace(Match(f"../{outside.name}", 1, 1, "secret"), "changed")
+    with pytest.raises(ValueError, match="escapes ShellTools cwd"):
+        await sh.write_file(str(outside), "overwritten")
+
+    assert outside.read_text() == "secret"
+
+
 @pytest.mark.asyncio
 async def test_close_terminates_underlying_bash_session(sh):
     """Verify close() terminates BashSession and the shell lazily restarts."""