diff --git a/client/cmd/service_allow_group_unix.go b/client/cmd/service_allow_group_unix.go index 946742aa3..24c821755 100644 --- a/client/cmd/service_allow_group_unix.go +++ b/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 diff --git a/client/cmd/service_allow_group_unix_test.go b/client/cmd/service_allow_group_unix_test.go index 0928a895c..7f34dd035 100644 --- a/client/cmd/service_allow_group_unix_test.go +++ b/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) diff --git a/client/cmd/service_allow_group_windows.go b/client/cmd/service_allow_group_windows.go index b7ba00172..ae1a1763c 100644 --- a/client/cmd/service_allow_group_windows.go +++ b/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) } diff --git a/client/cmd/service_socket.go b/client/cmd/service_socket.go index 080b8893c..399125f51 100644 --- a/client/cmd/service_socket.go +++ b/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 }