From 9726a917cb08a1580bdfdd8350ebd771ac1897f3 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Mon, 27 Jan 2020 12:33:11 -0400 Subject: [PATCH] MM-21915: improve tar gz path traversal (#13675) Automatic Merge --- app/extract_plugin_tar.go | 82 ++++++++++++++++ app/extract_plugin_tar_test.go | 174 +++++++++++++++++++++++++++++++++ app/plugin_install.go | 2 +- utils/extract.go | 76 -------------- utils/path.go | 15 --- utils/path_test.go | 31 ------ 6 files changed, 257 insertions(+), 123 deletions(-) create mode 100644 app/extract_plugin_tar.go create mode 100644 app/extract_plugin_tar_test.go delete mode 100644 utils/extract.go delete mode 100644 utils/path.go delete mode 100644 utils/path_test.go diff --git a/app/extract_plugin_tar.go b/app/extract_plugin_tar.go new file mode 100644 index 0000000000..f7ed301bf5 --- /dev/null +++ b/app/extract_plugin_tar.go @@ -0,0 +1,82 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package app + +import ( + "archive/tar" + "compress/gzip" + "io" + "os" + "path/filepath" + "strings" + + "github.com/mattermost/mattermost-server/v5/mlog" + "github.com/pkg/errors" +) + +// extractTarGz takes in an io.Reader containing the bytes for a .tar.gz file and +// a destination string to extract to. +func extractTarGz(gzipStream io.Reader, dst string) error { + if dst == "" { + return errors.New("no destination path provided") + } + + uncompressedStream, err := gzip.NewReader(gzipStream) + if err != nil { + return errors.Wrap(err, "failed to initialize gzip reader") + } + defer uncompressedStream.Close() + + tarReader := tar.NewReader(uncompressedStream) + + for { + header, err := tarReader.Next() + if err == io.EOF { + break + } else if err != nil { + return errors.Wrap(err, "failed to read next file from archive") + } + + // Pre-emptively check type flag to avoid reporting a misleading error in + // trying to sanitize the header name. + switch header.Typeflag { + case tar.TypeDir: + case tar.TypeReg: + default: + mlog.Warn("skipping unsupported header type on extracting tar file", mlog.String("header_type", string(header.Typeflag)), mlog.String("header_name", header.Name)) + continue + } + + // filepath.HasPrefix is deprecated, so we just use strings.HasPrefix to ensure + // the target path remains rooted at dst and has no `../` escaping outside. + path := filepath.Join(dst, header.Name) + if !strings.HasPrefix(path, dst) { + return errors.Errorf("failed to sanitize path %s", header.Name) + } + + switch header.Typeflag { + case tar.TypeDir: + if err := os.Mkdir(path, 0744); err != nil && !os.IsExist(err) { + return err + } + case tar.TypeReg: + dir := filepath.Dir(path) + + if err := os.MkdirAll(dir, 0744); err != nil { + return err + } + + outFile, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_TRUNC, os.FileMode(header.Mode)) + if err != nil { + return err + } + defer outFile.Close() + if _, err := io.Copy(outFile, tarReader); err != nil { + return err + } + } + } + + return nil +} diff --git a/app/extract_plugin_tar_test.go b/app/extract_plugin_tar_test.go new file mode 100644 index 0000000000..8fba0ef4d7 --- /dev/null +++ b/app/extract_plugin_tar_test.go @@ -0,0 +1,174 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package app + +import ( + "archive/tar" + "bytes" + "compress/gzip" + "fmt" + "io/ioutil" + "os" + "path/filepath" + "sort" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func assertDirectoryContents(t *testing.T, dir string, expectedFiles []string) { + var files []string + err := filepath.Walk(dir, func(path string, info os.FileInfo, err error) error { + require.NoError(t, err) + file := strings.TrimPrefix(path, dir) + file = strings.TrimPrefix(file, "/") + files = append(files, file) + return nil + }) + require.NoError(t, err) + + sort.Strings(files) + sort.Strings(expectedFiles) + assert.Equal(t, expectedFiles, files) +} + +func TestExtractTarGz(t *testing.T) { + makeArchive := func(t *testing.T, files []*tar.Header) bytes.Buffer { + // Build an in-memory archive with the specified files, writing the path as each + // file's contents when applicable. + var archive bytes.Buffer + archiveGzWriter := gzip.NewWriter(&archive) + archiveWriter := tar.NewWriter(archiveGzWriter) + for _, file := range files { + if file.Typeflag == tar.TypeReg { + contents := []byte(file.Name) + file.Size = int64(len(contents)) + err := archiveWriter.WriteHeader(file) + require.NoError(t, err) + + var written int + written, err = archiveWriter.Write(contents) + require.NoError(t, err) + require.EqualValues(t, len(contents), written) + } else { + err := archiveWriter.WriteHeader(file) + require.NoError(t, err) + } + } + err := archiveWriter.Close() + require.NoError(t, err) + err = archiveGzWriter.Close() + require.NoError(t, err) + + return archive + } + + t.Run("empty dst", func(t *testing.T) { + archive := makeArchive(t, nil) + err := extractTarGz(&archive, "") + require.Error(t, err) + }) + + testCases := []struct { + Files []*tar.Header + ExpectedError bool + ExpectedFiles []string + }{ + { + []*tar.Header{{Name: "../test/path", Typeflag: tar.TypeDir}}, + true, + nil, + }, + { + []*tar.Header{{Name: "../../test/path", Typeflag: tar.TypeDir}}, + true, + nil, + }, + { + []*tar.Header{{Name: "../../test/../path", Typeflag: tar.TypeDir}}, + true, + nil, + }, + { + []*tar.Header{{Name: "test/../../path", Typeflag: tar.TypeDir}}, + true, + nil, + }, + { + []*tar.Header{{Name: "test/path/../..", Typeflag: tar.TypeDir}}, + false, + []string{""}, + }, + { + []*tar.Header{{Name: "test", Typeflag: tar.TypeDir}}, + false, + []string{"", "test"}, + }, + { + []*tar.Header{ + {Name: "test", Typeflag: tar.TypeDir}, + {Name: "test/path", Typeflag: tar.TypeDir}, + }, + false, + []string{"", "test", "test/path"}, + }, + { + []*tar.Header{ + {Name: "test", Typeflag: tar.TypeDir}, + {Name: "test/path/", Typeflag: tar.TypeDir}, + }, + false, + []string{"", "test", "test/path"}, + }, + { + []*tar.Header{ + {Name: "test", Typeflag: tar.TypeDir}, + {Name: "test/path", Typeflag: tar.TypeDir}, + {Name: "test/path/file.ext", Typeflag: tar.TypeReg}, + }, + false, + []string{"", "test", "test/path", "test/path/file.ext"}, + }, + { + []*tar.Header{ + {Name: "/../../file.ext", Typeflag: tar.TypeReg}, + }, + true, + nil, + }, + { + []*tar.Header{ + {Name: "/../../link", Typeflag: tar.TypeLink}, + }, + false, + []string{""}, + }, + { + []*tar.Header{ + {Name: "..file", Typeflag: tar.TypeReg}, + }, + false, + []string{"", "..file"}, + }, + } + + for i, testCase := range testCases { + t.Run(fmt.Sprintf("test-%d", i), func(t *testing.T) { + dst, err := ioutil.TempDir("", "TestExtractTarGz") + require.NoError(t, err) + defer os.RemoveAll(dst) + + archive := makeArchive(t, testCase.Files) + err = extractTarGz(&archive, dst) + if testCase.ExpectedError { + require.Error(t, err) + } else { + require.NoError(t, err) + assertDirectoryContents(t, dst, testCase.ExpectedFiles) + } + }) + } +} diff --git a/app/plugin_install.go b/app/plugin_install.go index 413f97c4a8..1044ea3844 100644 --- a/app/plugin_install.go +++ b/app/plugin_install.go @@ -271,7 +271,7 @@ func (a *App) installPluginLocally(pluginFile, signature io.ReadSeeker, installa func extractPlugin(pluginFile io.ReadSeeker, extractDir string) (*model.Manifest, string, *model.AppError) { pluginFile.Seek(0, 0) - if err := utils.ExtractTarGz(pluginFile, extractDir); err != nil { + if err := extractTarGz(pluginFile, extractDir); err != nil { return nil, "", model.NewAppError("extractPlugin", "app.plugin.extract.app_error", nil, err.Error(), http.StatusBadRequest) } diff --git a/utils/extract.go b/utils/extract.go deleted file mode 100644 index 37064e1d16..0000000000 --- a/utils/extract.go +++ /dev/null @@ -1,76 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -package utils - -import ( - "archive/tar" - "compress/gzip" - "fmt" - "io" - "os" - "path/filepath" -) - -// ExtractTarGz takes in an io.Reader containing the bytes for a .tar.gz file and -// a destination string to extract to. -func ExtractTarGz(gzipStream io.Reader, dst string) error { - uncompressedStream, err := gzip.NewReader(gzipStream) - if err != nil { - return fmt.Errorf("ExtractTarGz: NewReader failed: %s", err.Error()) - } - defer uncompressedStream.Close() - - tarReader := tar.NewReader(uncompressedStream) - - for { - header, err := tarReader.Next() - - if err == io.EOF { - break - } - - if err != nil { - return fmt.Errorf("ExtractTarGz: Next() failed: %s", err.Error()) - } - - switch header.Typeflag { - case tar.TypeDir: - if PathTraversesUpward(header.Name) { - return fmt.Errorf("ExtractTarGz: path attempts to traverse upwards") - } - - path := filepath.Join(dst, header.Name) - if err := os.Mkdir(path, 0744); err != nil && !os.IsExist(err) { - return fmt.Errorf("ExtractTarGz: Mkdir() failed: %s", err.Error()) - } - case tar.TypeReg: - if PathTraversesUpward(header.Name) { - return fmt.Errorf("ExtractTarGz: path attempts to traverse upwards") - } - - path := filepath.Join(dst, header.Name) - dir := filepath.Dir(path) - - if err := os.MkdirAll(dir, 0744); err != nil { - return fmt.Errorf("ExtractTarGz: MkdirAll() failed: %s", err.Error()) - } - - outFile, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_TRUNC, os.FileMode(header.Mode)) - if err != nil { - return fmt.Errorf("ExtractTarGz: Create() failed: %s", err.Error()) - } - defer outFile.Close() - if _, err := io.Copy(outFile, tarReader); err != nil { - return fmt.Errorf("ExtractTarGz: Copy() failed: %s", err.Error()) - } - default: - return fmt.Errorf( - "ExtractTarGz: unknown type: %v in %v", - header.Typeflag, - header.Name) - } - } - - return nil -} diff --git a/utils/path.go b/utils/path.go deleted file mode 100644 index d1fc6ebc7f..0000000000 --- a/utils/path.go +++ /dev/null @@ -1,15 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -package utils - -import ( - "path/filepath" - "strings" -) - -// PathTraversesUpward will return true if the path attempts to traverse upwards by using -// ".." in the path. -func PathTraversesUpward(path string) bool { - return strings.HasPrefix(filepath.Clean(path), "..") -} diff --git a/utils/path_test.go b/utils/path_test.go deleted file mode 100644 index 86a756a925..0000000000 --- a/utils/path_test.go +++ /dev/null @@ -1,31 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -package utils - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestPathTraversesUpward(t *testing.T) { - cases := []struct { - input string - expected bool - }{ - {"../test/path", true}, - {"../../test/path", true}, - {"../../test/../path", true}, - {"test/../../path", true}, - {"test/path/../../", false}, - {"test", false}, - {"test/path", false}, - {"test/path/", false}, - {"test/path/file.ext", false}, - } - - for _, c := range cases { - assert.Equal(t, c.expected, PathTraversesUpward(c.input), c.input) - } -}