Parcourir la source

Make the socket directory with the umask instead of chmodding it afterwards

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;
a chmod through a descriptor that checks the owner is not enough either,
since a root-owned directory moved into place passes that check. On
macOS the default parent, /var/run, is writable by group daemon.

- Mkdir runs under a 0022 umask, as the bind runs under 0077, so the directory is born 0755 and nothing is fixed up afterwards

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Matt Richardson il y a 1 jour
Parent
commit
6a5cd6713a
1 fichiers modifiés avec 19 ajouts et 15 suppressions
  1. 19 15
      localapi/platform_unix.go

+ 19 - 15
localapi/platform_unix.go

@@ -71,14 +71,18 @@ func listen(path string, acc Access) (net.Listener, error) {
 	// exists, and an operator who names a deeper path creates it; a missing parent is a
 	// mistake to report, not a tree to build. A directory made here stays when the
 	// socket goes: an empty one costs nothing, and the next start wants it again.
-	// Mkdir applies the umask, so the 0755 is pinned afterwards, as the socket's mode
-	// 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)
-		}
-	case !errors.Is(err, os.ErrExist):
+	//
+	// The mode comes from the umask around the call, never from a chmod afterwards: a
+	// chmod by path could land on whatever somebody who can write the parent swapped in
+	// between the two calls, and a chmod through a descriptor on a root-owned directory
+	// they moved there, since root owns both. Born 0755 there is nothing to fix up.
+	// The umask is process wide and the window is one syscall; see umaskMu.
+	umaskMu.Lock()
+	old := syscall.Umask(0022)
+	err := os.Mkdir(dir, 0755)
+	syscall.Umask(old)
+	umaskMu.Unlock()
+	if err != nil && !errors.Is(err, os.ErrExist) {
 		return nil, fmt.Errorf("could not create %s: %w", dir, err)
 	}
 
@@ -97,7 +101,7 @@ func listen(path string, acc Access) (net.Listener, error) {
 	// the node is never looser than 0600, however briefly, whatever mode it ends up at;
 	// see umaskMu.
 	umaskMu.Lock()
-	old := syscall.Umask(0077)
+	old = syscall.Umask(0077)
 	ln, err := net.Listen(network, path)
 	syscall.Umask(old)
 	umaskMu.Unlock()
@@ -321,12 +325,12 @@ const sunPathMax = 104
 // with a bare EINVAL, so the usable limit is one byte short of the buffer.
 const maxSocketPath = sunPathMax - 1
 
-// umaskMu serialises setting the process umask around one syscall: the umask is process
-// wide, and two binds at once, a reload landing during startup, would otherwise
-// interleave their save and restore and leave the daemon's umask at one of ours. It is
-// held for the syscall alone. Another goroutine creating a file in that moment gets the
-// umask of the window rather than the process's own; nothing creates files on a hot
-// path.
+// umaskMu serialises the two places the process umask is set around one syscall: the
+// umask is process wide, and two endpoints binding at once, a reload landing during
+// startup, would otherwise interleave their save and restore and leave the daemon's
+// umask at one of ours. It is held for the syscall alone. Another goroutine creating a
+// file in that moment gets the umask of the window, 0022 or 0077, rather than the
+// process's own; nothing creates files on a hot path.
 var umaskMu sync.Mutex
 
 func pathTooLong(path string) bool { return len(path) > maxSocketPath }