fix(emissary): prevent output path traversal in saveParameter and saveArtifact#16031
fix(emissary): prevent output path traversal in saveParameter and saveArtifact#16031jayantkamble10000 wants to merge 1 commit intoargoproj:mainfrom
Conversation
|
Hey @jayantkamble10000 could you please sign off your commit? |
jayantkamble10000
force-pushed
the
fix/saveparameter-path-traversal
branch
2 times, most recently
from
e24dcd7 to
15d1477
Compare
|
Please check its done |
isubasinghe
left a comment
isubasinghe
left a comment
There was a problem hiding this comment.
https://go.dev/blog/osroot please take a read here
| if !strings.HasPrefix(filepath.Clean(dstPath)+string(os.PathSeparator), safeArtifactBase) { | ||
| return fmt.Errorf("path traversal in output artifact path %q", srcPath) | ||
| } |
There was a problem hiding this comment.
if !filepath.IsLocal(srcPath) is much cleaner
There was a problem hiding this comment.
Done — replaced strings.HasPrefix bounds check with filepath.IsLocal(srcPath) in both functions as suggested. Also updated the test from LegitimateAbsolutePath to LegitimateRelativePath since filepath.IsLocal correctly rejects absolute paths (which is the right behaviour — output paths should be relative to the container's workingDir). All tests pass.
|
Done — replaced strings. |
jayantkamble10000
force-pushed
the
fix/saveparameter-path-traversal
branch
4 times, most recently
from
d26d762 to
22eea89
Compare
|
Updated as suggested — switched to `os.OpenRoot` for creating the destination file so the OS enforces the directory boundary (also protects against symlink escapes, which a pure lexical check can't catch). Combined with `filepath.IsLocal` for an early, clear error before the file-create attempt. Regarding the two failing E2E checks: both failures are pre-existing flaky tests unrelated to this change:
Also rebased onto the latest `main` — branch is now a single clean commit. |
Normalize absolute source paths by stripping the leading slash before building the destination path (e.g. /tmp/artifact → tmp/artifact). Validate the resulting relative path with filepath.IsLocal to block any .. traversal, then create the destination file through os.OpenRoot so the OS enforces the directory boundary and symlink escapes are also prevented. Signed-off-by: Jayant Kamble <jayantkamble10000@gmail.com>
jayantkamble10000
force-pushed
the
fix/saveparameter-path-traversal
branch
from
22eea89 to
6d25864
Compare
|
@isubasinghe All 40 checks are now passing (including DCO). I believe I've addressed all the feedback — switched to |
|
Done @isubasinghe |
2 participants
Summary
Both
saveParameterandsaveArtifactbuilt destination paths withoutenforcing that the result stays inside
/var/run/argo/outputs/.saveParameterused raw string concatenation:A workflow spec with valueFrom.path: "../../ctr/containerB/exitcode"
resolves to /var/run/argo/ctr/containerB/exitcode — a world-writable
directory (created 0o777 by design for non-root containers) — letting
one container in a ContainerSet falsify another container's exit code
and bypass DAG dependency checks.
saveArtifact had the same issue: a traversal path resolves outside outputs/artifacts/ regardless of the .tgz suffix appended to the filename
Fix
Add a filepath.Join + strings.HasPrefix bounds check in both
functions. The check is placed before os.Open/os.Stat so traversal
paths are rejected even when the source file does not yet exist.
Tests
Added TestSaveParameterPathTraversal and TestSaveArtifactPathTraversal
covering:
Fixes https://github.com/argoproj/argo-workflows/security/advisories/GHSA-r9w2-rp4m-gwr9