Merge pull request #1151 from frazew/pull-preserve-mode

oci: preserve dir mode when extracting tar layers

Author: stefanprodan

oci/diff_test.go

@@ -63,3 +63,40 @@ func TestClient_Diff(t *testing.T) {
 	g.Expect(err).To(HaveOccurred())
 	g.Expect(err).To(MatchError("the remote artifact contents differs from the local one"))
 }
+
+func TestClient_PushPullDiff_RoundTrip(t *testing.T) {
+	g := NewWithT(t)
+	ctx := context.Background()
+	c := NewClient(DefaultOptions())
+	tag := "v0.0.1"
+	repo := "test-push" + randStringRunes(5)
+
+	url := fmt.Sprintf("%s/%s:%s", dockerReg, repo, tag)
+	metadata := Metadata{
+		Source:   "github.com/fluxcd/flux2",
+		Revision: "rev",
+	}
+
+	testDir := "testdata/artifact"
+	_, err := c.Push(ctx, url, testDir, WithPushMetadata(metadata))
+	g.Expect(err).ToNot(HaveOccurred())
+
+	err = c.Diff(ctx, url, testDir, nil)
+	g.Expect(err).ToNot(HaveOccurred())
+
+	testDirStat, err := os.Stat(testDir)
+	g.Expect(err).ToNot(HaveOccurred())
+
+	tmpPullDir, err := os.MkdirTemp("", "oci")
+	g.Expect(err).ToNot(HaveOccurred())
+	defer os.RemoveAll(tmpPullDir)
+
+	err = os.Chmod(tmpPullDir, testDirStat.Mode())
+	g.Expect(err).ToNot(HaveOccurred())
+
+	_, err = c.Pull(ctx, url, tmpPullDir)
+	g.Expect(err).ToNot(HaveOccurred())
+
+	err = c.Diff(ctx, url, tmpPullDir, nil)
+	g.Expect(err).ToNot(HaveOccurred())
+}

tar/tar.go

@@ -177,7 +177,12 @@ func Untar(r io.Reader, dir string, inOpts ...TarOption) (err error) {
 				}
 			}
 		case mode.IsDir():
-			if err := os.MkdirAll(abs, 0o750); err != nil {
+			// Ensure the owner can always traverse, read, and write
+			// into extracted directories, regardless of what the tar
+			// header claims. This prevents crafted archives from
+			// creating directories that block cleanup or future writes.
+			dirPerm := mode.Perm() | 0o700
+			if err := os.MkdirAll(abs, dirPerm); err != nil {
 				return err
 			}
 			madeDir[abs] = true

tar/tar_test.go

@@ -35,6 +35,7 @@ type untarTestCase struct {
 	content         []byte
 	wantErr         string
 	maxUntarSize    int
+	fileMode        int64
 }
 
 func TestUntar(t *testing.T) {
@@ -178,6 +179,40 @@ func TestUntar(t *testing.T) {
 	}
 }
 
+func TestUntarDirectoryPermissions(t *testing.T) {
+	testDirName := "test-dir"
+
+	f, err := createTestTar(untarTestCase{
+		fileName: testDirName + "/", // from tar.Header: a trailing slash makes the entry a TypeDir
+		fileMode: 0o555,
+		content:  nil,
+	})
+	if err != nil {
+		t.Fatalf("creating test tar: %v", err)
+	}
+
+	targetDir := t.TempDir()
+
+	if err := Untar(f, targetDir); err != nil {
+		t.Fatalf("untar: %v", err)
+	}
+
+	fullPath := filepath.Join(targetDir, testDirName)
+	fi, err := os.Lstat(fullPath)
+	if err != nil {
+		t.Errorf("stat %q: %v", fullPath, err)
+	}
+
+	if !fi.Mode().IsDir() {
+		t.Fatalf("%q: not a directory", fullPath)
+	}
+
+	ownerPerm := fi.Mode().Perm() & 0o700
+	if ownerPerm != 0o700 {
+		t.Errorf("the owner must always be able to traverse, read, and write extracted directories")
+	}
+}
+
 func Fuzz_Untar(f *testing.F) {
 	tf, err := createTestTar(untarTestCase{
 		name:     "file at root",
@@ -211,10 +246,15 @@ func createTestTar(tt untarTestCase) (*os.File, error) {
 	gzw := gzip.NewWriter(f)
 	writer := tar.NewWriter(gzw)
 
+	fileMode := tt.fileMode
+	if fileMode == 0 {
+		fileMode = 0o777
+	}
+
 	writer.WriteHeader(&tar.Header{
 		Name: tt.fileName,
 		Size: int64(len(tt.content)),
-		Mode: 0o777,
+		Mode: fileMode,
 	})
 
 	writer.Write(tt.content)