diff --git a/pkg/plugin/installer/http_installer.go b/pkg/plugin/installer/http_installer.go index c07cad80a..28e50b72b 100644 --- a/pkg/plugin/installer/http_installer.go +++ b/pkg/plugin/installer/http_installer.go @@ -21,10 +21,12 @@ import ( "compress/gzip" "io" "os" + "path" "path/filepath" "regexp" "strings" + securejoin "github.com/cyphar/filepath-securejoin" "github.com/pkg/errors" "helm.sh/helm/v3/internal/third_party/dep/fs" @@ -118,7 +120,7 @@ func (i *HTTPInstaller) Install() error { } if err := i.extractor.Extract(pluginData, i.CacheDir); err != nil { - return err + return errors.Wrap(err, "extracting files from archive") } if !isPlugin(i.CacheDir) { @@ -148,6 +150,58 @@ func (i HTTPInstaller) Path() string { return helmpath.DataPath("plugins", i.PluginName) } +// 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 +} + // Extract extracts compressed archives // // Implements Extractor. @@ -171,7 +225,10 @@ func (g *TarGzExtractor) Extract(buffer *bytes.Buffer, targetDir string) error { return err } - path := filepath.Join(targetDir, header.Name) + path, err := cleanJoin(targetDir, header.Name) + if err != nil { + return err + } switch header.Typeflag { case tar.TypeDir: diff --git a/pkg/plugin/installer/http_installer_test.go b/pkg/plugin/installer/http_installer_test.go index 99470ace6..3eb92ee77 100644 --- a/pkg/plugin/installer/http_installer_test.go +++ b/pkg/plugin/installer/http_installer_test.go @@ -277,3 +277,33 @@ func TestExtract(t *testing.T) { } } + +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) + } + } + +}