From 14cd5eacb32ea5526e5226aa998b8a9478768fea Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Thu, 3 Sep 2026 09:14:48 +0200 Subject: [PATCH] fix(security): bound password input before zxcvbn, and drop the legacy media mounts Two findings from the GHSA-pwx6-5pqc-c5xq scan bundle, both verified against the code and reproduced before fixing. **Unauthenticated denial of service via password strength (csf_495d53fa).** POST /api/auth/password-strength takes `body('password').notEmpty()` with no upper bound, sits behind express.json({ limit: '50mb' }), and hands the string to zxcvbn, whose matching is superlinear and runs synchronously on the event loop. Measured on this codebase, in ms of blocked loop: 128 -> 41, 512 -> 1367, 1000 -> 5097, 5000 -> did not return in two minutes. One unauthenticated request of about a kilobyte stops the whole process for five seconds; a few kilobytes stops it indefinitely. The /api/auth rate limit does not help when a single request is already enough. The cap lives in validatePassword() so it covers every caller, present and future; the route validator is defence in depth. 128 keeps the worst case at the cost of an ordinary request while staying far above any real password -- bcrypt consumes only the first 72 bytes, so length past that adds no entropy to the stored hash anyway. This is the only unauthenticated reach into zxcvbn: setup is token-gated and self-closing, and acceptInvite/adminAuth use the regex-only validator in passwordGenerator. **The /photos and /thumbnails static mounts (csf_9aa6afe6, csf_559cd5cc, csf_b14d462e, csf_547d26fa, and the gallery half of csf_34e420af).** They served the raw originals and thumbnail trees behind photoAuth, which authorises on a slug match. A static file server cannot apply per-photo rules, so everything the gallery API decides was absent: allow_downloads, per-category allow_downloads, watermarking, the resolution cap, reveal-mode windows, visibility='hidden', download logging, and the customer-assignment re-check that makes revocation immediate. photoAuth also bcrypt-compares an x-gallery-password header per request with no limiter -- both rate-limit gates return early for non-/api paths -- so the mount was an unmetered password oracle. The filenames needed to drive all of this are handed to every guest in the photos listing. Nothing builds these URLs: no reference in frontend/src, none in the email templates, and the only backend mentions are the /api/admin/photos/... API routes and a maintenance-mode prefix list. The equivalent authorised routes are /api/gallery/:slug/photo/:id and /thumbnail/:id. nginx still proxies the two locations; they now 404, which is the intent. **The /uploads mount (csf_1fc92f57).** It exposed the whole uploads/ root with no auth middleware at all, and that root also holds signed contract PDFs (uploads/contracts/signed) and client transfer files (uploads/transfers/), reachable by anyone who learned or guessed a filename. Narrowed to the two public asset trees it exists for; contracts and transfers keep their own authorised routes. Removing the mounts leaves src/middleware/photoAuth.js unreferenced by application code. Left in place deliberately -- deleting it and its tests is a separate cleanup, and a smaller diff backports more safely. Backend suite: 2742 passed. --- .../routes/passwordStrengthDos.test.js | 49 +++++++++++++++++++ backend/server.js | 35 ++++++++++--- backend/src/routes/auth.js | 11 ++++- backend/src/utils/passwordValidation.js | 28 +++++++++++ 4 files changed, 115 insertions(+), 8 deletions(-) create mode 100644 backend/__tests__/routes/passwordStrengthDos.test.js diff --git a/backend/__tests__/routes/passwordStrengthDos.test.js b/backend/__tests__/routes/passwordStrengthDos.test.js new file mode 100644 index 00000000..3759a0f0 --- /dev/null +++ b/backend/__tests__/routes/passwordStrengthDos.test.js @@ -0,0 +1,49 @@ +/** + * POST /api/auth/password-strength is unauthenticated and feeds its body into + * zxcvbn, whose matching is superlinear and runs synchronously on the event + * loop. Behind express.json({ limit: '50mb' }) that made a single request a + * whole-process denial of service: measured on this codebase, 1,000 characters + * blocked for ~5 seconds and 5,000 did not return in two minutes. + * + * The control is the length cap inside validatePassword(), so it holds for + * every caller. These tests pin the cap itself rather than the route, and use + * a wall-clock ceiling that only an unbounded zxcvbn call can breach. + */ +const { validatePassword, MAX_PASSWORD_LENGTH } = require('../../src/utils/passwordValidation'); + +describe('password validation length cap (zxcvbn DoS)', () => { + it('rejects an over-length password without doing superlinear work', () => { + const huge = 'aA1!'.repeat(MAX_PASSWORD_LENGTH); // 4x the cap + const started = Date.now(); + const result = validatePassword(huge); + const elapsed = Date.now() - started; + + expect(result.valid).toBe(false); + expect(result.errors.join(' ')).toMatch(/at most 128 characters/); + // Unbounded, this input would not return for minutes. + expect(elapsed).toBeLessThan(250); + }); + + it('is bounded at the cap itself, the worst input it will still analyse', () => { + const atCap = 'aA1!'.repeat(MAX_PASSWORD_LENGTH / 4); + expect(atCap).toHaveLength(MAX_PASSWORD_LENGTH); + + // 128 was chosen so the worst input the validator will still analyse costs + // about as much as an ordinary request (~41ms measured); 512 cost 1.4s. + const started = Date.now(); + validatePassword(atCap); + expect(Date.now() - started).toBeLessThan(1000); + }); + + it('still accepts an ordinary strong password', () => { + const result = validatePassword('Tr0ub4dour&3-horse-battery'); + expect(result.valid).toBe(true); + }); + + it('applies the cap through the context wrapper too', async () => { + const { validatePasswordInContext } = require('../../src/utils/passwordValidation'); + const huge = 'aA1!'.repeat(MAX_PASSWORD_LENGTH); + const result = await validatePasswordInContext(huge, 'admin', {}); + expect(result.valid).toBe(false); + }); +}); diff --git a/backend/server.js b/backend/server.js index e78da6f8..84e45283 100644 --- a/backend/server.js +++ b/backend/server.js @@ -596,14 +596,35 @@ const secureStatic = require('./src/middleware/secureStatic'); const storagePath = process.env.STORAGE_PATH || path.join(__dirname, '../storage'); process.env.EXTERNAL_MEDIA_ROOT = process.env.EXTERNAL_MEDIA_ROOT || '/external-media'; -// Static file serving for photos (protected) -app.use('/photos', require('./src/middleware/photoAuth'), setCorsHeaders, secureStatic(path.join(storagePath, 'events/active'))); +// The /photos and /thumbnails static mounts are gone. +// +// They served the raw originals tree and the thumbnail tree behind photoAuth +// alone, which authorises on a slug match. A static file server cannot apply +// the rules the gallery API applies per photo, so everything the API decides +// was simply absent here: allow_downloads, per-category allow_downloads, +// watermarking, the resolution cap, reveal-mode windows, visibility='hidden', +// download logging, and the customer-assignment re-check that lets an admin +// revoke access immediately. The filenames needed to exercise it are handed to +// every guest in the photos listing. +// +// Nothing builds these URLs: no reference in frontend/src, none in the email +// templates, and the only backend mentions are the /api/admin/photos/... API +// routes and a maintenance-mode prefix list. nginx still proxies /photos and +// /thumbnails; those locations now 404, which is the intended outcome. +// +// Serving these safely would mean reimplementing per-photo authorisation and +// image processing inside a static handler -- i.e. the gallery API, which +// already exists at /api/gallery/:slug/photo/:id and /thumbnail/:id. -// Static file serving for thumbnails (protected) -app.use('/thumbnails', require('./src/middleware/photoAuth'), setCorsHeaders, secureStatic(path.join(storagePath, 'thumbnails'))); - -// Static file serving for uploads (public - logos, favicons) -app.use('/uploads', setCorsHeaders, secureStatic(path.join(storagePath, 'uploads'))); +// Static file serving for uploads. +// +// Narrowed to the two public asset trees. The mount used to expose the whole +// uploads/ root with no auth middleware at all, and that root also holds +// signed contract PDFs (uploads/contracts/signed) and client transfer files +// (uploads/transfers/) -- both reachable by anyone who learned or guessed +// a filename. Those are served by their own authorised routes. +app.use('/uploads/logos', setCorsHeaders, secureStatic(path.join(storagePath, 'uploads/logos'))); +app.use('/uploads/favicons', setCorsHeaders, secureStatic(path.join(storagePath, 'uploads/favicons'))); // Static file serving for self-hosted webfonts (public — gallery visitors // load these via @font-face). Replaces the previous Google Fonts CDN diff --git a/backend/src/routes/auth.js b/backend/src/routes/auth.js index b4d6ae78..3d341edb 100644 --- a/backend/src/routes/auth.js +++ b/backend/src/routes/auth.js @@ -33,6 +33,7 @@ const { getClientIp } = require('../utils/requestIp'); const { sanitizePasswordInput } = require('../utils/passwordInput'); const { validatePasswordInContext, + MAX_PASSWORD_LENGTH, getBcryptRounds, logPasswordValidationFailure } = require('../utils/passwordValidation'); @@ -952,8 +953,16 @@ router.post('/admin/change-password', [ }); // Password strength check endpoint (for real-time validation) +// +// Unauthenticated, and it feeds the request body straight into zxcvbn, whose +// matching is superlinear and synchronous. Without the length bound a single +// request stops the event loop for the whole process -- ~5s at 1,000 +// characters and unbounded past that. validatePassword() enforces the same cap +// for every caller; this one keeps the oversized body from being accepted at +// the edge at all. router.post('/password-strength', [ - body('password').notEmpty(), + body('password').isString().isLength({ min: 1, max: MAX_PASSWORD_LENGTH }) + .withMessage(`Password must be 1-${MAX_PASSWORD_LENGTH} characters`), body('context').isIn(['admin', 'gallery']).optional() ], async (req, res) => { try { diff --git a/backend/src/utils/passwordValidation.js b/backend/src/utils/passwordValidation.js index 09dba192..666e8943 100644 --- a/backend/src/utils/passwordValidation.js +++ b/backend/src/utils/passwordValidation.js @@ -7,6 +7,21 @@ const zxcvbn = require('zxcvbn'); const logger = require('./logger'); // Configuration +// zxcvbn's matching is superlinear in the input length and runs synchronously +// on the event loop, so an unbounded password is a denial-of-service primitive +// rather than a slow request. The reachable caller is +// POST /api/auth/password-strength, which is unauthenticated and sits behind +// express.json({ limit: '50mb' }) -- one request stops the whole process. +// +// Measured on this codebase (ms of blocked event loop per call): +// 64 -> 12 128 -> 41 192 -> 105 256 -> 218 +// 384 -> 632 512 -> 1367 1000 -> 5097 5000 -> did not return in 2 min +// +// 128 keeps the worst case at the cost of an ordinary request while staying +// far above any real password: bcrypt consumes only the first 72 bytes, so +// anything longer already adds no entropy to the stored hash. +const MAX_PASSWORD_LENGTH = 128; + const PASSWORD_CONFIG = { minLength: 8, // Reduced from 12 to 8 for better usability requireUppercase: true, @@ -34,6 +49,18 @@ const COMMON_PASSWORDS = [ function validatePassword(password, options = {}) { const config = { ...PASSWORD_CONFIG, ...options }; const errors = []; + + // Bail before any superlinear work touches the string. This is the guard for + // every caller, including ones added later -- the per-route length validator + // is defence in depth, not the control. + if (typeof password === 'string' && password.length > MAX_PASSWORD_LENGTH) { + return { + valid: false, + errors: [`Password must be at most ${MAX_PASSWORD_LENGTH} characters`], + score: 0, + feedback: {}, + }; + } // Check if password exists if (!password || typeof password !== 'string') { @@ -366,6 +393,7 @@ function logPasswordValidationFailure(context, errors, metadata = {}) { } module.exports = { + MAX_PASSWORD_LENGTH, validatePassword, validatePasswordInContext, generateSecurePassword,