Merge pull request #6607 from thomastaylor312/fix/missing_path_validation

fix(chart): Ports security fix for invalid paths in tarballs

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

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)
 }

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() == "" {

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
 }
 

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())
+		}
+	}
+}