Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Improve the extractor and add tests (#8317)
Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>
(cherry picked from commit b6bbe4f)
  • Loading branch information
technosophos authored and Matthew Fisher committed Jun 15, 2020
1 parent 8f83204 commit 0ad800e
Show file tree
Hide file tree
Showing 2 changed files with 89 additions and 2 deletions.
61 changes: 59 additions & 2 deletions pkg/plugin/installer/http_installer.go
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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.
Expand All @@ -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:
Expand Down
30 changes: 30 additions & 0 deletions pkg/plugin/installer/http_installer_test.go
Expand Up @@ -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)
}
}

}

0 comments on commit 0ad800e

Please sign in to comment.