Unauthenticated client is allowed to connect at the moment when an authenticated client connects

[nzinov]

Hi!

I've got a bug in 0.43.1. When I start an unautheticated client it can't connect, but if I start valid client in a different browser, the impostor client connects at that moment

[nzinov]

I guess the problem is that unauthenticated client remains connected in transport, and onSync sends current state both to real client and the impostor

[delucis]

Hi @nzinov — thanks for the report. Could you provide some more information and ideally a reproduction? In particular, it would be helpful to know which browsers you are using and to see the server and client set-up code.

[nzinov]

Hi @delucis,

My code is here: https://github.com/nzinov/phystech-seabattle.

Steps to reproduce are:

1. Run npm install && npm run build && npm run serve
2. Open /?match=main&player=0 in browser, it should load and store authentication token in cookie
3. Open /?match=main&player=0 in incognito/another browser, it won't load due to mismatched credentials
4. Reload window opened on step 2, window opened on step 3 gets game state at this point.

The bug is almost certaintly here

boardgame.io/src/server/transport/socketio.ts

Line 34 in dc96b26

const clients = roomInfo.get(matchID).values();

On send() the message is send to all connected clients with given playerID, even those that failed authentication in onSync call

[delucis]

Thanks for the extra details. Right, I think you’ve found the bug correctly:

1. When a client syncs, it is added to clientInfo even if it isn’t authenticated.

2. The send API sends to all clients claiming to be a given playerID regardless of authentication status, so actions by the authenticated player leak to the unauthenticated client.

If I understand correctly, this should mean the unauthenticated client can’t make any actions, but this would still be a vector to leak secret state.

Perhaps the simplest solution would be to check the return here:

boardgame.io/src/server/transport/socketio.ts

Line 159 in dc96b26

await master.onSync(...args);

And if it is { error: 'unauthorized' }, immediately remove the client from clientInfo. What do you think? That would allow us to keep any actual authentication logic out of the transport layer.

[nzinov]

I am not quite familiar with concurrency model used in transport. Is it possible that send call would occur in another thread before we delete unauthenticated client? I think a more foolproof solution would be to add an authenticated flag to clientInfo, set it in onSync and check in send

[delucis]

That sounds like a good idea. Would you like to put together a PR?

[delucis]

Just had a few more thoughts:

Is it possible that send call would occur in another thread before we delete unauthenticated client?

Theoretically I think so, yes. If the master is waiting to (fail to) authenticate, when another client triggers send (or the sendAll call in onUpdate), then the inauthentic client could presumably receive the leaked data before being kicked out of clientInfo. (The time window would likely be very small, but greater than 0.)

a more foolproof solution would be to add an authenticated flag to clientInfo, set it in onSync and check in send

This does add one additional consideration: when does the authenticated flag expire? The Lobby API allows a player to leave a match, and someone else to take their place, receiving new credentials for that playerID. With an authenticated flag, in theory a client could claim a seat, connect authentically, then leave while keeping the socket open. This would be a pretty reliable exploit, as opposed to the timing-dependent race condition above, so perhaps if there is no foolproof fix, the first solution would be preferable? What do you think?

[nzinov]

Alternatively, we can save latest version of credentials provided by client into clientInfo and check them every time in send()