Don't allow inheriting session unless username matches

[snej]

The method Server.inheritClientSession will use a persistent session with a matching ID without comparing the usernames first. This allows a client (accidentally or maliciously) to take over a session belonging to a different user, if it can guess the session ID.

Most seriously, this means the client would inherit the other user's topic subscriptions. It looks as though this bypasses authorization hooks, so the user could have access to topics it shouldn't; that could be a serious security vulnerability.

At a minimum, it allows a client to disconnect another client or wipe out its persistent session if it can guess the ID; an annoying DoS.

This seems to be covered by section 5.4.2 of the MQTT spec:

...the implementation should check that the Client is authorized to use the Client Identifier as this gives access to the MQTT Session State (described in [section 4.1]. This authorization check is to protect against the case where one Client, accidentally or maliciously, provides a Client Identifier that is already being used by some other Client.

The fix is pretty simple. I'll post a PR shortly.

[thedevop]

Hi @snej, currently the authentication is called before inheritClientSession:

server/server.go

Lines 443 to 457 in 5966c7f

	if !s.hooks.OnConnectAuthenticate(cl, pk) { // [MQTT-3.1.4-2]
	err := s.SendConnack(cl, packets.ErrBadUsernameOrPassword, false, nil)
	if err != nil {
	return fmt.Errorf("invalid connection send ack: %w", err)
	}
	return packets.ErrBadUsernameOrPassword
	}
	atomic.AddInt64(&s.Info.ClientsConnected, 1)
	defer atomic.AddInt64(&s.Info.ClientsConnected, -1)
	s.hooks.OnSessionEstablish(cl, pk)
	sessionPresent := s.inheritClientSession(pk, cl)

Although with static username, it is reasonable to assume username should remain the same to inherit a clientID, but I have seen use-case where username is dynamic, like using JWT.

Given MQTT spec (section 5.4.2) leaves what should be checked (username, IP, etc) to the authentication mechanisms, perhaps implementing custom OnConnectAuthenticate is the better route if the requirement is to associate username with clientID?

[snej]

currently the authentication is called before inheritClientSession

Yes, but this issue isn't related to client authentication, rather to channel authorization.

For example, if I log in with my valid username and password, but use a session ID that matches your last session, then I will end up with all of your channel subscriptions, whether or not I was supposed to be able to access those channels.

perhaps implementing custom OnConnectAuthenticate is the better route if the requirement is to associate username with clientID?

That seems reasonable. The Auth Ledger hook should be updated to do this, then, since I assume that's what most developers will use.

The check in the hook would probably look something like this?

func (h *myAuthHook) OnConnectAuthenticate(cl *mochi.Client, pk packets.Packet) bool {
	existing, _ := h.server.Clients.Get(pk.Connect.ClientIdentifier)
	if existing != nil && !slices.Equal(existing.Properties.Username, cl.Properties.Username) {
		return false
	}
	...
}

This does have the problem that the error sent to the client will be ErrBadUsernameOrPassword, which isn't accurate; probably ErrClientIdentifierNotValid would be better. Is there any way to customize the error returned to the client?

[thedevop]

Yes, but this issue isn't related to client authentication, rather to channel authorization.

For example, if I log in with my valid username and password, but use a session ID that matches your last session, then I will end up with all of your channel subscriptions, whether or not I was supposed to be able to access those channels.

Just to ensure we're on the same page, session ID means the client ID the client used to connect? If so, then the use-case you described for the complete authentication should includes clientID/username/password. I assume that username/password should always be associated with a clientID (not only on inheriting a session)

MQTTv5 reason code

Value	Hex	Reason Code name	Description
133	0x85	Client Identifier not valid	The Client Identifier is a valid string but is not allowed by the Server.
135	0x87	Not authorized	The Client is not authorized to connect.

MQTTv3 connect return code

Value	Return Code Response	Description
2	0x02 Connection Refused, identifier rejected	The Client identifier is correct UTF-8 but not allowed by the Server
5	0x05 Connection Refused, not authorized	The Client is not authorized to connect

perhaps implementing custom OnConnectAuthenticate is the better route if the requirement is to associate username with clientID?

That seems reasonable. The Auth Ledger hook should be updated to do this, then, since I assume that's what most developers will use.

There are 2 general use-cases of mochi-mqtt:

1. embedded MQTT server, generally do not use auth ledger and already use custom authentications.
2. packaged solution, as mochi indicated previously, the auth ledger was meant to be a simple hook. However, more and more people are using this with different needs, perhaps it's time to revisit and redesign this?

The check in the hook would probably look something like this?

func (h *myAuthHook) OnConnectAuthenticate(cl *mochi.Client, pk packets.Packet) bool {
	existing, _ := h.server.Clients.Get(pk.Connect.ClientIdentifier)
	if existing != nil && !slices.Equal(existing.Properties.Username, cl.Properties.Username) {
		return false
	}
	...
}

This does have the problem that the error sent to the client will be ErrBadUsernameOrPassword, which isn't accurate; probably ErrClientIdentifierNotValid would be better. Is there any way to customize the error returned to the client?

Yes, currently OnConnectAuthenticate only returns a bool. We can either change the hardcoded return code to something generic like packets.ErrNotAuthorized:

server/server.go

Lines 443 to 450 in 5966c7f

	if !s.hooks.OnConnectAuthenticate(cl, pk) { // [MQTT-3.1.4-2]
	err := s.SendConnack(cl, packets.ErrBadUsernameOrPassword, false, nil)
	if err != nil {
	return fmt.Errorf("invalid connection send ack: %w", err)
	}
	return packets.ErrBadUsernameOrPassword
	}

or change the signature of OnConnectAuthenticate. Thoughts @mochi-co?

[werbenhu]

@snej Consider a scenario where I want all devices to log in using a single username and password, but each device is identified by a different ClientID. I think the association between username and clientID should be managed and allocated by the users themselves through hooks for better suitability.

[snej]

I'm fine having the Server struct not enforce matching usernames; I'll close this PR. But it would be good if the OnConnectAuthenticate hook could specify a different error code -- otherwise a client accidentally using someone else's client ID would be told its login is incorrect, which could be very confusing. I'll create a new PR for that.