fix: Cover a few Windows cases and also remove a duplicate tar reader

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>

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.

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

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