From cfcee69b7e2ae9b1a4ba95dd3a914bb315af1f2a Mon Sep 17 00:00:00 2001 From: Matt Butcher Date: Thu, 10 Jan 2019 10:00:25 -0700 Subject: [PATCH] fix: Cover a few Windows cases and also remove a duplicate tar reader Signed-off-by: Matt Butcher --- pkg/chartutil/expand.go | 47 ++----------------------------------- pkg/chartutil/load.go | 13 +++++++++++ pkg/chartutil/load_test.go | 48 +++++++++++++++++++++++++++++++++----- 3 files changed, 57 insertions(+), 51 deletions(-) diff --git a/pkg/chartutil/expand.go b/pkg/chartutil/expand.go index 1d49b159f..33f7c98f5 100644 --- a/pkg/chartutil/expand.go +++ b/pkg/chartutil/expand.go @@ -17,60 +17,17 @@ limitations under the License. package chartutil import ( - "archive/tar" - "compress/gzip" "io" "os" - "path/filepath" ) // Expand uncompresses and extracts a chart into the specified directory. func Expand(dir string, r io.Reader) error { - gr, err := gzip.NewReader(r) + ch, err := LoadArchive(r) if err != nil { return err } - defer gr.Close() - tr := tar.NewReader(gr) - for { - header, err := tr.Next() - if err == io.EOF { - break - } else if err != nil { - return err - } - - //split header name and create missing directories - d, _ := filepath.Split(header.Name) - fullDir := filepath.Join(dir, d) - _, err = os.Stat(fullDir) - if err != nil && d != "" { - if err := os.MkdirAll(fullDir, 0700); err != nil { - return err - } - } - - path := filepath.Clean(filepath.Join(dir, header.Name)) - info := header.FileInfo() - if info.IsDir() { - if err = os.MkdirAll(path, info.Mode()); err != nil { - return err - } - continue - } - - file, err := os.OpenFile(path, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, info.Mode()) - if err != nil { - return err - } - _, err = io.Copy(file, tr) - if err != nil { - file.Close() - return err - } - file.Close() - } - return nil + return SaveDir(ch, dir) } // ExpandFile expands the src file into the dest directory. diff --git a/pkg/chartutil/load.go b/pkg/chartutil/load.go index 0e077b1b8..b554efeee 100644 --- a/pkg/chartutil/load.go +++ b/pkg/chartutil/load.go @@ -27,6 +27,7 @@ import ( "os" "path" "path/filepath" + "regexp" "strings" "github.com/golang/protobuf/ptypes/any" @@ -64,6 +65,8 @@ type BufferedFile struct { Data []byte } +var drivePathPattern = regexp.MustCompile(`^[a-zA-Z]:/`) + // LoadArchive loads from a reader containing a compressed tar archive. func LoadArchive(in io.Reader) (*chart.Chart, error) { unzipped, err := gzip.NewReader(in) @@ -108,13 +111,23 @@ func LoadArchive(in io.Reader) (*chart.Chart, error) { println("path", n) n = path.Clean(n) + println(" cleaned", n) if n == "." { + // In this case, the original path was relative when it should have been absolute. return nil, errors.New("chart illegally contains empty path") } if strings.HasPrefix(n, "..") { return nil, errors.New("chart illegally references parent directory") } + // In some particularly arcane acts of path creativity, it is possible to intermix + // UNIX and Windows style paths in such a way that you produce a result of the form + // c:/foo even after all the built-in absolute path checks. So we explicitly check + // for this condition. + if drivePathPattern.MatchString(n) { + return nil, errors.New("chart contains illegally named files") + } + if parts[0] == "Chart.yaml" { return nil, errors.New("chart yaml not in base directory") } diff --git a/pkg/chartutil/load_test.go b/pkg/chartutil/load_test.go index 207aae641..c031a6a96 100644 --- a/pkg/chartutil/load_test.go +++ b/pkg/chartutil/load_test.go @@ -87,21 +87,57 @@ func TestLoadArchive_InvalidArchive(t *testing.T) { expectError string }{ {"illegal-dots.tgz", "../../malformed-helm-test", "chart illegally references parent directory"}, - {"illegal-dots2.tgz", "foo/../../malformed-helm-test", "chart illegally references parent directory"}, - {"illegal-name.tgz", ".", "chart illegally contains empty path"}, - {"illegal-name.tgz", " ", "chart illegally contains empty path"}, - {"illegal-name.tgz", " ", "chart illegally contains empty path"}, + {"illegal-dots2.tgz", "/foo/../../malformed-helm-test", "chart illegally references parent directory"}, + {"illegal-dots3.tgz", "/../../malformed-helm-test", "chart illegally references parent directory"}, + {"illegal-dots4.tgz", "./../../malformed-helm-test", "chart illegally references parent directory"}, + {"illegal-name.tgz", "./.", "chart illegally contains empty path"}, + {"illegal-name2.tgz", "/./.", "chart illegally contains empty path"}, + {"illegal-name3.tgz", "missing-leading-slash", "chart illegally contains empty path"}, + {"illegal-name4.tgz", "/missing-leading-slash", "chart metadata (Chart.yaml) missing"}, + {"illegal-abspath.tgz", "//foo", "chart illegally contains absolute paths"}, + {"illegal-abspath2.tgz", "///foo", "chart illegally contains absolute paths"}, + {"illegal-abspath3.tgz", "\\\\foo", "chart illegally contains absolute paths"}, + {"illegal-abspath3.tgz", "\\..\\..\\foo", "chart illegally references parent directory"}, + + // Under special circumstances, this can get normalized to things that look like absolute Windows paths + {"illegal-abspath4.tgz", "\\.\\c:\\\\foo", "chart contains illegally named files"}, + {"illegal-abspath5.tgz", "/./c://foo", "chart contains illegally named files"}, + {"illegal-abspath6.tgz", "\\\\?\\Some\\windows\\magic", "chart illegally contains absolute paths"}, } { illegalChart := filepath.Join(tmpdir, tt.chartname) - writeTar(illegalChart, tt.internal, []byte("junk")) + writeTar(illegalChart, tt.internal, []byte("hello: world")) _, err = Load(illegalChart) if err == nil { t.Fatal("expected error when unpacking illegal files") } if err.Error() != tt.expectError { - t.Errorf("Expected %q, got %q", tt.expectError, err.Error()) + t.Errorf("Expected %q, got %q for %s", tt.expectError, err.Error(), tt.chartname) } } + + // Make sure that absolute path gets interpreted as relative + illegalChart := filepath.Join(tmpdir, "abs-path.tgz") + writeTar(illegalChart, "/Chart.yaml", []byte("hello: world")) + _, err = Load(illegalChart) + if err.Error() != "invalid chart (Chart.yaml): name must not be empty" { + t.Error(err) + } + + // And just to validate that the above was not spurious + illegalChart = filepath.Join(tmpdir, "abs-path2.tgz") + writeTar(illegalChart, "files/whatever.yaml", []byte("hello: world")) + _, err = Load(illegalChart) + if err.Error() != "chart metadata (Chart.yaml) missing" { + t.Error(err) + } + + // Finally, test that drive letter gets stripped off on Windows + illegalChart = filepath.Join(tmpdir, "abs-winpath.tgz") + writeTar(illegalChart, "c:\\Chart.yaml", []byte("hello: world")) + _, err = Load(illegalChart) + if err.Error() != "invalid chart (Chart.yaml): name must not be empty" { + t.Error(err) + } } func TestLoadFiles(t *testing.T) {