diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 17f338fe..34005983 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -128,3 +128,7 @@ **Vulnerability:** The backend CSV export for audit logs neutralized `=`, `+`, `-`, and `@` but failed to neutralize `|` (pipe) characters, allowing potential DDE (Dynamic Data Exchange) injection if exported logs were opened in spreadsheet software. **Learning:** Spreadsheet formula defenses must cover all command-style prefixes including `|` across all CSV export boundaries, both frontend and backend. **Prevention:** Update the sanitization regex in the backend export function to `/^[=+\-@|]/` so that all potentially executable spreadsheet payloads are prefixed with a single quote. +## 2024-07-12 - Prevent User Enumeration via Authentication Timing Attacks +**Vulnerability:** The login endpoint verified the provided password hash only if a user record was found. If no user was found, it returned early. This meant requests for valid emails took longer to process than invalid emails, enabling attackers to enumerate valid email addresses via timing discrepancies. +**Learning:** Returning early on failed database lookups before performing expensive cryptographic operations creates a measurable timing difference. Cryptographic paths must be balanced. +**Prevention:** Always evaluate passwords against a `DUMMY_HASH` if the user lookup fails, ensuring constant-time execution regardless of whether the user exists. Explicitly coerce passwords to strings (`typeof password === 'string' ? password : ''`) before passing them to the hash verifier to avoid TypeErrors from unexpected JSON payloads while still evaluating against the dummy hash. diff --git a/package.json b/package.json index 8cefdc74..f8686164 100644 --- a/package.json +++ b/package.json @@ -13,7 +13,7 @@ "coverage": "npm run test:coverage", "server": "node server/server.mjs", "test:api": "node tests/api/auth-secret.test.mjs && node tests/api/smoke.mjs && node tests/api/ratelimit.test.mjs && node tests/api/attachment-status.test.mjs && node tests/api/session-revocation.test.mjs && node tests/api/orchestrator-attribution.test.mjs", - "test:unit": "node tests/unit/opencode-config.test.mjs && node tests/unit/changelog-release-notes.test.mjs && node tests/unit/analytics.test.mjs && node tests/unit/cpm.test.mjs && node tests/unit/baseline-compare.test.mjs && node tests/unit/workload.test.mjs && node tests/unit/cost-evm.test.mjs && node tests/unit/msproject.test.mjs && node tests/unit/auth-password.test.mjs && node tests/unit/editor-unsaved.test.mjs && node tests/unit/static-coverage-evidence.test.mjs && node tests/unit/dep-types.test.mjs && node tests/unit/weekly-report.test.mjs && node tests/unit/clearfolio.test.mjs && node tests/unit/clearfolio-adapter-mock-hmac.test.mjs && node tests/unit/orchestrator.test.mjs && node tests/unit/orchestrator-coverage.test.mjs && node tests/unit/orchestrator-attribution.test.mjs && node tests/unit/sprint-stats.test.mjs && node tests/unit/burndown.test.mjs && node tests/unit/pm-analysis.test.mjs && node tests/unit/cloud-sync-security.test.mjs && node tests/unit/attachment-status.test.mjs && node tests/unit/clearfolio-status-signal.test.mjs && node tests/unit/coverage-script-contract.test.mjs && node tests/unit/toast-accessibility.test.mjs", + "test:unit": "node tests/unit/opencode-config.test.mjs && node tests/unit/changelog-release-notes.test.mjs && node tests/unit/analytics.test.mjs && node tests/unit/cpm.test.mjs && node tests/unit/baseline-compare.test.mjs && node tests/unit/workload.test.mjs && node tests/unit/cost-evm.test.mjs && node tests/unit/msproject.test.mjs && node tests/unit/auth-password.test.mjs && node tests/unit/editor-unsaved.test.mjs && node tests/unit/static-coverage-evidence.test.mjs && node tests/unit/dep-types.test.mjs && node tests/unit/weekly-report.test.mjs && node tests/unit/clearfolio.test.mjs && node tests/unit/clearfolio-adapter-mock-hmac.test.mjs && node tests/unit/orchestrator.test.mjs && node tests/unit/orchestrator-coverage.test.mjs && node tests/unit/orchestrator-attribution.test.mjs && node tests/unit/sprint-stats.test.mjs && node tests/unit/burndown.test.mjs && node tests/unit/pm-analysis.test.mjs && node tests/unit/cloud-sync-security.test.mjs && node tests/unit/attachment-status.test.mjs && node tests/unit/clearfolio-status-signal.test.mjs && node tests/unit/coverage-script-contract.test.mjs && node tests/unit/toast-accessibility.test.mjs && node tests/unit/auth-timing.test.mjs", "test:coverage": "c8 --all --include=app.js --include=cloud-sync.js --include=scripts/ci/static_coverage_evidence.mjs --include=server/attachment_status.mjs --include=server/app.mjs --include=server/auth.mjs --include=server/clearfolio.mjs --include=server/orchestrator.mjs --reporter=json --reporter=json-summary npm run test:coverage:cases", "test:coverage:cases": "node tests/unit/coverage-script-contract.test.mjs && node tests/unit/attachment-status.test.mjs && node tests/unit/clearfolio-status-signal.test.mjs && node tests/unit/clearfolio-adapter-mock-hmac.test.mjs && node tests/unit/orchestrator.test.mjs && node tests/unit/orchestrator-coverage.test.mjs && node tests/unit/orchestrator-attribution.test.mjs && node tests/unit/msproject.test.mjs && node tests/unit/auth-password.test.mjs && node tests/unit/editor-unsaved.test.mjs && node tests/unit/static-coverage-evidence.test.mjs && npm run test:api", "test:e2e": "playwright test", diff --git a/server/app.mjs b/server/app.mjs index c432a84f..afcc9953 100644 --- a/server/app.mjs +++ b/server/app.mjs @@ -12,6 +12,8 @@ import { normalizeAttachmentStatusBudgetMs, normalizeAttachmentStatusConcurrency import { chat as orchestratorChat } from './orchestrator.mjs'; import { computeEvm } from '../analytics.js'; // pure math, shared with the client +const DUMMY_HASH = hashPassword(''); + const getOrg = (id) => db.prepare('SELECT * FROM orgs WHERE id = ?').get(id); // Append-only audit trail. Never throws into the request path. @@ -192,9 +194,9 @@ app.post('/api/auth/signup', async (c) => { app.post('/api/auth/login', async (c) => { const { email, password } = await c.req.json().catch(() => ({})); const u = db.prepare('SELECT * FROM users WHERE email = ?').get(email || ''); - // Pass password through only when it is a string — verifyPassword rejects - // non-strings (objects/arrays) so they never match an empty-password hash. - if (!u || typeof password !== 'string' || !verifyPassword(password, u.password_hash)) { + // Always evaluate the password against a hash to mitigate timing attacks. + const isMatch = verifyPassword(typeof password === 'string' ? password : '', u ? u.password_hash : DUMMY_HASH); + if (!u || typeof password !== 'string' || !isMatch) { return c.json({ error: 'invalid credentials' }, 401); } return c.json({ token: signToken({ sub: u.id, email: u.email, tv: u.token_version }) }); @@ -1352,7 +1354,8 @@ app.post('/api/auth/change-password', requireAuth, async (c) => { const { oldPassword, newPassword } = await c.req.json().catch(() => ({})); if (typeof newPassword !== 'string' || newPassword.length < 8) return c.json({ error: 'new password (min 8) required' }, 400); const u = db.prepare('SELECT password_hash FROM users WHERE id = ?').get(uid); - if (!u || typeof oldPassword !== 'string' || !verifyPassword(oldPassword, u.password_hash)) { + const isMatch = verifyPassword(typeof oldPassword === 'string' ? oldPassword : '', u ? u.password_hash : DUMMY_HASH); + if (!u || typeof oldPassword !== 'string' || !isMatch) { return c.json({ error: 'current password incorrect' }, 403); } db.prepare('UPDATE users SET password_hash = ? WHERE id = ?').run(hashPassword(newPassword), uid); @@ -1365,7 +1368,8 @@ app.delete('/api/account', requireAuth, async (c) => { const uid = c.get('user').sub; const { password } = await c.req.json().catch(() => ({})); const u = db.prepare('SELECT password_hash FROM users WHERE id = ?').get(uid); - if (!u || typeof password !== 'string' || !verifyPassword(password, u.password_hash)) { + const isMatch = verifyPassword(typeof password === 'string' ? password : '', u ? u.password_hash : DUMMY_HASH); + if (!u || typeof password !== 'string' || !isMatch) { return c.json({ error: 'password required to delete account' }, 403); } db.exec('BEGIN'); diff --git a/tests/unit/auth-timing.test.mjs b/tests/unit/auth-timing.test.mjs new file mode 100644 index 00000000..ead5e476 --- /dev/null +++ b/tests/unit/auth-timing.test.mjs @@ -0,0 +1,51 @@ +import assert from 'node:assert'; +import { spawnSync } from 'node:child_process'; + +const SECRET = '0123456789abcdef0123456789abcdef'; + +const script = ` +import assert from 'node:assert'; +import { hashPassword, verifyPassword } from './server/auth.mjs'; + +const DUMMY_HASH = hashPassword(''); + +function simulateLogin(email, password, userFound) { + const passwordHash = userFound ? hashPassword('correct_password') : null; + const u = userFound ? { password_hash: passwordHash } : null; + + const isMatch = verifyPassword(typeof password === 'string' ? password : '', u ? u.password_hash : DUMMY_HASH); + + if (!u || typeof password !== 'string' || !isMatch) { + return false; + } + return true; +} + +try { + // Test invalid user, returns false but evaluates DUMMY_HASH + assert.strictEqual(simulateLogin('invalid@test.com', 'pass', false), false); + + // Test valid user, wrong password, returns false + assert.strictEqual(simulateLogin('valid@test.com', 'wrong', true), false); + + // Test valid user, correct password, returns true + assert.strictEqual(simulateLogin('valid@test.com', 'correct_password', true), true); + + // Test type mismatch on password, returns false + assert.strictEqual(simulateLogin('valid@test.com', { invalid: 'type' }, true), false); + + console.log('✓ auth timing integration tests passed'); +} catch (err) { + console.error('Test failed:', err); + process.exit(1); +} +`; + +const result = spawnSync(process.execPath, ['--input-type=module', '--eval', script], { + cwd: process.cwd(), + env: { ...process.env, SCOPEWEAVE_JWT_SECRET: SECRET }, + encoding: 'utf8', +}); + +assert.equal(result.status, 0, result.stderr || result.stdout); +process.stdout.write(result.stdout);