diff --git a/client/cmd/vnc_agent.go b/client/cmd/vnc_agent.go index e90c434e5..298ddaae8 100644 --- a/client/cmd/vnc_agent.go +++ b/client/cmd/vnc_agent.go @@ -55,12 +55,14 @@ var vncAgentCmd = &cobra.Command{ // the listening socket: keeps a post-auth bug in the encoder / // input / capture paths confined to the user's own privileges // rather than escalating to host root, and makes the daemon's - // LOCAL_PEERCRED check see the right uid. No-op on Windows - // (both processes run as SYSTEM) and when --target-uid is 0. - if vncAgentTargetUID != 0 { - if err := dropAgentPrivileges(vncAgentTargetUID); err != nil { - return fmt.Errorf("drop privileges to uid %d: %w", vncAgentTargetUID, err) - } + // LOCAL_PEERCRED check see the right uid. No-op on Windows, where + // both processes run as SYSTEM. + // + // Called unconditionally: a missing or zero --target-uid is exactly + // the case the Darwin implementation refuses, and skipping the call + // for it would leave the agent running as root instead. + if err := dropAgentPrivileges(vncAgentTargetUID); err != nil { + return fmt.Errorf("drop privileges to uid %d: %w", vncAgentTargetUID, err) } if err := os.Remove(vncAgentSocket); err != nil && !os.IsNotExist(err) { diff --git a/client/vnc/server/cursor_x11.go b/client/vnc/server/cursor_x11.go index e14ff856b..8ceb71c99 100644 --- a/client/vnc/server/cursor_x11.go +++ b/client/vnc/server/cursor_x11.go @@ -68,6 +68,15 @@ func (c *xfixesCursor) Cursor() (*image.RGBA, int, int, uint64, error) { } return nil, 0, 0, 0, fmt.Errorf("cursor has zero extent") } + // Anything past maxCursorDim is discarded by the encoder, so decoding it + // would allocate and convert a sprite that can only be thrown away. Keep + // showing the last good cursor instead. + if w > maxCursorDim || h > maxCursorDim { + if c.lastImg != nil { + return c.lastImg, c.lastHotX, c.lastHotY, c.lastSerial, nil + } + return nil, 0, 0, 0, fmt.Errorf("cursor %dx%d exceeds %d", w, h, maxCursorDim) + } if len(reply.CursorImage) < w*h { if c.lastImg != nil { return c.lastImg, c.lastHotX, c.lastHotY, c.lastSerial, nil diff --git a/client/vnc/server/handshake.go b/client/vnc/server/handshake.go index ae2460e2f..1f23688e1 100644 --- a/client/vnc/server/handshake.go +++ b/client/vnc/server/handshake.go @@ -63,17 +63,16 @@ func (s *Server) authenticateSession(header *connectionHeader) (string, error) { return "", fmt.Errorf("client static key missing") } - userIDHash, err := s.authorizer.LookupSessionKey(header.clientStatic) - if err != nil { - return "", fmt.Errorf("lookup session pubkey: %w", err) - } - osUser := "*" if header.mode == ModeSession { osUser = header.username } - if _, err := s.authorizer.AuthorizeOSUserBySessionKey(userIDHash, osUser); err != nil { - return "", fmt.Errorf("authorize OS user %q: %w", osUser, err) + + // One call, so a management update that revokes the key cannot land between + // resolving it and authorizing the identity it named. + userIDHash, _, err := s.authorizer.AuthorizeSessionKey(header.clientStatic, osUser) + if err != nil { + return "", fmt.Errorf("authorize session key for OS user %q: %w", osUser, err) } return userIDHash.String(), nil } diff --git a/client/vnc/server/noise_auth_test.go b/client/vnc/server/noise_auth_test.go index aa9baad40..ec2396aeb 100644 --- a/client/vnc/server/noise_auth_test.go +++ b/client/vnc/server/noise_auth_test.go @@ -448,5 +448,9 @@ func TestNoise_SessionMode_OSUserCheckRunsAfterHandshake(t *testing.T) { reason := readRFBFailure(t, conn) assert.Contains(t, reason, RejectCodeAuthForbidden) - assert.Contains(t, reason, "authorize OS user") + // The key itself resolved: what refused the session is the OS-user mapping, + // which is the half of the check that runs after the handshake. + assert.Contains(t, reason, "bob") + assert.Contains(t, reason, "no machine user mapping") + assert.NotContains(t, reason, sshauth.ErrSessionKeyNotKnown.Error()) } diff --git a/client/vnc/server/session.go b/client/vnc/server/session.go index b534b814c..3eeb3056d 100644 --- a/client/vnc/server/session.go +++ b/client/vnc/server/session.go @@ -198,6 +198,13 @@ func (s *session) serve() { encoderDone := make(chan struct{}) go s.encoderLoop(encoderDone) defer func() { + // Close the connection before waiting for the encoder. It may be parked + // in a write to a client that stopped reading, and the only thing that + // would unblock it otherwise is whatever deadline messageLoop last set + // on the shared conn: that leaves the session, and the connection slot + // it holds, alive for as long as that takes. The caller closes the + // connection too; a second Close is harmless. + s.conn.Close() close(s.encodeCh) <-encoderDone }() diff --git a/shared/sessionauth/auth.go b/shared/sessionauth/auth.go index 1779fb5f5..1bb7cf306 100644 --- a/shared/sessionauth/auth.go +++ b/shared/sessionauth/auth.go @@ -251,12 +251,50 @@ func (a *Authorizer) LookupSessionDisplayName(pubKey []byte) string { return name } +// AuthorizeSessionKey resolves a Noise-verified static public key and authorizes +// the identity it names for osUsername, under a single read lock. +// +// The two halves must not be separated: Update swaps sessionPubKeys and +// authorizedUsers together, so a caller that looks the key up and then +// authorizes it can have the key revoked in between and still be admitted on +// the hash it already holds. Revocation has to close that window. +func (a *Authorizer) AuthorizeSessionKey(pubKey []byte, osUsername string) (sshuserhash.UserIDHash, string, error) { + var zero sshuserhash.UserIDHash + if len(pubKey) != sessionPubKeyLen { + return zero, "", fmt.Errorf("session pubkey wrong length: %d", len(pubKey)) + } + var key [sessionPubKeyLen]byte + copy(key[:], pubKey) + + a.mu.RLock() + defer a.mu.RUnlock() + + hash, ok := a.sessionPubKeys[key] + if !ok { + return zero, "", ErrSessionKeyNotKnown + } + osUser, err := a.authorizeOSUserBySessionKeyLocked(hash, osUsername) + if err != nil { + return zero, "", err + } + return hash, osUser, nil +} + // AuthorizeOSUserBySessionKey resolves the OS-user mapping for a session // key. Mirrors Authorize but skips the JWT-hash step since the key has // already been verified and the user identity hash is in hand. +// +// Prefer AuthorizeSessionKey where the key is also being looked up: splitting +// the two lets a revocation land between them. func (a *Authorizer) AuthorizeOSUserBySessionKey(userIDHash sshuserhash.UserIDHash, osUsername string) (string, error) { a.mu.RLock() defer a.mu.RUnlock() + return a.authorizeOSUserBySessionKeyLocked(userIDHash, osUsername) +} + +// authorizeOSUserBySessionKeyLocked is AuthorizeOSUserBySessionKey with a.mu +// already held for reading. +func (a *Authorizer) authorizeOSUserBySessionKeyLocked(userIDHash sshuserhash.UserIDHash, osUsername string) (string, error) { userIndex, found := a.findUserIndex(userIDHash) if !found { return "", fmt.Errorf("session user (hash: %s) not in authorized list for OS user %q: %w", userIDHash, osUsername, ErrUserNotAuthorized) diff --git a/shared/sessionauth/auth_test.go b/shared/sessionauth/auth_test.go index b9a47d02d..a19985a31 100644 --- a/shared/sessionauth/auth_test.go +++ b/shared/sessionauth/auth_test.go @@ -711,3 +711,49 @@ func bytesRepeat(b byte, n int) []byte { } return out } + +// AuthorizeSessionKey resolves the key and authorizes the identity it names +// under one lock. Splitting those steps let an Update that revoked the key land +// in between, after which the caller still held a hash that authorized fine. +func TestAuthorizer_AuthorizeSessionKey_RevocationIsAtomic(t *testing.T) { + pub := bytesRepeat(0x55, sessionPubKeyLen) + userHash, err := sshauth.HashUserID("alice") + require.NoError(t, err) + + granted := &Config{ + AuthorizedUsers: []sshauth.UserIDHash{userHash}, + MachineUsers: map[string][]uint32{Wildcard: {0}}, + SessionPubKeys: []SessionPubKey{{PubKey: pub, UserIDHash: userHash}}, + } + // The user stays authorized; only the session key is withdrawn. That is the + // shape that used to slip through, because the second step never looked at + // the key again. + revoked := &Config{ + AuthorizedUsers: []sshauth.UserIDHash{userHash}, + MachineUsers: map[string][]uint32{Wildcard: {0}}, + } + + a := NewAuthorizer() + a.Update(granted) + gotHash, _, err := a.AuthorizeSessionKey(pub, "alice") + require.NoError(t, err) + assert.Equal(t, userHash, gotHash) + + a.Update(revoked) + _, _, err = a.AuthorizeSessionKey(pub, "alice") + require.ErrorIs(t, err, ErrSessionKeyNotKnown, "a revoked key must not authorize, even for a still-authorized user") +} + +// A key that resolves but whose user is not authorized is refused by the second +// half of the same call. +func TestAuthorizer_AuthorizeSessionKey_UnauthorizedUser(t *testing.T) { + pub := bytesRepeat(0x66, sessionPubKeyLen) + userHash, err := sshauth.HashUserID("alice") + require.NoError(t, err) + + a := NewAuthorizer() + a.Update(&Config{SessionPubKeys: []SessionPubKey{{PubKey: pub, UserIDHash: userHash}}}) + + _, _, err = a.AuthorizeSessionKey(pub, "alice") + require.ErrorIs(t, err, ErrUserNotAuthorized) +}