Lobby vulnerabilities

[vdfdev]

I was looking at the code while developing a PR, and I think the Lobby endpoints have several vulnerabilities:

Leaking credentials

The credentials are initialized by playerID when the room is created, and does not change if hte user leaves or joins the room. So if a user leaves the room, they could possibly keep the credential, and when another user joins that room in their place, the previous user could play on behalf of the new user.

• Status: fixed in feat(server): Lobby server improvements #532

Reflective XSS for instance here:

router.get('/games/:name/:id', async ctx => {
 const gameID = ctx.params.id;
 const room = await db.get(`${gameName}:${GameMetadataKey(gameID)}`);
  if (!room) {
    ctx.throw(404, 'Room ' + gameID + ' not found');

if gameID is a javascript, then this javascript will be able to execute in the user browsers.

• Status: not addressed

XSRF

It seems like it has some protection with API_SECRET, but this is very weak as it is a global for the whole server. An attacker can just check what API_SECRET is being returned by the frontend once and a server restart would be needed to change the credential. Also, this option is buried deep inside the code and not documented anywhere.

• Status: not addressed

Race conditions

The "db" API has no interface for transactions or atomic checks, which leads to possible race conditions. Theoretically we could have two HTTP requests changing the state at the same time, and both executes their "checks" at the same time without seeing the change of each other states. This will definitely be a problem with several concurrent users, and specially if we need to scale to multiple servers accessing the database in parallel.

• Status: not addressed

[vdfdev]

I will spend some time improving the lobby in general. I want to refactor the code too, and add more features.

[nicolodavis]

Thanks for looking into this @flamecoals!

[nicolodavis]

Should we look into JSON Web Tokens for auth stuff in general?

[delucis]

JWT could be interesting. For now, might the following make sense to prevent leaking credentials?

• 1. Set seat credentials when a player joins a game, instead of initialising them all when the game is created.

• 2. By default generate credentials using shortid as currently, but allow the credentials function to be customised, calling it with relevant request information. You can already customise the ID generator using lobbyConfig.uuid, but this is also used to generate game IDs, so it might be helpful to separate ID generation from credential generation.

• 3. It might also be nice if you could configure an isAuthenticated method, which would default to the current behaviour of credentials !== gameMetadata.players[playerID].credentials but would allow other authentication approaches, such as storing a user ID as the credentials and checking a token on the request against some other authentication service.

• 4. If using some alternative authentication scheme like this, options could be added to require authentication for the /create and /join routes too.

[nicolodavis]

@delucis Sounds good!

[delucis]

Cool. It'll probably take me a little while to get to all of this, but I've found another bug with the database caching that's an easy fix, so I'll take care of that in the next couple of days.

[nicolodavis]

Thanks for looking into this, Chris!

…

On Tue, Dec 17, 2019, 3:41 PM Chris Swithinbank ***@***.***> wrote: Cool. It'll probably take me a little while to get to all of this, but I've found another bug with the database caching that's an easy fix, so I'll take care of that in the next couple of days. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#429>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZULUVFKQ6UZWQ7ZO3CRLQZB7BLANCNFSM4H4PEXDQ> .

[vdfdev]

FYI, I just ran into a race condition mentioned previously on joining the rooms while developing this PR:
freeboardgames/FreeBoardGames.org#688

Initially the code was running in memory and it was OK executing in parallel two users joining. However, when I started using Postgres, the two joins would execute in parallel the SQL to check the previous credential, and then both would write each user's credential only, making so one user would have no credential on the database. I had to change our server to do these requests serially to avoid this issue. But this could happen in the wild too, especially with moves if they happen very fast.

We should have a concept of transactions on the adapter to avoid this issue.

[delucis]

@flamecoals Hypothetically, anywhere that we read from storage, run code, then write to storage technically needs some kind of transaction API. This would be quite a bit of work (rewriting all the third-party storage implementations), but what might it look like from a high level?

Something like this?

await db.runTransaction(
  // map of desired inputs to fetch from db
  { metadata: true },
  // callback that is retried if inputs change and only writes when inputs haven’t changed
  (writer, { metadata }) => {
    metadata.players[playerID].credentials = 'foo';
    writer.setMetadata(metadata);
  }
);

That might offer a much more flexible database API allowing transactions to be used more broadly, but I don’t know what support would look like or how easy it is to implement in each backend.