Skip to content

fix(policyfile): keep a repository source path inside the clone - #183

Open
tas50 wants to merge 1 commit into
mainfrom
fix/policyfile-git-path-traversal
Open

fix(policyfile): keep a repository source path inside the clone#183
tas50 wants to merge 1 commit into
mainfrom
fix/policyfile-git-path-traversal

Conversation

@tas50

@tas50 tas50 commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

What this is (and is not)

This is hardening, not a live vulnerability fix. I want to be straight about that up front, because I initially reported it as exploitable and was wrong.

fetchGit joins the lock's path sub-option onto the clone directory with no containment check:

root = filepath.Join(clone, sub)   // sub comes from Policyfile.lock.json

Every other lock-derived path in this file goes through safeJoin — the cache key, the cookbook name, the identifier. This one does not, and a path of "../.." resolves anywhere on the machine. copyTree would then pull whatever sits there into the cookbook cache, and cinc policy push would send it to the server.

Why it is not reachable today: EnsureCookbook is the only caller, and it dispatches on cinc-api's CookbookLock.Origin(), which prefers the path key over the repository key:

for _, k := range []SourceKind{SourcePath, SourceArtifactserver, SourceGit, SourceChefServer} {

So a lock carrying both keys is classified as a path source and never reaches the fetch function under test. My original probe called that function directly and skipped this dispatch entirely.

Why fix it anyway

The join is currently protected only by key precedence in another module. Reorder those keys upstream, or add a second caller, and the guard silently disappears. A containment check belongs next to the join it protects, not in a dependency's iteration order.

The test shows the escape is otherwise real. On main the path resolves outside the clone and is stopped only by the metadata.rb existence check, which an attacker controlling the lock can satisfy:

error = no cookbook (metadata.rb or metadata.json) found at "/var/folders/.../T/outside"
                                                             ^^^ outside the clone

withinDir rather than safeJoin, because nested paths are legitimate here (the cookbooks/<name> monorepo layout); only escaping the clone is not.

Tests

  • TestFetchGitRejectsPathEscapingTheClone covers ../outside and nested/../../outside. Both fail on main.
  • TestFetchGitAcceptsNestedPath confirms the monorepo layout still resolves, so the guard is not over-tight.

Both target the fetch function directly, since that is the unit carrying the defect.

Related, not fixed here

That same Origin() precedence means a monorepo lock carrying both keys is silently treated as a local path source, which looks like a bug in its own right. It lives in cinc-api rather than this repo, so I have left it alone.

go test ./..., go vet ./..., and gofmt -l . are clean.

@tas50
tas50 force-pushed the fix/policyfile-git-path-traversal branch 3 times, most recently from 53b4083 to 5de4988 Compare September 8, 2026 17:38
fetchGit joined the lock's "path" sub-option onto the clone directory
with no containment check, while every other lock-derived path in the
file goes through safeJoin. A path of "../.." resolves to any directory
on the machine, and copyTree would pull whatever sits there into the
cookbook cache.

This is defence in depth, not a live fix. EnsureCookbook is fetchGit's
only caller and reaches it through cinc-api's CookbookLock.Origin, which
prefers the "path" key over "git" and classifies such a lock as a path
source before fetchGit is ever called. The join is therefore protected
only by key precedence in another module: reorder those keys upstream,
or call fetchGit directly, and the guard disappears. It belongs next to
the join.

withinDir rather than safeJoin, because nested paths are legitimate here
(the "cookbooks/<name>" monorepo layout); only escaping the clone is not.

Signed-off-by: Tim Smith <tim@mondoo.com>
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