Bind the daemon unix sockets at their final mode so no chmod follows the bind

client/cmd/service_allow_group_unix.go

@@ -21,21 +21,31 @@ import (
 const (
 	openSocketMode       os.FileMode = 0666
 	restrictedSocketMode os.FileMode = 0660
+	ownerOnlySocketMode  os.FileMode = 0600
 )
 
-// umaskOwnerOnly masks every permission bit except the owner's, so a file
-// created under it is 0600 whatever the process umask happens to be.
-const umaskOwnerOnly = 0o177
-
-// listenUnixPrivate binds a Unix socket that only its owner can connect to,
-// whatever umask the service manager started the daemon with. applySocketAccess
-// widens it afterwards to exactly what the configuration asks for.
+// listenUnixPrivate binds a Unix socket at the narrowest mode the configuration
+// allows, so it is never briefly more open than it should be. A Unix socket's
+// mode is checked at connect() rather than at accept(), so a socket that is
+// momentarily world-writable can be connected to before the daemon narrows it,
+// and that caller stays connected afterwards.
 //
-// The umask is process-wide, so it is restored before returning and the window
-// is kept to the bind itself. Nothing else creates files during daemon startup:
-// this runs before the server and its goroutines exist.
-func listenUnixPrivate(address string) (net.Listener, error) {
-	previous := syscall.Umask(umaskOwnerOnly)
+// Where no group is configured the final mode is reached at the bind itself and
+// nothing touches the path afterwards, which is what keeps the historical
+// unrestricted socket free of a chmod that could follow a symlink another
+// account planted. A restricted socket binds owner-only and applySocketAccess
+// hands it to the group, under the checks that step carries.
+//
+// The umask is process-wide, so it is restored immediately and the window is
+// the bind alone. Nothing else creates files at this point in startup: the
+// server and its goroutines do not exist yet.
+func listenUnixPrivate(address string, allowed []string) (net.Listener, error) {
+	mode := openSocketMode
+	if len(allowed) > 0 {
+		mode = ownerOnlySocketMode
+	}
+
+	previous := syscall.Umask(int(^mode & 0o777))
 	listener, err := net.Listen("unix", address)
 	syscall.Umask(previous)
 	if err != nil {
@@ -78,15 +88,20 @@ func checkAllowGroupSet(principals []string) error {
 	return nil
 }
 
-// applySocketAccess sets the access the socket grants to other accounts: the
-// allowed group at 0660, or every local account at 0666 when no group is
-// configured.
+// applySocketAccess hands a socket to the configured group at 0660. It does
+// nothing when no group is configured: listenUnixPrivate already bound such a
+// socket at its final mode, and touching the path again would only add a chmod
+// that could follow something another account put there.
 //
 // The owner is left untouched so a daemon running as an ordinary user, as in a
 // rootless container, keeps access to the socket it created. The group is set
 // before the mode is widened, so the window between the two is one where the
 // group has no access rather than one where it has access it should not.
 func applySocketAccess(path string, principals []string) error {
+	if len(principals) == 0 {
+		return nil
+	}
+
 	// The listener just bound this path, so anything else standing there now is
 	// something another account substituted. Checked before either call below,
 	// neither of which should ever act on a name the daemon did not create.
@@ -94,13 +109,6 @@ func applySocketAccess(path string, principals []string) error {
 		return err
 	}
 
-	if len(principals) == 0 {
-		if err := os.Chmod(path, openSocketMode); err != nil {
-			return fmt.Errorf("set mode %#o: %w", openSocketMode, err)
-		}
-		return nil
-	}
-
 	principal, err := principalOfKind(principals[0], ipcauth.KindGID)
 	if err != nil {
 		return err

client/cmd/service_allow_group_unix_test.go

@@ -85,11 +85,15 @@ func TestCheckAllowGroupSet_SingleGroupOnly(t *testing.T) {
 }
 
 func TestApplySocketAccess(t *testing.T) {
-	t.Run("no principals leaves the socket open", func(t *testing.T) {
+	// With nothing configured the socket already carries its final mode from
+	// the bind, so this must not touch the path at all: a chmod here is the one
+	// that could follow a symlink another account planted.
+	t.Run("no principals leaves the socket alone", func(t *testing.T) {
 		path := listenTestSocket(t)
+		before := socketMode(t, path)
 
 		require.NoError(t, applySocketAccess(path, nil))
-		assert.Equal(t, os.FileMode(0666), socketMode(t, path))
+		assert.Equal(t, before, socketMode(t, path))
 	})
 
 	t.Run("a principal hands the socket to that group", func(t *testing.T) {
@@ -129,9 +133,9 @@ func TestApplySocketAccess(t *testing.T) {
 
 		gid := strconv.Itoa(os.Getgid())
 		require.Error(t, applySocketAccess(link, []string{"gid:" + gid}))
-		require.Error(t, applySocketAccess(link, nil), "the open path must not follow it either")
+		require.NoError(t, applySocketAccess(link, nil), "with nothing configured there is nothing to apply")
 
-		// The substituted target keeps the mode it was created with.
+		// Either way the substituted target keeps the mode it was created with.
 		info, err := os.Stat(target)
 		require.NoError(t, err)
 		assert.Equal(t, os.FileMode(0600), info.Mode().Perm())
@@ -160,24 +164,37 @@ func TestApplySocketAccess(t *testing.T) {
 }
 
 // The kernel checks a Unix socket's mode at connect(), not at accept(), so a
-// socket that is briefly world-writable can be connected to before the daemon
-// narrows it, and the caller stays connected afterwards. Binding under a
-// restrictive umask closes that window whatever umask the service manager used.
-func TestListenUnixPrivate_IgnoresAPermissiveUmask(t *testing.T) {
+// socket that is briefly wider than intended can be connected to before the
+// daemon narrows it, and that caller stays connected afterwards. The bind must
+// therefore land on the final mode, whatever umask the service manager used.
+func TestListenUnixPrivate_BindsAtTheFinalMode(t *testing.T) {
 	previous := syscall.Umask(0)
 	t.Cleanup(func() { syscall.Umask(previous) })
 
-	dir, err := os.MkdirTemp("", "nb-sock")
-	require.NoError(t, err)
-	t.Cleanup(func() { assert.NoError(t, os.RemoveAll(dir)) })
+	tests := map[string]struct {
+		allowed []string
+		want    os.FileMode
+	}{
+		"unrestricted binds open, so nothing has to widen it later": {want: 0666},
+		"restricted binds owner-only, for applySocketAccess to hand to the group": {
+			allowed: []string{"gid:0"}, want: 0600,
+		},
+	}
 
-	path := filepath.Join(dir, "d.sock")
-	listener, err := listenUnixPrivate(path)
-	require.NoError(t, err)
-	t.Cleanup(func() { assert.NoError(t, listener.Close()) })
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			dir, err := os.MkdirTemp("", "nb-sock")
+			require.NoError(t, err)
+			t.Cleanup(func() { assert.NoError(t, os.RemoveAll(dir)) })
 
-	assert.Equal(t, os.FileMode(0600), socketMode(t, path),
-		"the socket must be owner-only as bound, before any restriction is applied")
+			path := filepath.Join(dir, "d.sock")
+			listener, err := listenUnixPrivate(path, tc.allowed)
+			require.NoError(t, err)
+			t.Cleanup(func() { assert.NoError(t, listener.Close()) })
+
+			assert.Equal(t, tc.want, socketMode(t, path))
+		})
+	}
 
 	// And the process umask is left as it was found.
 	restored := syscall.Umask(0)

client/cmd/service_allow_group_windows.go

@@ -62,7 +62,7 @@ func applySocketAccess(path string, principals []string) error {
 // listenUnixPrivate binds a Unix socket. Windows has no umask and no mode on
 // these sockets, so binding is all there is to do; a configured restriction is
 // refused by applySocketAccess before the daemon serves.
-func listenUnixPrivate(address string) (net.Listener, error) {
+func listenUnixPrivate(address string, _ []string) (net.Listener, error) {
 	return net.Listen("unix", address)
 }
 

client/cmd/service_socket.go

@@ -52,12 +52,11 @@ func listenOnAddress(addr string, allowed []string) (*socketListener, error) {
 		removeStaleUnixSocket(address)
 
 		// A Unix socket accepts connections the moment it is bound, and the
-		// kernel checks its mode at connect() rather than at accept(). Creating
-		// it owner-only closes the window between the bind and applySocketAccess:
-		// without this, a permissive umask leaves the socket open to everybody
-		// for that interval, and a caller that got in stays connected after the
-		// mode is narrowed.
-		listener, err := listenUnixPrivate(address)
+		// kernel checks its mode at connect() rather than at accept(), so the
+		// socket is bound at the narrowest mode the configuration allows rather
+		// than bound wide and narrowed after: a caller that gets in during such
+		// a window stays connected once the mode changes.
+		listener, err := listenUnixPrivate(address, allowed)
 		if err != nil {
 			return nil, err
 		}