Преглед изворни кода

Set the new socket directory's mode through a descriptor, not its path

listen made the directory and then chmodded it by path, before the
directory check ran. Whoever can write the parent could swap a link in
between the two calls, and a root daemon would set 0755 on its target. On
macOS the default parent, /var/run, is writable by group daemon.

- pinDirMode opens the directory with O_DIRECTORY|O_NOFOLLOW, checks this process owns it, and fchmods it
- test that a link in the directory's place is refused and its target keeps its mode

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Matt Richardson пре 23 часа
родитељ
комит
a91a232495
2 измењених фајлова са 53 додато и 2 уклоњено
  1. 29 2
      localapi/platform_unix.go
  2. 24 0
      localapi/platform_unix_test.go

+ 29 - 2
localapi/platform_unix.go

@@ -74,8 +74,8 @@ func listen(path string, acc Access) (net.Listener, error) {
 	// is; only on a directory this call created.
 	switch err := os.Mkdir(dir, 0755); {
 	case err == nil:
-		if err := os.Chmod(dir, 0755); err != nil {
-			return nil, fmt.Errorf("could not set the mode of %s: %w", dir, err)
+		if err := pinDirMode(dir, 0755); err != nil {
+			return nil, err
 		}
 	case !errors.Is(err, os.ErrExist):
 		return nil, fmt.Errorf("could not create %s: %w", dir, err)
@@ -159,6 +159,33 @@ func describeNode(fi os.FileInfo) string {
 	}
 }
 
+// pinDirMode sets the mode of the directory listen has just made, through a descriptor
+// rather than by path. Whoever can write the parent could rename the new directory
+// away and leave a link in its place between the Mkdir and a chmod by path, which a
+// root daemon would then follow to set the mode on the link's target. The open does
+// not follow a link, and the directory it finds must be one this process owns, or it
+// is not the one just made and is refused.
+func pinDirMode(dir string, mode os.FileMode) error {
+	f, err := os.OpenFile(dir, os.O_RDONLY|syscall.O_DIRECTORY|syscall.O_NOFOLLOW, 0)
+	if err != nil {
+		return fmt.Errorf("could not open %s to set its mode: %w", dir, err)
+	}
+	defer f.Close()
+
+	fi, err := f.Stat()
+	if err != nil {
+		return fmt.Errorf("could not set the mode of %s: %w", dir, err)
+	}
+	if uid, ok := owner(fi); !ok || uid != uint32(os.Geteuid()) {
+		return fmt.Errorf("%s was replaced after it was created, refusing to change it", dir)
+	}
+
+	if err := f.Chmod(mode); err != nil {
+		return fmt.Errorf("could not set the mode of %s: %w", dir, err)
+	}
+	return nil
+}
+
 // checkSocketDir refuses to serve from a directory that somebody other than the user
 // nebula runs as, or root, could change. The socket's mode and group say who may connect
 // to it; what makes the path they connect to the daemon's is the directory, since

+ 24 - 0
localapi/platform_unix_test.go

@@ -427,3 +427,27 @@ func TestChmodSocketRefusesWhatIsNotOurSocket(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, os.FileMode(0660), fi.Mode().Perm())
 }
+
+// TestPinDirModeDoesNotFollowALink: the mode of a directory listen made is set through a
+// descriptor opened without following links, so a link put in its place is refused and
+// its target keeps its mode.
+func TestPinDirModeDoesNotFollowALink(t *testing.T) {
+	base := t.TempDir()
+
+	dir := filepath.Join(base, "made")
+	require.NoError(t, os.Mkdir(dir, 0700))
+	require.NoError(t, pinDirMode(dir, 0755))
+	fi, err := os.Stat(dir)
+	require.NoError(t, err)
+	assert.Equal(t, os.FileMode(0755), fi.Mode().Perm())
+
+	target := filepath.Join(base, "target")
+	require.NoError(t, os.WriteFile(target, nil, 0600))
+	link := filepath.Join(base, "link")
+	require.NoError(t, os.Symlink(target, link))
+
+	require.Error(t, pinDirMode(link, 0755))
+	fi, err = os.Stat(target)
+	require.NoError(t, err)
+	assert.Equal(t, os.FileMode(0600), fi.Mode().Perm(), "the link's target keeps its mode")
+}