Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve session ID security #1059

Merged
merged 3 commits into from
Jun 25, 2024
Merged

Improve session ID security #1059

merged 3 commits into from
Jun 25, 2024

Conversation

SleepyLeslie
Copy link
Collaborator

Follow-up of #994. This PR revises the session ID generation logic to improve security in the absence of a secure session secret. It also adds a section in the admin panel "security" section to nag system admins when GRIST_SESSION_SECRET is not set.

Following is an excerpt from internal conversation.

TL;DR: Grist's current implementation generates semi-secure session IDs and uses a publicly known default signing key to sign them when the environment variable GRIST_SESSION_SECRET is not set. This PR generates cryptographically secure session IDs to dismiss security concerns around an insecure signing key, and encourages system admins to configure their own signing key anyway.

The session secret is required by expressjs/session to sign its session IDs. It's designed as an extra protection against session hijacking by randomly guessing session IDs and hitting a valid one.
While it is easy to encourage users to set a distinct session secret, this is unnecessary if session IDs are generated in a cryptographically secure way.
As of now Grist uses version 4 UUIDs as session IDs (see app/server/lib/gristSessions.ts - it uses shortUUID.generate which invokes uuid.v4 under the hood). These contain 122 bits of entropy, technically insufficient to be considered cryptographically secure. In practice, this is never considered a real vulnerability. To compare, RSA2048 is still very commonly used in web servers, yet it only has 112 bits of security (>=128 bits = "secure", rule of thumb in cryptography). But for peace of mind I propose using crypto.getRandomValues to generate real 128-bit random values. This should render session ID signing unnecessary and hence dismiss security concerns around an insecure signing key.

@paulfitz
Copy link
Member

Follow-up of #994

How does this relate to #994?

Not reviewing yet, but this looks like a good incremental step, adding an admin panel check and improving the session IDs. Thanks for digging into this @SleepyLeslie.

@SleepyLeslie
Copy link
Collaborator Author

SleepyLeslie commented Jun 20, 2024

How does this relate to #994?

Related to comments here #994 (comment)

@paulfitz paulfitz requested a review from jordigh June 24, 2024 19:12
Copy link
Contributor

@jordigh jordigh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took me a while to reassure myself that the session secret is not the source of our session security. This PR seems fine to me now.

@paulfitz paulfitz merged commit 24ce54b into main Jun 25, 2024
13 checks passed
@SleepyLeslie SleepyLeslie deleted the sleepyleslie/session-secret branch June 25, 2024 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants