From 8ad7037828e5a0fca1009dabe290130da6368e39 Mon Sep 17 00:00:00 2001 From: Matt Butcher Date: Mon, 15 Jun 2020 16:58:15 -0600 Subject: [PATCH] backported archive improvements from v3 (#8318) Signed-off-by: Matt Butcher (cherry picked from commit def975f556a8d32307306be119a5f3b0411224d9) --- pkg/plugin/installer/http_installer.go | 58 +++++++++++- pkg/plugin/installer/http_installer_test.go | 100 +++++++++++++++++++- 2 files changed, 154 insertions(+), 4 deletions(-) diff --git a/pkg/plugin/installer/http_installer.go b/pkg/plugin/installer/http_installer.go index 4561a542e..f1b058c77 100644 --- a/pkg/plugin/installer/http_installer.go +++ b/pkg/plugin/installer/http_installer.go @@ -19,14 +19,16 @@ import ( "archive/tar" "bytes" "compress/gzip" + "errors" "fmt" "io" "os" + "path" "path/filepath" "regexp" "strings" - fp "github.com/cyphar/filepath-securejoin" + securejoin "github.com/cyphar/filepath-securejoin" "k8s.io/helm/pkg/getter" "k8s.io/helm/pkg/helm/environment" @@ -184,7 +186,7 @@ func (g *TarGzExtractor) Extract(buffer *bytes.Buffer, targetDir string) error { return err } - path, err := fp.SecureJoin(targetDir, header.Name) + path, err := cleanJoin(targetDir, header.Name) if err != nil { return err } @@ -212,3 +214,55 @@ func (g *TarGzExtractor) Extract(buffer *bytes.Buffer, targetDir string) error { return nil } + +// CleanJoin resolves dest as a subpath of root. +// +// This function runs several security checks on the path, generating an error if +// the supplied `dest` looks suspicious or would result in dubious behavior on the +// filesystem. +// +// CleanJoin assumes that any attempt by `dest` to break out of the CWD is an attempt +// to be malicious. (If you don't care about this, use the securejoin-filepath library.) +// It will emit an error if it detects paths that _look_ malicious, operating on the +// assumption that we don't actually want to do anything with files that already +// appear to be nefarious. +// +// - The character `:` is considered illegal because it is a separator on UNIX and a +// drive designator on Windows. +// - The path component `..` is considered suspicions, and therefore illegal +// - The character \ (backslash) is treated as a path separator and is converted to /. +// - Beginning a path with a path separator is illegal +// - Rudimentary symlink protects are offered by SecureJoin. +func cleanJoin(root, dest string) (string, error) { + + // On Windows, this is a drive separator. On UNIX-like, this is the path list separator. + // In neither case do we want to trust a TAR that contains these. + if strings.Contains(dest, ":") { + return "", errors.New("path contains ':', which is illegal") + } + + // The Go tar library does not convert separators for us. + // We assume here, as we do elsewhere, that `\\` means a Windows path. + dest = strings.ReplaceAll(dest, "\\", "/") + + // We want to alert the user that something bad was attempted. Cleaning it + // is not a good practice. + for _, part := range strings.Split(dest, "/") { + if part == ".." { + return "", errors.New("path contains '..', which is illegal") + } + } + + // If a path is absolute, the creator of the TAR is doing something shady. + if path.IsAbs(dest) { + return "", errors.New("path is absolute, which is illegal") + } + + // SecureJoin will do some cleaning, as well as some rudimentary checking of symlinks. + newpath, err := securejoin.SecureJoin(root, dest) + if err != nil { + return "", err + } + + return filepath.ToSlash(newpath), nil +} diff --git a/pkg/plugin/installer/http_installer_test.go b/pkg/plugin/installer/http_installer_test.go index 3511ecf66..1b8bfef8e 100644 --- a/pkg/plugin/installer/http_installer_test.go +++ b/pkg/plugin/installer/http_installer_test.go @@ -22,11 +22,12 @@ import ( "encoding/base64" "fmt" "io/ioutil" - "k8s.io/helm/pkg/helm/helmpath" "os" "path/filepath" "syscall" "testing" + + "k8s.io/helm/pkg/helm/helmpath" ) var _ Installer = new(HTTPInstaller) @@ -222,7 +223,7 @@ func TestExtract(t *testing.T) { Name, Body string Mode int64 }{ - {"../../plugin.yaml", "sneaky plugin metadata", 0600}, + {"./plugin.yaml", "sneaky plugin metadata", 0600}, {"README.md", "some text", 0777}, } for _, file := range files { @@ -283,3 +284,98 @@ func TestExtract(t *testing.T) { } } + +func TestExtractBadPlugin(t *testing.T) { + //create a temp home + hh, err := ioutil.TempDir("", "helm-home-") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(hh) + + home := helmpath.Home(hh) + if err := os.MkdirAll(home.Plugins(), 0755); err != nil { + t.Fatalf("Could not create %s: %s", home.Plugins(), err) + } + + cacheDir := filepath.Join(home.Cache(), "plugins", "plugin-key") + if err := os.MkdirAll(cacheDir, 0755); err != nil { + t.Fatalf("Could not create %s: %s", cacheDir, err) + } + + syscall.Umask(0000) + + var tarbuf bytes.Buffer + tw := tar.NewWriter(&tarbuf) + var files = []struct { + Name, Body string + Mode int64 + }{ + {"../../plugin.yaml", "sneaky plugin metadata", 0600}, + {"README.md", "some text", 0777}, + } + for _, file := range files { + hdr := &tar.Header{ + Name: file.Name, + Typeflag: tar.TypeReg, + Mode: file.Mode, + Size: int64(len(file.Body)), + } + if err := tw.WriteHeader(hdr); err != nil { + t.Fatal(err) + } + if _, err := tw.Write([]byte(file.Body)); err != nil { + t.Fatal(err) + } + } + if err := tw.Close(); err != nil { + t.Fatal(err) + } + + var buf bytes.Buffer + gz := gzip.NewWriter(&buf) + if _, err := gz.Write(tarbuf.Bytes()); err != nil { + t.Fatal(err) + } + gz.Close() + + source := "https://repo.localdomain/plugins/fake-plugin-0.0.1.tgz" + extr, err := NewExtractor(source) + if err != nil { + t.Fatal(err) + } + + if err = extr.Extract(&buf, cacheDir); err == nil { + t.Error("Should have failed because of bad plugin.yaml path") + } +} + +func TestCleanJoin(t *testing.T) { + for i, fixture := range []struct { + path string + expect string + expectError bool + }{ + {"foo/bar.txt", "/tmp/foo/bar.txt", false}, + {"/foo/bar.txt", "", true}, + {"./foo/bar.txt", "/tmp/foo/bar.txt", false}, + {"./././././foo/bar.txt", "/tmp/foo/bar.txt", false}, + {"../../../../foo/bar.txt", "", true}, + {"foo/../../../../bar.txt", "", true}, + {"c:/foo/bar.txt", "/tmp/c:/foo/bar.txt", true}, + {"foo\\bar.txt", "/tmp/foo/bar.txt", false}, + {"c:\\foo\\bar.txt", "", true}, + } { + out, err := cleanJoin("/tmp", fixture.path) + if err != nil { + if !fixture.expectError { + t.Errorf("Test %d: Path was not cleaned: %s", i, err) + } + continue + } + if fixture.expect != out { + t.Errorf("Test %d: Expected %q but got %q", i, fixture.expect, out) + } + } + +}