Make session key authorization atomic, unblock the encoder on teardown, drop agent privileges unconditionally

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
+		// LOCAL_PEERCRED check see the right uid. No-op on Windows, where
-		// (both processes run as SYSTEM) and when --target-uid is 0.
+		// both processes run as SYSTEM.
-		if vncAgentTargetUID != 0 {
+		//
-			if err := dropAgentPrivileges(vncAgentTargetUID); err != nil {
+		// Called unconditionally: a missing or zero --target-uid is exactly
-				return fmt.Errorf("drop privileges to uid %d: %w", vncAgentTargetUID, err)
+		// 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) {

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

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
 }

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())
 }

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
 	}()

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)

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)
+}