Skip to content

Fix path traversal bypass in ResolveLocalPath sandbox#9726

Open
nishantmonu51 wants to merge 1 commit into
mainfrom
nishantmonu51/fix-resolvelocalpath-traversal
Open

Fix path traversal bypass in ResolveLocalPath sandbox#9726
nishantmonu51 wants to merge 1 commit into
mainfrom
nishantmonu51/fix-resolvelocalpath-traversal

Conversation

@nishantmonu51

Copy link
Copy Markdown
Collaborator

ResolveLocalPath is used as the file/DuckDB sandbox in hosted deployments (via AllowHostAccess=false). Two defects let a project-supplied path escape the repo root:

  • Absolute inputs were returned without filepath.Clean, so .. segments survived (filepath.Join, which cleans, only ran for relative paths). path: /repo/../../etc/passwd passed HasPrefix("/repo/../../etc/passwd", "/repo") and resolved to /etc/passwd.
  • The prefix check had no trailing separator, so a sibling directory sharing the root's prefix passed. With root /data/proj123, path: ../proj123-x/secret cleaned to /data/proj123-x/secret and slipped through — a potential cross-tenant read.

Fix:

  • filepath.Clean the resolved path for both the absolute and relative branches before the sandbox check.
  • Replace strings.HasPrefix(finalPath, root) with finalPath == root || strings.HasPrefix(finalPath, root+separator), cleaning root first.
  • Added TestResolveLocalPath covering absolute/relative traversal, both sibling-prefix variants, in-root paths, and the allowHostAccess=true passthrough.

Reproducing steps:

  1. Configure a duckdb or local file connector with allow_host_access: false (the state hosted Rill Cloud runs the sandbox in).
  2. Create a source/model whose path is an absolute path containing .., e.g. path: /<project_root>/../../etc/hosts.
  3. Reconcile/ingest the source. Before this change the file resolves outside the project root and is read; after it, the request is rejected with "path is outside repo root".
  4. Alternatively, with project root /data/proj123, point path at ../proj123-x/<file> and observe it previously resolved to a sibling tenant directory that still passed the prefix check.

In code, the regression is captured by:

  • ResolveLocalPath("/root/../etc/x", "/root", false) — now returns an error.
  • ResolveLocalPath("../root-x/secret", "/root", false) — now returns an error.

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!

Clean the resolved path for both absolute and relative inputs, and use a
separator-aware prefix check so sibling directories sharing the root's
prefix no longer pass the sandbox.

Claude-Session: https://claude.ai/code/session_01XWGRxG2FDSw2NgJEn7NmeE
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant