Merge pull request #2291 from rhamitarora/rhamitarora/ARO-26358-codeql-fix

NO-JIRA: Fix tar slip path traversal in codesign archive extraction

Author: openshift-merge-bot[bot]

pkg/cli/admin/internal/codesign/machoresign.go

@@ -4,10 +4,12 @@ import (
 	"archive/tar"
 	"compress/gzip"
 	"debug/macho"
+	"fmt"
 	"io"
 	"io/fs"
 	"os"
 	"path/filepath"
+	"strings"
 )
 
 func ResignMacho(path string, asArchive bool, command string, links []string) error {
@@ -125,6 +127,7 @@ func extractTarToTmpAndSign(path string) (string, error) {
 	if err != nil {
 		return "", err
 	}
+	cleanTempDir := filepath.Clean(tempDir)
 
 	gzipFile, err := os.Open(path)
 	if err != nil {
@@ -155,6 +158,15 @@ func extractTarToTmpAndSign(path string) (string, error) {
 
 		tempExtractionPath := filepath.Clean(filepath.Join(tempDir, header.Name))
 
+		// Guard against tar slip: ensure the resolved path stays within tempDir.
+		// filepath.Clean/Join normalize but do not constrain the result, so an
+		// entry name containing ".." could escape the extraction directory. The
+		// safe branch is determined solely by strings.HasPrefix so that static
+		// analyzers recognize this as a sanitizer.
+		if !strings.HasPrefix(tempExtractionPath, cleanTempDir+string(os.PathSeparator)) {
+			return "", fmt.Errorf("invalid archive entry %q: escapes extraction directory", header.Name)
+		}
+
 		switch header.Typeflag {
 		case tar.TypeReg:
 			f, err := os.OpenFile(tempExtractionPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, os.FileMode(header.Mode).Perm())

pkg/cli/admin/internal/codesign/machoresign_test.go

@@ -0,0 +1,100 @@
+package codesign
+
+import (
+	"archive/tar"
+	"compress/gzip"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// writeTarGz writes a gzip-compressed tar archive at path containing the given
+// regular-file entries keyed by entry name.
+func writeTarGz(t *testing.T, path string, entries map[string][]byte) {
+	t.Helper()
+
+	f, err := os.Create(path)
+	if err != nil {
+		t.Fatalf("creating archive: %v", err)
+	}
+	defer f.Close()
+
+	gw := gzip.NewWriter(f)
+	tw := tar.NewWriter(gw)
+
+	for name, data := range entries {
+		hdr := &tar.Header{
+			Name:     name,
+			Mode:     0644,
+			Size:     int64(len(data)),
+			Typeflag: tar.TypeReg,
+		}
+		if err := tw.WriteHeader(hdr); err != nil {
+			t.Fatalf("writing header %q: %v", name, err)
+		}
+		if _, err := tw.Write(data); err != nil {
+			t.Fatalf("writing data %q: %v", name, err)
+		}
+	}
+
+	if err := tw.Close(); err != nil {
+		t.Fatalf("closing tar writer: %v", err)
+	}
+	if err := gw.Close(); err != nil {
+		t.Fatalf("closing gzip writer: %v", err)
+	}
+}
+
+func TestExtractTarToTmpAndSign_RejectsPathTraversal(t *testing.T) {
+	dir := t.TempDir()
+	archivePath := filepath.Join(dir, "malicious.tar.gz")
+	sentinel := filepath.Join(dir, "escaped.txt")
+
+	writeTarGz(t, archivePath, map[string][]byte{
+		"../escaped.txt": []byte("owned"),
+	})
+
+	tempDir, err := extractTarToTmpAndSign(archivePath)
+	if tempDir != "" {
+		defer os.RemoveAll(tempDir)
+	}
+	if err == nil {
+		t.Fatal("expected an error for path-traversal entry, got nil")
+	}
+	if tempDir != "" {
+		t.Fatalf("expected empty tempDir on rejection, got %q", tempDir)
+	}
+
+	if _, statErr := os.Stat(sentinel); statErr == nil {
+		t.Fatalf("path traversal succeeded: file written outside extraction dir at %s", sentinel)
+	}
+}
+
+func TestExtractTarToTmpAndSign_ExtractsRegularFile(t *testing.T) {
+	dir := t.TempDir()
+	archivePath := filepath.Join(dir, "benign.tar.gz")
+
+	writeTarGz(t, archivePath, map[string][]byte{
+		"tool": []byte("not a macho binary"),
+	})
+
+	tempDir, err := extractTarToTmpAndSign(archivePath)
+	if tempDir != "" {
+		defer os.RemoveAll(tempDir)
+	}
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if tempDir == "" {
+		t.Fatal("expected a non-empty tempDir on success")
+	}
+	if !strings.HasPrefix(filepath.Base(tempDir), "oc-release-extract-") {
+		t.Fatalf("unexpected tempDir name: %q", tempDir)
+	}
+
+	extracted := filepath.Join(tempDir, "tool")
+	if _, statErr := os.Stat(extracted); statErr != nil {
+		t.Fatalf("expected extracted file at %s: %v", extracted, statErr)
+	}
+}

pkg/cli/image/archive/archive.go

@@ -141,6 +141,19 @@ func unpackLayer(dest string, layer io.Reader, options *TarOptions) (size int64,
 			}
 		}
 
+		// Ensure the resolved destination stays within dest before performing any
+		// filesystem operation (including parent-directory creation below), so a
+		// malicious entry containing '..' cannot escape the extraction directory.
+		path := filepath.Join(dest, hdr.Name)
+		rel, err := filepath.Rel(dest, path)
+		if err != nil {
+			return 0, err
+		}
+		// Note as these operations are platform specific, so must the slash be.
+		if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
+			return 0, breakoutError(fmt.Errorf("%q is outside of %q", hdr.Name, dest))
+		}
+
 		// Windows does not support filenames with colons in them. Ignore
 		// these files. This is not a problem though (although it might
 		// appear that it is). Let's suppose a client is running docker pull.
@@ -201,16 +214,6 @@ func unpackLayer(dest string, layer io.Reader, options *TarOptions) (size int64,
 			}
 		}
 
-		path := filepath.Join(dest, hdr.Name)
-		rel, err := filepath.Rel(dest, path)
-		if err != nil {
-			return 0, err
-		}
-
-		// Note as these operations are platform specific, so must the slash be.
-		if strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
-			return 0, breakoutError(fmt.Errorf("%q is outside of %q", hdr.Name, dest))
-		}
 		base := filepath.Base(path)
 
 		if strings.HasPrefix(base, archive.WhiteoutPrefix) {