Repository navigation
fix(chart): write expanded chart files through os.Root - #32773
Open
Thanatos9404 wants to merge 1 commit into
Open
Thanatos9404 wants to merge 1 commit into
Thanatos9404 wants to merge 1 commit into
Conversation
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]>
2 of 3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
ExpandandExpandFileinpkg/chart/v2/util(and the copy ininternal/chart/v3/util) write the files of a chart archive to disk. They are used byhelm pull --untarand byhelm linton a packaged chart. Each output path is built withsecurejoin.SecureJoinand then written with theospackage.That path is only right at the moment it is computed. The filepath-securejoin README describes the
SecureJoinAPI as "fundamentally unsafe" when path components can change between the join and the use, and recommends newer APIs for new code.SecureJoinalso 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.lockwritten 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:
Expandopens the destination withos.Root, creates the chart directory through it, then opens a secondos.Rooton the chart directory and writes every file through that one.os.SameFile.os.Rootwas added in Go 1.24, andRoot.MkdirAllandRoot.WriteFilein Go 1.25;mainanddev-v3both declarego 1.26.0. #32774 does the same for plugin extraction. Once both are merged nothing importsgithub.com/cyphar/filepath-securejoinany more, and I will add thego mod tidychange 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:
Expandfail with an error ending inpath escapes from parent. Before,SecureJoinrewrote it into a path inside the destination and extraction continued there.os.Rootdoes not follow absolute links. Before,SecureJoinrewrote them to a path under the destination (the absolute path appended to it). Relative symlinks that stay inside the chart directory are still followed.os.Rootrefuses, such asa:b.yaml(an alternate data stream) or a reserved device name likecon, now fail withpath escapes from parent. Before, the data went into the stream or to the device.Tests: in both
expand_test.gofiles, 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:
Expandsucceeds 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:
docs neededlabel should be applied if so)