Disallow artifact path traversal, escape-hatch experiment #2815
Conversation
* Remove astonishing `i += 100` loop * Rename getTargetPath to targetPath * Rename args (they're both paths, with different purposes) * Rename internal variables * Use filepath.Join where appropriate * Add call to filepath.Clean for dlPath * Rewrite tests in table-driven form * Remove ...x/x/x/x/x/x/x... test
* Remove astonishing `i += 100` loop * Rename getTargetPath to targetPath * Rename args (they're both paths, with different purposes) * Rename internal variables * Use filepath.Join where appropriate * Add call to filepath.Clean for dlPath * Rewrite tests in table-driven form * Remove ...x/x/x/x/x/x/x... test
DrJosh9000
force-pushed
the
ps-68-disallow-artifact-path-traversal
branch
from
4a4a257 to
822ac07
Compare
| // of the download path, then trim the last component of the destination. | ||
| lastIndex := len(destPathComponents) - 1 | ||
| lastDestComponent := destPathComponents[lastIndex] | ||
| if lastDestComponent == dlPathComponents[0] { |
There was a problem hiding this comment.
not relevant to this PR, but this isn't mentioned anywhere in the doc: https://buildkite.com/docs/agent/v3/cli-artifact#downloading-artifacts-artifact-download-examples
😮
agent/download.go
Outdated
| if c := dlPathComponents[0]; c != "" && c != ".." { | ||
| break | ||
| } | ||
| dlPathComponents = dlPathComponents[1:] |
There was a problem hiding this comment.
I feel mutating users' intention is a bit questionable. It's likely to cause some support tickets. 🤔
And given they can add relative path in both download path and destination path, is this fix sufficient?
There was a problem hiding this comment.
I answered this out of band, but basically, it's less surprising if a user has deliberately written the command:
buildkite-agent artifact download "*foo" ../../destthan if they wrote the following (and "*foo" happened to match an artifact with path "../../dest"):
buildkite-agent artifact download "*foo" .
|
I'm strongly considering inverting the experiment, as there's little evidence I can find that anyone is relying on the old behaviour. |
DrJosh9000
changed the title
Description
Prevent
..path components in artifact records from causingartifact downloadto traverse outside the destination path, unless a new experiment is enabled.A secondary change is the removal of the loop from target path calculation that compared every 100th download path component against the final destination path component; the probability anyone has exercised the code path is effectively zero, and the behaviour would be incredibly surprising if it were hit.
Context
https://linear.app/buildkite/issue/PS-68/fix-surprising-agent-behaviour-in-artifact-download
Changes
..path componentsfor ... i += 100loopTesting
go test ./...). Buildkite employees may check this if the pipeline has run automatically.go fmt ./...)