Security: Changing /nick into an admin nick gives you op

[iriediese]

Expected Behavior

Admin rights (op) should be public-key based. Even if /nick is used to change your nickname into one belonging to an admin, you should not have any different priviledges.

Actual Behavior

When a non-admin changes their /nick to that of an admin, they get admin priviledges. This only happens if the admin is online at the time. Since two people cannot have the same name, the admin needs to also change their name beforehand.

Steps to reproduce behavior

scenario 1:

foo1 is admin. foo2 is not. Both need to be connected to the chat.
foo1 changes their name to foo3. they are no longer an admin
foo2 changes their name to foo1. they are now an admin.

scenario 2:
foo1 is admin. foo1 is connected to the chat.
foo1 changes their nick to foo2. they are no longer an admin.

The first 2 scenarios only happen if both are connected. The following, Scenario 3, seems to work as intended.
scenario 3:
foo1 is admin, foo2 is not. Neither are connected.
foo2 connects. foo2 changes nick to foo1.
foo1 connects. Their nick will be Guest1.
Guest1 is an admin. foo1 is not.

Additional Comments

It probably has to do with the manipulation of the IDs happenning upon changing /nicks.

[shazow]

Thanks for bringing this up @iriediese!

It's because the IsOp check in room is separate from in the host.

The host one is fine:

ssh-chat/auth.go

Lines 115 to 122 in 953c3d4

	// IsOp checks if a public key is an op.
	func (a *Auth) IsOp(key ssh.PublicKey) bool {
	if key == nil {
	return false
	}
	authkey := newAuthKey(key)
	return a.ops.In(authkey)
	}

But the room one relies on the ID (which is derived from the nick):

ssh-chat/chat/room.go

Lines 213 to 215 in 953c3d4

	func (r *Room) IsOp(u *message.User) bool {
	return r.Ops.In(u.ID())
	}

This is wrong.

The solution should be either to remove IsOp checking from the Room altogether (maybe have the host pass a callable that returns whether some user is an op), or to add an attribute to the user object so that it's retained across ID changes.

While we're at it, should also evaluate whether we depend on any other ID things in the Room that could be affected by renames.