MM-21915: improve tar gz path traversal (#13675)

Automatic Merge

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
+}

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

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

utils/extract.go

@@ -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
-}

utils/path.go

@@ -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), "..")
-}

utils/path_test.go

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