diff --git a/go.mod b/go.mod index 219d9e212..44b64b4ee 100644 --- a/go.mod +++ b/go.mod @@ -28,6 +28,7 @@ require ( github.com/containerd/containerd v1.3.0-beta.2.0.20190823190603-4a2f61c4f2b4 github.com/containerd/continuity v0.0.0-20181203112020-004b46473808 github.com/cpuguy83/go-md2man v1.0.10 + github.com/cyphar/filepath-securejoin v0.2.2 github.com/davecgh/go-spew v1.1.1 github.com/deislabs/oras v0.7.0 github.com/dgrijalva/jwt-go v3.2.0+incompatible diff --git a/pkg/chart/loader/archive.go b/pkg/chart/loader/archive.go index 1418f7bd9..45b9fb468 100644 --- a/pkg/chart/loader/archive.go +++ b/pkg/chart/loader/archive.go @@ -22,6 +22,8 @@ import ( "compress/gzip" "io" "os" + "path" + "regexp" "strings" "github.com/pkg/errors" @@ -29,6 +31,8 @@ import ( "helm.sh/helm/v3/pkg/chart" ) +var drivePathPattern = regexp.MustCompile(`^[a-zA-Z]:/`) + // FileLoader loads a chart from a file type FileLoader string @@ -54,11 +58,13 @@ func LoadFile(name string) (*chart.Chart, error) { return LoadArchive(raw) } -// LoadArchive loads from a reader containing a compressed tar archive. -func LoadArchive(in io.Reader) (*chart.Chart, error) { +// LoadArchiveFiles reads in files out of an archive into memory. This function +// performs important path security checks and should always be used before +// expanding a tarball +func LoadArchiveFiles(in io.Reader) ([]*BufferedFile, error) { unzipped, err := gzip.NewReader(in) if err != nil { - return &chart.Chart{}, err + return nil, err } defer unzipped.Close() @@ -71,7 +77,7 @@ func LoadArchive(in io.Reader) (*chart.Chart, error) { break } if err != nil { - return &chart.Chart{}, err + return nil, err } if hd.FileInfo().IsDir() { @@ -92,12 +98,33 @@ func LoadArchive(in io.Reader) (*chart.Chart, error) { // Normalize the path to the / delimiter n = strings.ReplaceAll(n, delimiter, "/") + if path.IsAbs(n) { + return nil, errors.New("chart illegally contains absolute paths") + } + + n = path.Clean(n) + if n == "." { + // In this case, the original path was relative when it should have been absolute. + return nil, errors.Errorf("chart illegally contains content outside the base directory: %q", hd.Name) + } + 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") } if _, err := io.Copy(b, tr); err != nil { - return &chart.Chart{}, err + return nil, err } files = append(files, &BufferedFile{Name: n, Data: b.Bytes()}) @@ -107,6 +134,15 @@ func LoadArchive(in io.Reader) (*chart.Chart, error) { if len(files) == 0 { return nil, errors.New("no files in chart archive") } + return files, nil +} + +// LoadArchive loads from a reader containing a compressed tar archive. +func LoadArchive(in io.Reader) (*chart.Chart, error) { + files, err := LoadArchiveFiles(in) + if err != nil { + return nil, err + } return LoadFiles(files) } diff --git a/pkg/chart/loader/load_test.go b/pkg/chart/loader/load_test.go index 59f49349a..e36fd96a8 100644 --- a/pkg/chart/loader/load_test.go +++ b/pkg/chart/loader/load_test.go @@ -17,8 +17,15 @@ limitations under the License. package loader import ( + "archive/tar" "bytes" + "compress/gzip" + "io/ioutil" + "os" + "path/filepath" + "strings" "testing" + "time" "helm.sh/helm/v3/pkg/chart" ) @@ -160,6 +167,97 @@ func TestLoadV2WithReqs(t *testing.T) { verifyDependenciesLock(t, c) } +func TestLoadInvalidArchive(t *testing.T) { + tmpdir, err := ioutil.TempDir("", "helm-test-") + if err != nil { + t.Fatal(err) + } + defer os.Remove(tmpdir) + + writeTar := func(filename, internalPath string, body []byte) { + dest, err := os.Create(filename) + if err != nil { + t.Fatal(err) + } + zipper := gzip.NewWriter(dest) + tw := tar.NewWriter(zipper) + + h := &tar.Header{ + Name: internalPath, + Mode: 0755, + Size: int64(len(body)), + ModTime: time.Now(), + } + if err := tw.WriteHeader(h); err != nil { + t.Fatal(err) + } + if _, err := tw.Write(body); err != nil { + t.Fatal(err) + } + tw.Close() + zipper.Close() + dest.Close() + } + + for _, tt := range []struct { + chartname string + internal string + 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-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 content outside the base directory"}, + {"illegal-name2.tgz", "/./.", "chart illegally contains content outside the base directory"}, + {"illegal-name3.tgz", "missing-leading-slash", "chart illegally contains content outside the base directory"}, + {"illegal-name4.tgz", "/missing-leading-slash", "validation: chart.metadata is required"}, + {"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("hello: world")) + _, err = Load(illegalChart) + if err == nil { + t.Fatal("expected error when unpacking illegal files") + } + if !strings.Contains(err.Error(), tt.expectError) { + t.Errorf("Expected error to contain %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() != "validation: chart.metadata.name is required" { + 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() != "validation: chart.metadata is required" { + 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() != "validation: chart.metadata.name is required" { + t.Error(err) + } +} + func verifyChart(t *testing.T, c *chart.Chart) { t.Helper() if c.Name() == "" { diff --git a/pkg/chartutil/expand.go b/pkg/chartutil/expand.go index 983bc17af..87a5f88af 100644 --- a/pkg/chartutil/expand.go +++ b/pkg/chartutil/expand.go @@ -17,58 +17,69 @@ limitations under the License. package chartutil import ( - "archive/tar" - "compress/gzip" "io" + "io/ioutil" "os" "path/filepath" + + securejoin "github.com/cyphar/filepath-securejoin" + "github.com/pkg/errors" + "sigs.k8s.io/yaml" + + "helm.sh/helm/v3/pkg/chart" + "helm.sh/helm/v3/pkg/chart/loader" ) // Expand uncompresses and extracts a chart into the specified directory. func Expand(dir string, r io.Reader) error { - gr, err := gzip.NewReader(r) + files, err := loader.LoadArchiveFiles(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.Dir(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 + // Get the name of the chart + var chartName string + for _, file := range files { + if file.Name == "Chart.yaml" { + ch := &chart.Metadata{} + if err := yaml.Unmarshal(file.Data, ch); err != nil { + return errors.Wrap(err, "cannot load Chart.yaml") } - } - - path := filepath.Clean(filepath.Join(dir, header.Name)) - info := header.FileInfo() - if info.IsDir() { - if err = os.MkdirAll(path, info.Mode()); err != nil { + if err != nil { return err } - continue + chartName = ch.Name } + } + if chartName == "" { + return errors.New("chart name not specified") + } - file, err := os.OpenFile(path, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, info.Mode()) + // Find the base directory + chartdir, err := securejoin.SecureJoin(dir, chartName) + if err != nil { + return err + } + + // Copy all files verbatim. We don't parse these files because parsing can remove + // comments. + for _, file := range files { + outpath, err := securejoin.SecureJoin(chartdir, file.Name) if err != nil { return err } - if _, err = io.Copy(file, tr); err != nil { - file.Close() + + // Make sure the necessary subdirs get created. + basedir := filepath.Dir(outpath) + if err := os.MkdirAll(basedir, 0755); err != nil { + return err + } + + if err := ioutil.WriteFile(outpath, file.Data, 0644); err != nil { return err } - file.Close() } + return nil } diff --git a/pkg/chartutil/expand_test.go b/pkg/chartutil/expand_test.go new file mode 100644 index 000000000..b69acd3b6 --- /dev/null +++ b/pkg/chartutil/expand_test.go @@ -0,0 +1,121 @@ +/* +Copyright The Helm Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package chartutil + +import ( + "io/ioutil" + "os" + "path/filepath" + "testing" +) + +func TestExpand(t *testing.T) { + dest, err := ioutil.TempDir("", "helm-testing-") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dest) + + reader, err := os.Open("testdata/frobnitz-1.2.3.tgz") + if err != nil { + t.Fatal(err) + } + + if err := Expand(dest, reader); err != nil { + t.Fatal(err) + } + + expectedChartPath := filepath.Join(dest, "frobnitz") + fi, err := os.Stat(expectedChartPath) + if err != nil { + t.Fatal(err) + } + if !fi.IsDir() { + t.Fatalf("expected a chart directory at %s", expectedChartPath) + } + + dir, err := os.Open(expectedChartPath) + if err != nil { + t.Fatal(err) + } + + fis, err := dir.Readdir(0) + if err != nil { + t.Fatal(err) + } + + expectLen := 11 + if len(fis) != expectLen { + t.Errorf("Expected %d files, but got %d", expectLen, len(fis)) + } + + for _, fi := range fis { + expect, err := os.Stat(filepath.Join("testdata", "frobnitz", fi.Name())) + if err != nil { + t.Fatal(err) + } + if fi.Size() != expect.Size() { + t.Errorf("Expected %s to have size %d, got %d", fi.Name(), expect.Size(), fi.Size()) + } + } +} + +func TestExpandFile(t *testing.T) { + dest, err := ioutil.TempDir("", "helm-testing-") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dest) + + if err := ExpandFile(dest, "testdata/frobnitz-1.2.3.tgz"); err != nil { + t.Fatal(err) + } + + expectedChartPath := filepath.Join(dest, "frobnitz") + fi, err := os.Stat(expectedChartPath) + if err != nil { + t.Fatal(err) + } + if !fi.IsDir() { + t.Fatalf("expected a chart directory at %s", expectedChartPath) + } + + dir, err := os.Open(expectedChartPath) + if err != nil { + t.Fatal(err) + } + + fis, err := dir.Readdir(0) + if err != nil { + t.Fatal(err) + } + + expectLen := 11 + if len(fis) != expectLen { + t.Errorf("Expected %d files, but got %d", expectLen, len(fis)) + } + + for _, fi := range fis { + expect, err := os.Stat(filepath.Join("testdata", "frobnitz", fi.Name())) + if err != nil { + t.Fatal(err) + } + if fi.Size() != expect.Size() { + t.Errorf("Expected %s to have size %d, got %d", fi.Name(), expect.Size(), fi.Size()) + } + } +}