Skip to content

fix(chart): write expanded chart files through os.Root - #32773

Open
Thanatos9404 wants to merge 1 commit into
helm:mainfrom
Thanatos9404:harden/chart-expand-os-root
Open

Thanatos9404 wants to merge 1 commit into
helm:mainfrom
Thanatos9404:harden/chart-expand-os-root

Conversation

@Thanatos9404

@Thanatos9404 Thanatos9404 commented Oct 18, 2026 •

Copy link
Copy Markdown

What this PR does / why we need it:

Expand and ExpandFile in pkg/chart/v2/util (and the copy in internal/chart/v3/util) write the files of a chart archive to disk. They are used by helm pull --untar and by helm lint on a packaged chart. Each output path is built with securejoin.SecureJoin and then written with the os package.

That path is only right at the moment it is computed. The filepath-securejoin README describes the SecureJoin API as "fundamentally unsafe" when path components can change between the join and the use, and recommends newer APIs for new code. SecureJoin also deals with a symlink in the destination that points outside it by rewriting it into a path inside the destination, so extraction carries on and writes somewhere the archive did not name.

Writes that leave their intended directory have come up in Helm several times: GHSA-hr2v-4r36-88hr (chart extraction collapsing into the output directory through a dot segment chart name), GHSA-v53g-5gjp-272r (a chart saved outside its directory through its name) and GHSA-557j-xg8c-q2mm (Chart.lock written through a symlink), and for plugins GHSA-vmx8-mqv2-9gmg and GHSA-qq3j-xp49-j73f. All of those are fixed. This PR does not fix a known vulnerability. It removes the path joining from chart expansion, so that a chart file cannot be written outside the chart directory by construction instead of relying on each path check being right.

The change:

  • Expand opens the destination with os.Root, creates the chart directory through it, then opens a second os.Root on the chart directory and writes every file through that one.
  • The chart name checks added for GHSA-hr2v-4r36-88hr are unchanged. The check that the chart directory is never the destination itself is kept; with no joined path left to compare, it now compares the two directories with os.SameFile.
  • An empty destination still means the current directory.

os.Root was added in Go 1.24, and Root.MkdirAll and Root.WriteFile in Go 1.25; main and dev-v3 both declare go 1.26.0. #32774 does the same for plugin extraction. Once both are merged nothing imports github.com/cyphar/filepath-securejoin any more, and I will add the go mod tidy change to whichever of the two is merged second (or send it separately).

Special notes for your reviewer:

For well formed charts nothing changes: same files and layout, directories 0755, files 0644, and the same errors for bad chart names and for archive entries the loader rejects. The behaviour changes, all for unusual destinations or entry names, are:

  1. A symlink already in the destination that leads outside the chart directory (the chart directory itself, a subdirectory or a file) now makes Expand fail with an error ending in path escapes from parent. Before, SecureJoin rewrote it into a path inside the destination and extraction continued there.
  2. Absolute symlinks are refused even when they point inside the chart directory, because os.Root does not follow absolute links. Before, SecureJoin rewrote them to a path under the destination (the absolute path appended to it). Relative symlinks that stay inside the chart directory are still followed.
  3. On Windows, entries with names that os.Root refuses, such as a:b.yaml (an alternate data stream) or a reserved device name like con, now fail with path escapes from parent. Before, the data went into the stream or to the device.

Tests: in both expand_test.go files, one table test feeds archives that try to leave the destination (an entry with .., an absolute entry, and a symlink entry followed by a write through it) and destinations that already contain symlinks pointing outside (chart directory, subdirectory, file, and a chart directory symlinked to the destination itself). Every case must fail, leave the outside directory empty and write nothing into the destination root. Other tests check that a well formed chart with nested directories, with and without directory entries, is written with the same content and modes into a destination that does not exist yet, that a relative symlink inside the chart directory is still followed, and that an empty destination still extracts into the current directory.

On main, the three cases with an existing symlink pointing outside fail in each package: Expand succeeds and writes at the rewritten path. Every other new test passes on main and on this branch. The symlink cases skip on Windows when symlinks cannot be created; I ran them on Linux.

If applicable:

  • this PR contains user facing changes (the docs needed label should be applied if so)
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

Expand (pkg/chart/v2/util and internal/chart/v3/util) built each output
path with securejoin.SecureJoin and then wrote to it through the os
package. The joined path is only valid at the moment it is computed:
the filepath-securejoin documentation notes that SecureJoin cannot
protect against path components that change before the path is used.
A symlink in the destination pointing outside it was also rewritten
into a path inside the destination instead of being refused.

Open the destination with os.Root, create the chart directory through
it, and write every file through a second os.Root opened on the chart
directory, so no file can be created outside the chart directory. The
chart name checks are unchanged, and the check that the chart
directory is never the destination itself now uses os.SameFile. An
empty destination still means the current directory.

Behaviour for well formed charts is unchanged. Changes for unusual
destinations: a symlink in the destination that leaves it, or any
absolute symlink, now makes Expand fail instead of being rewritten;
relative symlinks that stay inside the chart directory are still
followed. On Windows, entries named like a:b.yaml or con now fail
instead of writing an alternate data stream or a device.

Signed-off-by: Yashvardhan Thanvi <[email protected]>
Copilot AI balanced review requested due to automatic review settings October 18, 2026 12:30
@pull-request-size pull-request-size Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 18, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added the v4.x Issues and Pull Requests related to the major version v4 label Oct 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Denotes a PR that changes 100-499 lines, ignoring generated files. v4.x Issues and Pull Requests related to the major version v4

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants