Improve session ID security (#1059)

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.

app/client/ui/AdminPanel.ts

@@ -145,6 +145,13 @@ Please log in as an administrator.`)),
           description: t('Current authentication method'),
           value: this._buildAuthenticationDisplay(owner),
           expandedContent: this._buildAuthenticationNotice(owner),
+        }),
+        dom.create(AdminSectionItem, {
+          id: 'session',
+          name: t('Session Secret'),
+          description: t('Key to sign sessions with'),
+          value: this._buildSessionSecretDisplay(owner),
+          expandedContent: this._buildSessionSecretNotice(owner),
         })
       ]),
       dom.create(AdminSection, t('Version'), [
@@ -241,6 +248,27 @@ We recommend enabling one of these if Grist is accessible over the network or be
 to multiple people.');
   }
 
+  private _buildSessionSecretDisplay(owner: IDisposableOwner) {
+    return dom.domComputed(
+      use => {
+        const req = this._checks.requestCheckById(use, 'session-secret');
+        const result = req ? use(req.result) : undefined;
+
+        if (result?.status === 'warning') {
+          return cssValueLabel(cssDangerText('default'));
+        }
+
+        return cssValueLabel(cssHappyText('configured'));
+      }
+    );
+  }
+
+  private _buildSessionSecretNotice(owner: IDisposableOwner) {
+    return t('Grist signs user session cookies with a secret key. Please set this key via the environment variable \
+GRIST_SESSION_SECRET. Grist falls back to a hard-coded default when it is not set. We may remove this notice \
+in the future as session IDs generated since v1.1.16 are inherently cryptographically secure.');
+  }
+
   private _buildUpdates(owner: MultiHolder) {
     // We can be in those states:
     enum State {
@@ -472,7 +500,11 @@ to multiple people.');
     return dom.domComputed(
       use => [
         ...use(this._checks.probes).map(probe => {
-          const isRedundant = probe.id === 'sandboxing';
+          const isRedundant = [
+            'sandboxing',
+            'authentication',
+            'session-secret'
+          ].includes(probe.id);
           const show = isRedundant ? options.showRedundant : options.showNovel;
           if (!show) { return null; }
           const req = this._checks.requestCheck(probe);

app/common/BootProbe.ts

@@ -8,7 +8,8 @@ export type BootProbeIds =
     'sandboxing' |
     'system-user' |
     'authentication' |
-    'websockets'
+    'websockets' |
+    'session-secret'
 ;
 
 export interface BootProbeResult {

app/server/lib/BootProbes.ts

@@ -6,6 +6,7 @@ import { GristServer } from 'app/server/lib/GristServer';
 import * as express from 'express';
 import WS from 'ws';
 import fetch from 'node-fetch';
+import { DEFAULT_SESSION_SECRET } from 'app/server/lib/coreCreator';
 
 /**
  * Self-diagnostics useful when installing Grist.
@@ -61,6 +62,7 @@ export class BootProbes {
     this._probes.push(_sandboxingProbe);
     this._probes.push(_authenticationProbe);
     this._probes.push(_webSocketsProbe);
+    this._probes.push(_sessionSecretProbe);
     this._probeById = new Map(this._probes.map(p => [p.id, p]));
   }
 }
@@ -284,3 +286,17 @@ const _authenticationProbe: Probe = {
     };
   },
 };
+
+const _sessionSecretProbe: Probe = {
+  id: 'session-secret',
+  name: 'Session secret',
+  apply: async(server, req) => {
+    const usingDefaultSessionSecret = server.create.sessionSecret() === DEFAULT_SESSION_SECRET;
+    return {
+      status: usingDefaultSessionSecret ? 'warning' : 'success',
+      details: {
+        "GRIST_SESSION_SECRET": process.env.GRIST_SESSION_SECRET ? "set" : "not set",
+      }
+    };
+  },
+};

app/server/lib/coreCreator.ts

@@ -3,11 +3,14 @@ import { checkMinIOBucket, checkMinIOExternalStorage,
 import { makeSimpleCreator } from 'app/server/lib/ICreate';
 import { Telemetry } from 'app/server/lib/Telemetry';
 
+export const DEFAULT_SESSION_SECRET =
+  'Phoo2ag1jaiz6Moo2Iese2xoaphahbai3oNg7diemohlah0ohtae9iengafieS2Hae7quungoCi9iaPh';
+
 export const makeCoreCreator = () => makeSimpleCreator({
   deploymentType: 'core',
   // This can and should be overridden by GRIST_SESSION_SECRET
   // (or generated randomly per install, like grist-omnibus does).
-  sessionSecret: 'Phoo2ag1jaiz6Moo2Iese2xoaphahbai3oNg7diemohlah0ohtae9iengafieS2Hae7quungoCi9iaPh',
+  sessionSecret: DEFAULT_SESSION_SECRET,
   storage: [
     {
       name: 'minio',

app/server/lib/gristSessions.ts

@@ -6,10 +6,10 @@ import {GristServer} from 'app/server/lib/GristServer';
 import {fromCallback} from 'app/server/lib/serverUtils';
 import {Sessions} from 'app/server/lib/Sessions';
 import {promisifyAll} from 'bluebird';
+import * as crypto from 'crypto';
 import * as express from 'express';
 import assignIn = require('lodash/assignIn');
 import * as path from 'path';
-import * as shortUUID from "short-uuid";
 
 
 export const cookieName = process.env.GRIST_SESSION_COOKIE || 'grist_sid';
@@ -118,7 +118,10 @@ export function initGristSessions(instanceRoot: string, server: GristServer) {
   // cookie could be stolen (with some effort) by the custom domain's owner, we limit the damage
   // by only honoring custom-domain cookies for requests to that domain.
   const generateId = (req: RequestWithOrg) => {
-    const uid = shortUUID.generate();
+    // Generate 256 bits of cryptographically random data to use as the session ID.
+    // This ensures security against brute-force session hijacking even without signing the session ID.
+    const randomNumbers = crypto.getRandomValues(new Uint8Array(32));
+    const uid = Buffer.from(randomNumbers).toString("hex");
     return req.isCustomHost ? `c-${uid}@${req.org}@${req.get('host')}` : `g-${uid}`;
   };
   const sessionSecret = server.create.sessionSecret();

static/locales/en.client.json

@@ -1541,6 +1541,7 @@
         "Error": "Error",
         "Error checking for updates": "Error checking for updates",
         "Grist allows for very powerful formulas, using Python. We recommend setting the environment variable GRIST_SANDBOX_FLAVOR to gvisor if your hardware supports it (most will), to run formulas in each document within a sandbox isolated from other documents and isolated from the network.": "Grist allows for very powerful formulas, using Python. We recommend setting the environment variable GRIST_SANDBOX_FLAVOR to gvisor if your hardware supports it (most will), to run formulas in each document within a sandbox isolated from other documents and isolated from the network.",
+        "Grist signs user session cookies with a secret key. Please set this key via the environment variable GRIST_SESSION_SECRET. Grist falls back to a hard-coded default when it is not set. We may remove this notice in the future since session IDs have been updated to be inherently cryptographically secure.": "Grist signs user session cookies with a secret key. Please set this key via the environment variable GRIST_SESSION_SECRET. Grist falls back to a hard-coded default when it is not set. We may remove this notice in the future as session IDs generated since v1.1.16 are inherently cryptographically secure.",
         "Grist is up to date": "Grist is up to date",
         "Grist releases are at ": "Grist releases are at ",
         "Last checked {{time}}": "Last checked {{time}}",