Ensure file permissions are set appropriately when untarring archives…

… during autoupdate (#1680)

ee/tuf/library_manager.go

@@ -1,10 +1,13 @@
 package tuf
 
 import (
+	"archive/tar"
 	"bytes"
+	"compress/gzip"
 	"context"
 	"fmt"
 	"io"
+	"io/fs"
 	"log/slog"
 	"net/http"
 	"os"
@@ -16,7 +19,6 @@ import (
 	"time"
 
 	"github.com/Masterminds/semver"
-	"github.com/kolide/kit/fsutil"
 	"github.com/kolide/launcher/pkg/backoff"
 	"github.com/kolide/launcher/pkg/traces"
 	"github.com/theupdateframework/go-tuf/data"
@@ -207,9 +209,8 @@ func (ulm *updateLibraryManager) moveVerifiedUpdate(binary autoupdatableBinary,
 		}
 	}()
 
-	// Untar the archive. Note that `UntarBundle` calls `filepath.Dir(destination)`, so the inclusion of `binary`
-	// here doesn't matter as it's immediately stripped off.
-	if err := fsutil.UntarBundle(filepath.Join(stagedVersionedDirectory, string(binary)), stagedUpdate); err != nil {
+	// Untar the archive.
+	if err := untar(stagedVersionedDirectory, stagedUpdate); err != nil {
 		return fmt.Errorf("could not untar update to %s: %w", stagedVersionedDirectory, err)
 	}
 
@@ -249,6 +250,87 @@ func (ulm *updateLibraryManager) moveVerifiedUpdate(binary autoupdatableBinary,
 	return nil
 }
 
+// untar extracts the archive `source` to the given `destinationDir`. It sanitizes
+// extract paths and file permissions.
+func untar(destinationDir string, source string) error {
+	f, err := os.Open(source)
+	if err != nil {
+		return fmt.Errorf("opening source: %w", err)
+	}
+	defer f.Close()
+
+	gzr, err := gzip.NewReader(f)
+	if err != nil {
+		return fmt.Errorf("creating gzip reader from %s: %w", source, err)
+	}
+	defer gzr.Close()
+
+	tr := tar.NewReader(gzr)
+	for {
+		header, err := tr.Next()
+		if err == io.EOF {
+			break
+		}
+		if err != nil {
+			return fmt.Errorf("reading tar file: %w", err)
+		}
+
+		if err := sanitizeExtractPath(destinationDir, header.Name); err != nil {
+			return fmt.Errorf("checking filename: %w", err)
+		}
+
+		destPath := filepath.Join(destinationDir, header.Name)
+		info := header.FileInfo()
+		if info.IsDir() {
+			if err = os.MkdirAll(destPath, sanitizePermissions(info)); err != nil {
+				return fmt.Errorf("creating directory %s for tar file: %w", destPath, err)
+			}
+			continue
+		}
+
+		if err := writeBundleFile(destPath, sanitizePermissions(info), tr); err != nil {
+			return fmt.Errorf("writing file: %w", err)
+		}
+	}
+	return nil
+}
+
+// sanitizeExtractPath checks that the supplied extraction path is not
+// vulnerable to zip slip attacks. See https://snyk.io/research/zip-slip-vulnerability
+func sanitizeExtractPath(filePath string, destination string) error {
+	destpath := filepath.Join(destination, filePath)
+	if !strings.HasPrefix(destpath, filepath.Clean(destination)+string(os.PathSeparator)) {
+		return fmt.Errorf("illegal file path %s", filePath)
+	}
+	return nil
+}
+
+// Allow owner read/write/execute, and only read/execute for all others.
+const allowedPermissionBits fs.FileMode = fs.ModeType | 0755
+
+// sanitizePermissions ensures that only the file owner has write permissions.
+func sanitizePermissions(fileInfo fs.FileInfo) fs.FileMode {
+	return fileInfo.Mode() & allowedPermissionBits
+}
+
+// writeBundleFile reads from the given reader to create a file at the given path, with the desired permissions.
+func writeBundleFile(destPath string, perm fs.FileMode, srcReader io.Reader) error {
+	file, err := os.OpenFile(destPath, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, perm)
+	if err != nil {
+		return fmt.Errorf("opening %s: %w", destPath, err)
+	}
+	if _, err := io.Copy(file, srcReader); err != nil {
+		if closeErr := file.Close(); closeErr != nil {
+			return fmt.Errorf("copying to %s: %v; close error: %w", destPath, err, closeErr)
+		}
+		return fmt.Errorf("copying to %s: %w", destPath, err)
+	}
+	if err := file.Close(); err != nil {
+		return fmt.Errorf("closing %s: %w", destPath, err)
+	}
+	return nil
+}
+
 // removeUpdate removes a given version from the given binary's update library.
 func (ulm *updateLibraryManager) removeUpdate(binary autoupdatableBinary, binaryVersion string) {
 	directoryToRemove := filepath.Join(updatesDirectory(binary, ulm.baseDir), binaryVersion)

ee/tuf/library_manager_test.go

@@ -3,6 +3,7 @@ package tuf
 import (
 	"context"
 	"fmt"
+	"io/fs"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -12,6 +13,7 @@ import (
 	"sync"
 	"testing"
 
+	"github.com/kolide/kit/ulid"
 	tufci "github.com/kolide/launcher/ee/tuf/ci"
 	"github.com/kolide/launcher/pkg/log/multislogger"
 	"github.com/stretchr/testify/require"
@@ -291,6 +293,148 @@ func TestAddToLibrary_verifyStagedUpdate_handlesInvalidFiles(t *testing.T) {
 	}
 }
 
+func Test_sanitizeExtractPath(t *testing.T) {
+	t.Parallel()
+
+	var tests = []struct {
+		filepath    string
+		destination string
+		expectError bool
+	}{
+		{
+			filepath:    "file",
+			destination: "/tmp",
+			expectError: false,
+		},
+		{
+			filepath:    "subdir/../subdir/file",
+			destination: "/tmp",
+			expectError: false,
+		},
+
+		{
+			filepath:    "../../../file",
+			destination: "/tmp",
+			expectError: true,
+		},
+		{
+			filepath:    "./././file",
+			destination: "/tmp",
+			expectError: false,
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.filepath, func(t *testing.T) {
+			t.Parallel()
+
+			if tt.expectError {
+				require.Error(t, sanitizeExtractPath(tt.filepath, tt.destination), tt.filepath)
+			} else {
+				require.NoError(t, sanitizeExtractPath(tt.filepath, tt.destination), tt.filepath)
+			}
+		})
+	}
+}
+
+func Test_sanitizePermissions(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		testCaseName         string
+		givenFilePermissions fs.FileMode
+	}{
+		{
+			testCaseName:         "directory, valid permissions",
+			givenFilePermissions: fs.ModeDir | 0755,
+		},
+		{
+			testCaseName:         "directory, invalid permissions (group has write)",
+			givenFilePermissions: fs.ModeDir | 0775,
+		},
+		{
+			testCaseName:         "directory, invalid permissions (public has write)",
+			givenFilePermissions: fs.ModeDir | 0757,
+		},
+		{
+			testCaseName:         "directory, invalid permissions (everyone has write)",
+			givenFilePermissions: fs.ModeDir | 0777,
+		},
+		{
+			testCaseName:         "executable file, valid permissions",
+			givenFilePermissions: 0755,
+		},
+		{
+			testCaseName:         "executable file, invalid permissions (group has write)",
+			givenFilePermissions: 0775,
+		},
+		{
+			testCaseName:         "executable file, invalid permissions (public has write)",
+			givenFilePermissions: 0757,
+		},
+		{
+			testCaseName:         "executable file, invalid permissions (everyone has write)",
+			givenFilePermissions: 0777,
+		},
+		{
+			testCaseName:         "non-executable file, valid permissions",
+			givenFilePermissions: 0644,
+		},
+		{
+			testCaseName:         "non-executable file, invalid permissions (group has write)",
+			givenFilePermissions: 0664,
+		},
+		{
+			testCaseName:         "non-executable file, invalid permissions (public has write)",
+			givenFilePermissions: 0646,
+		},
+		{
+			testCaseName:         "non-executable file, invalid permissions (everyone has write)",
+			givenFilePermissions: 0666,
+		},
+	}
+
+	for _, tt := range testCases {
+		tt := tt
+		t.Run(tt.testCaseName, func(t *testing.T) {
+			t.Parallel()
+
+			// Create a temp file to extract a FileInfo from it with tt.givenFilePermissions
+			tmpDir := t.TempDir()
+			pathUnderTest := filepath.Join(tmpDir, ulid.New())
+			if tt.givenFilePermissions.IsDir() {
+				require.NoError(t, os.MkdirAll(pathUnderTest, tt.givenFilePermissions))
+			} else {
+				f, err := os.OpenFile(pathUnderTest, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, tt.givenFilePermissions)
+				require.NoError(t, err)
+				require.NoError(t, f.Close())
+			}
+			fileInfo, err := os.Stat(pathUnderTest)
+			require.NoError(t, err)
+
+			sanitizedPermissions := sanitizePermissions(fileInfo)
+
+			// Confirm no group write
+			require.True(t, sanitizedPermissions&0020 == 0)
+
+			// Confirm no public write
+			require.True(t, sanitizedPermissions&0002 == 0)
+
+			// Confirm type is set correctly
+			require.Equal(t, tt.givenFilePermissions.Type(), sanitizedPermissions.Type())
+
+			// Confirm owner permissions are unmodified
+			var ownerBits fs.FileMode = 0700
+			if runtime.GOOS == "windows" {
+				// Windows doesn't have executable bit
+				ownerBits = 0600
+			}
+			require.Equal(t, tt.givenFilePermissions&ownerBits, sanitizedPermissions&ownerBits)
+		})
+	}
+}
+
 func TestTidyLibrary(t *testing.T) {
 	t.Parallel()
 

go.mod

@@ -14,7 +14,7 @@ require (
 	github.com/gorilla/websocket v1.4.2
 	github.com/groob/plist v0.0.0-20190114192801-a99fbe489d03
 	github.com/knightsc/system_policy v1.1.1-0.20211029142728-5f4c0d5419cc
-	github.com/kolide/kit v0.0.0-20221107170827-fb85e3d59eab
+	github.com/kolide/kit v0.0.0-20240411131714-94dd1939cf50
 	github.com/kolide/krypto v0.1.1-0.20231229162826-db516b7e0121
 	github.com/mat/besticon v3.9.0+incompatible
 	github.com/mattn/go-sqlite3 v1.14.19