mirror of
https://github.com/screentinker/screentinker.git
synced 2026-08-14 06:16:20 -06:00
fix(devices): generate access-gating six-digit codes with a CSPRNG
The on-device settings PIN (devices.settings_pin, minted at pairing) and the pairing code assigned to imported devices both came from `Math.floor(100000 + Math.random() * 900000)`. Math.random is not a CSPRNG. V8 implements it as xorshift128+, whose internal state is recoverable from a handful of consecutive outputs, and every call in a process draws from that one shared stream. Both values are also observable by ordinary users — settings_pin is returned in device API responses today — so a user who collects a few outputs could predict the values minted around them, including for other tenants. lib/numeric-code.sixDigitCode() uses crypto.randomInt, which is CSPRNG-backed and rejection-samples so the distribution stays uniform. Range is 100000..999999 inclusive, identical to the old expression, so codes are still exactly six digits with no leading zero — the on-device keypad and pairing UI are unchanged. Deliberately NOT converted, because neither gates access: the image-generation seed in lib/image-gen.js, and the anti-burn-in pixel jitter inside generated widget HTML. Also unchanged: the settings_pin backfill in db/database.js, which uses SQLite's random() — that is ChaCha20 seeded from OS entropy, not a weak PRNG. This is the generator half of the finding only. The separate half — that settings_pin is returned to every workspace member, including read-only roles — is a response-shape change and waits on the consumer enumeration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
8a55798eaf
commit
dce0bc6f54
26
server/lib/numeric-code.js
Normal file
26
server/lib/numeric-code.js
Normal file
|
|
@ -0,0 +1,26 @@
|
|||
'use strict';
|
||||
|
||||
// Six-digit codes that gate access: the on-device settings PIN (devices.settings_pin) and
|
||||
// the provisioning pairing code (devices.pairing_code).
|
||||
//
|
||||
// Both were generated with `Math.floor(100000 + Math.random() * 900000)`. Math.random is
|
||||
// not a CSPRNG — V8 implements it as xorshift128+, whose internal state is recoverable
|
||||
// from a handful of consecutive outputs, and every call in a process draws from that one
|
||||
// shared stream. These two values are also OBSERVABLE by ordinary users (the PIN is
|
||||
// returned in device API responses today), so an attacker who can collect a few can
|
||||
// predict the ones minted around them for other tenants.
|
||||
//
|
||||
// crypto.randomInt is CSPRNG-backed and rejection-samples, so the distribution stays
|
||||
// uniform across the range rather than skewing the way a modulo would.
|
||||
//
|
||||
// Range is 100000..999999 inclusive — identical to what the old expression produced, so
|
||||
// the code is always exactly six digits with no leading zero, which is what the on-device
|
||||
// keypad and the pairing UI expect.
|
||||
|
||||
const crypto = require('crypto');
|
||||
|
||||
function sixDigitCode() {
|
||||
return String(crypto.randomInt(100000, 1000000));
|
||||
}
|
||||
|
||||
module.exports = { sixDigitCode };
|
||||
|
|
@ -5,6 +5,7 @@ const os = require('os');
|
|||
const path = require('path');
|
||||
const fs = require('fs');
|
||||
const config = require('../config');
|
||||
const { sixDigitCode } = require('../lib/numeric-code');
|
||||
const VERSION = require('../version');
|
||||
const { PLATFORM_ROLES, resolveSessionUser } = require('../middleware/auth');
|
||||
const loopLag = require('../services/loop-lag');
|
||||
|
|
@ -332,7 +333,7 @@ router.post('/import', importUpload.single('file'), async (req, res) => {
|
|||
for (const d of (data.devices || [])) {
|
||||
const newId = uuid.v4();
|
||||
idMap.devices[d.id] = newId;
|
||||
const pairingCode = String(Math.floor(100000 + Math.random() * 900000));
|
||||
const pairingCode = sixDigitCode(); // CSPRNG (lib/numeric-code): this code claims a device
|
||||
db.prepare(`INSERT INTO devices (id, user_id, workspace_id, name, pairing_code, status, screen_width, screen_height, created_at) VALUES (?, ?, ?, ?, ?, 'provisioning', ?, ?, ?)`).run(newId, userId, workspaceId, d.name, pairingCode, d.screen_width || null, d.screen_height || null, d.created_at || Math.floor(Date.now() / 1000));
|
||||
stats.devices++;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -555,6 +555,7 @@ app.get('/api/content/:id/thumbnail', (req, res) => {
|
|||
// req.isPlatformAdmin, req.actingAs. Route handlers in 2.1 don't read these
|
||||
// yet (they still filter by user_id); 2.2 will migrate them one route at a time.
|
||||
const { requireAuth } = require('./middleware/auth');
|
||||
const { sixDigitCode } = require('./lib/numeric-code');
|
||||
const { resolveTenancy } = require('./lib/tenancy');
|
||||
// Public API token front door (Phase 1). Attached ONLY to the public routers below.
|
||||
const { bearerAuth, tokenScopeGate, agencyGate } = require('./middleware/apiToken');
|
||||
|
|
@ -917,8 +918,11 @@ app.post('/api/provision/pair', requireAuth, resolveTenancy, checkDeviceLimit, (
|
|||
|
||||
const deviceName = name || 'Display ' + (db.prepare('SELECT COUNT(*) as count FROM devices WHERE user_id = ?').get(req.user.id).count + 1);
|
||||
// Generate a random 6-digit PIN for the hidden settings menu — each device gets a
|
||||
// unique PIN provisioned by the server (never a hardcoded default).
|
||||
const settingsPin = String(Math.floor(100000 + Math.random() * 900000));
|
||||
// unique PIN provisioned by the server (never a hardcoded default). CSPRNG-backed
|
||||
// (lib/numeric-code): this PIN gates the on-device settings menu and is observable in
|
||||
// device API responses, so Math.random's recoverable state would let one tenant predict
|
||||
// another's.
|
||||
const settingsPin = sixDigitCode();
|
||||
db.prepare("UPDATE devices SET pairing_code = NULL, name = ?, user_id = ?, workspace_id = ?, status = 'online', settings_pin = ?, updated_at = strftime('%s','now') WHERE id = ?")
|
||||
.run(deviceName, req.user.id, req.workspaceId, settingsPin, device.id);
|
||||
|
||||
|
|
|
|||
66
server/test/numeric-code.test.js
Normal file
66
server/test/numeric-code.test.js
Normal file
|
|
@ -0,0 +1,66 @@
|
|||
'use strict';
|
||||
|
||||
// The settings PIN and the provisioning pairing code both gate access — the PIN to the
|
||||
// on-device settings menu, the pairing code to claiming a device into a workspace. Both
|
||||
// must come from a CSPRNG.
|
||||
//
|
||||
// Math.random is not one: V8 implements it as xorshift128+, whose state is recoverable
|
||||
// from a few consecutive outputs, and every call in a process shares that stream. Since
|
||||
// the PIN is observable in device API responses, a user who collects a few could predict
|
||||
// values minted around them.
|
||||
//
|
||||
// The source assertion at the bottom is the one that fails if a future change reintroduces
|
||||
// Math.random for either value — the statistical tests below cannot distinguish a CSPRNG
|
||||
// from a good PRNG, so they alone would not catch a regression.
|
||||
|
||||
const { test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
const { sixDigitCode } = require('../lib/numeric-code');
|
||||
|
||||
test('a code is always exactly six digits, 100000-999999', () => {
|
||||
for (let i = 0; i < 2000; i++) {
|
||||
const c = sixDigitCode();
|
||||
assert.match(c, /^[1-9]\d{5}$/, `got ${c}`);
|
||||
const n = Number(c);
|
||||
assert.ok(n >= 100000 && n <= 999999, `out of range: ${n}`);
|
||||
}
|
||||
});
|
||||
|
||||
test('codes are not obviously degenerate (spread across the range, few repeats)', () => {
|
||||
const seen = new Set();
|
||||
const buckets = new Array(9).fill(0); // by leading digit 1..9
|
||||
for (let i = 0; i < 5000; i++) {
|
||||
const c = sixDigitCode();
|
||||
seen.add(c);
|
||||
buckets[Number(c[0]) - 1]++;
|
||||
}
|
||||
assert.ok(seen.size > 4900, `too many collisions in 5000 draws: ${seen.size} unique`);
|
||||
// Every leading digit should appear; a badly-scaled range would starve 1 or 9.
|
||||
for (let d = 0; d < 9; d++) assert.ok(buckets[d] > 0, `leading digit ${d + 1} never appeared`);
|
||||
});
|
||||
|
||||
test('the generator is CSPRNG-backed, not Math.random', () => {
|
||||
// Inspect the FUNCTION body, not the file: the file's comment legitimately mentions
|
||||
// Math.random while explaining why it is not used.
|
||||
const body = sixDigitCode.toString();
|
||||
assert.ok(/crypto\.randomInt|randomInt/.test(body), `must use crypto.randomInt, got: ${body}`);
|
||||
assert.ok(!/Math\.random/.test(body), 'must not use Math.random');
|
||||
});
|
||||
|
||||
test('no access-gating code is generated with Math.random anywhere on the server', () => {
|
||||
// Scans the two call sites that mint access-gating values. Deliberately narrow: other
|
||||
// Math.random uses in the tree are non-security (an image-generation seed in
|
||||
// lib/image-gen.js, and anti-burn-in pixel jitter inside generated widget HTML), and
|
||||
// sweeping those in would make this test a nuisance rather than a guard.
|
||||
const root = path.join(__dirname, '..');
|
||||
for (const rel of ['server.js', 'routes/status.js']) {
|
||||
const src = fs.readFileSync(path.join(root, rel), 'utf8');
|
||||
for (const line of src.split('\n')) {
|
||||
if (!/Math\.random/.test(line)) continue;
|
||||
assert.ok(!/(pin|pairing|code|secret|token)/i.test(line),
|
||||
`${rel} mints an access-gating value with Math.random: ${line.trim()}`);
|
||||
}
|
||||
}
|
||||
});
|
||||
Loading…
Reference in a new issue