Use secure uuid.v4 as session cookie secret

[Otto-AA]

Summary

This changes the secret used to create session cookies from the insecure uuid.v1 to the secure uuid.v4. The secret does not seem important for the security of the sessions, however as best practice we should change it to uuid.v4.

Details

To create session cookies NSS uses express-session. The cookie creation depends on a secret option, which should ideally be random characters according to the documentation. It is used to sign the session cookies (nssidp.sid).

However, as detailed in this post, the uuid v1 does not include randomness. Instead it depends on the current timestamp, the clock sequence and the MAC. This is not sufficiently random for a cookie secret.

An attacker can obtain the MAC and clock sequence from another uuid, in particular they can POST a new file and it will be created with a new uuid. With this, the knowledge of the approximate server starting time, and any valid session cookie (e.g. their own), they can bruteforce the secret uuid used by the server. I've tested this part, and locally I bruteforced the secret pretty quickly (e.g. if the server was started within one specific hour, I could bruteforce it within less than a day).

Impact

Likely no direct impact. From what I understand, it is a security in depth mechanism that could help against bad custom session id generation (which NSS currently does not use) and the possibility to detect bruteforcing attempts early on (which does not matter as NSS only seems to use the MemoryStore). The express-session library internally creates a 24-byte random session id. To impersonate other persons, one would need to know this id. Knowing the secret allows to bypass the signature verification, but gives no clue about other peoples session ids. A lot of discussion on this is here.

Draft PR

I've marked it as a draft pull request, as I didn't test it locally. I wouldn't know how this change could break something, but if you want to be sure please try it locally first.

[bourgeoa]

Thanks

• Use secure uuid.v4 as session cookie secret seems OK,
• for slug I wonder if there is any need (no security issue)
  It may be worse see the cons of uuid.v4

Cons of UUID v4:
There is a chance that a UUID could be duplicated. However, the probability is low.

[Otto-AA]

@bourgeoa I've updated the description of the initial post. I've got curious and made a deep dive into the express-session library and it turns out that the secret is not as important for the security as I would have expected (basically only an additional layer of security in case the other layers fail). However, we should still fix this as currently we loose this layer of defense.

I've also removed the change of the uuid.v4 change for the slug, so you can commit this security change without thinking more about what we would want for the slugs. Feel free to add the slug change in another commit if you agree that it would be beneficial.

[bourgeoa]

@Otto-AA build failed uuid.v4 not available
Either replace in import or change to node crypto.randomUUID() (available as from node 14.17)

[csarven]

See my recommendations to CSS about Slug: CommunitySolidServer/CommunitySolidServer#574 (comment) . Essentially when a server supports Slug, it can take the client's slugtext as one of the input towards generating the final value / effective request URI. Using uuid v4 instead of v1 should be the other parameter. Of course, the server doesn't need to put client's slugtext to use at all and no one would know about it. In that case it is effectively the same as not supporting Slug, or in other words, the server accepts requests including Slug.

[Otto-AA]

@Otto-AA build failed uuid.v4 not available
Either replace in import or change to node crypto.randomUUID() (available as from node 14.17)

Where do you get this error? I've started NSS locally and it could use uuid.v4() without a problem (with node v18.14.0).

If you mean the error from the CI (error: pathspec 'use-uuid-v4' did not match any file(s) known to git) I think the problem is that the test/surface/docker/server/Dockerfile clones node-solid-server and tries to checkout use-uuid-v4 there. However, the branch does not exist currently on node-solid-server, only on my fork. So it tries to checkout the branch on the main repo, not my fork.

[bourgeoa]

If you mean the error from the CI (error: pathspec 'use-uuid-v4' did not match any file(s) known to git) I think the problem is that the test/surface/docker/server/Dockerfile clones node-solid-server and tries to checkout use-uuid-v4 there. However, the branch does not exist currently on node-solid-server, only on my fork. So it tries to checkout the branch on the main repo, not my fork.

OK sorry I read the CI to quickly. It was late.

[bourgeoa]

LGTM

[Otto-AA]

OK sorry I read the CI to quickly. It was late.

Ok, no worries. Thanks for the fast responses in the first place!