fix(executor): chown the file created by createExcl through its open handle, not by name after the close (GO-003)

commit: f9fc79880a98d918ca56266355e3b08808d75c01
author: brice · 9d ago · 2026-10-12 20:10 UTC
parent: 63269d4
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

2 files · +32 -6

M internal/executor/filecmd.go +9 -6


@@ -230,7 +230,10 @@ func readText(root *os.Root, name string) (text string, ok bool, err error) {
230	230		// createExcl creates name in root with the content, failing if it exists (an
231	231		// existing entry, a symbolic link included, is removed first, never followed),
232	232		// and gives it to owner when set. The creation uses O_EXCL\|O_NOFOLLOW, so
233		-	// something planted between the removal and the creation makes it fail.
	233	+	// something planted between the removal and the creation makes it fail. The
	234	+	// owner is set on the open file, not by name: the user of the steps owns the
	235	+	// directory and could swap the name for something else between the close and a
	236	+	// chown by name.
234	237		func createExcl(root os.Root, name string, content []byte, perm os.FileMode, owner [2]int) error {
235	238		if err := root.Remove(name); err != nil && !errors.Is(err, os.ErrNotExist) {
236	239		return err

@@ -243,13 +246,13 @@ func createExcl(root *os.Root, name string, content []byte, perm os.FileMode, ow
243	246		_ = fh.Close()
244	247		return err
245	248		}
246		-	if err := fh.Close(); err != nil {
247		-	return err
248		-	}
249	249		if owner != nil {
250		-	return root.Lchown(name, owner[0], owner[1])
	250	+	if err := fh.Chown(owner[0], owner[1]); err != nil {
	251	+	_ = fh.Close()
	252	+	return err
	253	+	}
251	254		}
252		-	return nil
	255	+	return fh.Close()
253	256		}
254	257
255	258		// processAddPath is AddPathFileCommand.ProcessCommand (FileCommandManager.cs:134).

 // createExcl creates name in root with the content, failing if it exists (an
 // existing entry, a symbolic link included, is removed first, never followed),
 // and gives it to owner when set. The creation uses O_EXCL|O_NOFOLLOW, so
-// something planted between the removal and the creation makes it fail.
+// something planted between the removal and the creation makes it fail. The
+// owner is set on the open file, not by name: the user of the steps owns the
+// directory and could swap the name for something else between the close and a
+// chown by name.
 func createExcl(root *os.Root, name string, content []byte, perm os.FileMode, owner *[2]int) error {
 	if err := root.Remove(name); err != nil && !errors.Is(err, os.ErrNotExist) {
 		return err
 		_ = fh.Close()
 		return err
 	}
-	if err := fh.Close(); err != nil {
-		return err
-	}
 	if owner != nil {
-		return root.Lchown(name, owner[0], owner[1])
+		if err := fh.Chown(owner[0], owner[1]); err != nil {
+			_ = fh.Close()
+			return err
+		}
 	}
-	return nil
+	return fh.Close()
 }
 // processAddPath is AddPathFileCommand.ProcessCommand (FileCommandManager.cs:134).

M internal/executor/filecmd_test.go +23 -0


@@ -353,3 +353,26 @@ func TestCreateExclReplacesAndNeverFollows(t *testing.T) {
353	353		t.Fatal("still a link")
354	354		}
355	355		}
	356	+
	357	+	// The owner is applied to the open file (GO-003): chown by name after the close
	358	+	// would follow a name that the user of the steps could have swapped. Own uid and
	359	+	// gid are accepted by an unprivileged process, so the path runs here.
	360	+	func TestCreateExclChownsTheOpenFile(t *testing.T) {
	361	+	dir := t.TempDir()
	362	+	root, err := os.OpenRoot(dir)
	363	+	if err != nil {
	364	+	t.Fatal(err)
	365	+	}
	366	+	defer func() { _ = root.Close() }()
	367	+	owner := &[2]int{os.Getuid(), os.Getgid()}
	368	+	if err := createExcl(root, "f", []byte("x"), 0o644, owner); err != nil {
	369	+	t.Fatal(err)
	370	+	}
	371	+	// A uid that this process may not give files to makes the creation fail, and
	372	+	// the half-made file is not reported as success.
	373	+	if os.Getuid() != 0 {
	374	+	if err := createExcl(root, "g", []byte("x"), 0o644, &[2]int{0, 0}); err == nil {
	375	+	t.Fatal("chown to root by an unprivileged process succeeded")
	376	+	}
	377	+	}
	378	+	}


@@ -353,3 +353,26 @@ func TestCreateExclReplacesAndNeverFollows(t *testing.T) {
353	t.Fatal("still a link")	353		t.Fatal("still a link")
354	}	354		}
355	}	355		}
		356	+
		357	+	// The owner is applied to the open file (GO-003): chown by name after the close
		358	+	// would follow a name that the user of the steps could have swapped. Own uid and
		359	+	// gid are accepted by an unprivileged process, so the path runs here.
		360	+	func TestCreateExclChownsTheOpenFile(t *testing.T) {
		361	+	dir := t.TempDir()
		362	+	root, err := os.OpenRoot(dir)
		363	+	if err != nil {
		364	+	t.Fatal(err)
		365	+	}
		366	+	defer func() { _ = root.Close() }()
		367	+	owner := &[2]int{os.Getuid(), os.Getgid()}
		368	+	if err := createExcl(root, "f", []byte("x"), 0o644, owner); err != nil {
		369	+	t.Fatal(err)
		370	+	}
		371	+	// A uid that this process may not give files to makes the creation fail, and
		372	+	// the half-made file is not reported as success.
		373	+	if os.Getuid() != 0 {
		374	+	if err := createExcl(root, "g", []byte("x"), 0o644, &[2]int{0, 0}); err == nil {
		375	+	t.Fatal("chown to root by an unprivileged process succeeded")
		376	+	}
		377	+	}
		378	+	}