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/<id>),
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.
(cherry picked from commit 14cd5eacb3)
This commit is contained in:
@@ -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);
|
||||||
|
});
|
||||||
|
});
|
||||||
+28
-7
@@ -521,14 +521,35 @@ const secureStatic = require('./src/middleware/secureStatic');
|
|||||||
const storagePath = process.env.STORAGE_PATH || path.join(__dirname, '../storage');
|
const storagePath = process.env.STORAGE_PATH || path.join(__dirname, '../storage');
|
||||||
process.env.EXTERNAL_MEDIA_ROOT = process.env.EXTERNAL_MEDIA_ROOT || '/external-media';
|
process.env.EXTERNAL_MEDIA_ROOT = process.env.EXTERNAL_MEDIA_ROOT || '/external-media';
|
||||||
|
|
||||||
// Static file serving for photos (protected)
|
// The /photos and /thumbnails static mounts are gone.
|
||||||
app.use('/photos', require('./src/middleware/photoAuth'), setCorsHeaders, secureStatic(path.join(storagePath, 'events/active')));
|
//
|
||||||
|
// 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)
|
// Static file serving for uploads.
|
||||||
app.use('/thumbnails', require('./src/middleware/photoAuth'), setCorsHeaders, secureStatic(path.join(storagePath, 'thumbnails')));
|
//
|
||||||
|
// Narrowed to the two public asset trees. The mount used to expose the whole
|
||||||
// Static file serving for uploads (public - logos, favicons)
|
// uploads/ root with no auth middleware at all, and that root also holds
|
||||||
app.use('/uploads', setCorsHeaders, secureStatic(path.join(storagePath, 'uploads')));
|
// signed contract PDFs (uploads/contracts/signed) and client transfer files
|
||||||
|
// (uploads/transfers/<id>) -- 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
|
// Static file serving for self-hosted webfonts (public — gallery visitors
|
||||||
// load these via @font-face). Replaces the previous Google Fonts CDN
|
// load these via @font-face). Replaces the previous Google Fonts CDN
|
||||||
|
|||||||
@@ -30,6 +30,7 @@ const { getEventShareToken, resolveShareIdentifier } = require('../services/shar
|
|||||||
const { getClientIp } = require('../utils/requestIp');
|
const { getClientIp } = require('../utils/requestIp');
|
||||||
const {
|
const {
|
||||||
validatePasswordInContext,
|
validatePasswordInContext,
|
||||||
|
MAX_PASSWORD_LENGTH,
|
||||||
getBcryptRounds,
|
getBcryptRounds,
|
||||||
logPasswordValidationFailure
|
logPasswordValidationFailure
|
||||||
} = require('../utils/passwordValidation');
|
} = require('../utils/passwordValidation');
|
||||||
@@ -831,8 +832,16 @@ router.post('/admin/change-password', [
|
|||||||
});
|
});
|
||||||
|
|
||||||
// Password strength check endpoint (for real-time validation)
|
// 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', [
|
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()
|
body('context').isIn(['admin', 'gallery']).optional()
|
||||||
], async (req, res) => {
|
], async (req, res) => {
|
||||||
try {
|
try {
|
||||||
|
|||||||
@@ -7,6 +7,21 @@ const zxcvbn = require('zxcvbn');
|
|||||||
const logger = require('./logger');
|
const logger = require('./logger');
|
||||||
|
|
||||||
// Configuration
|
// 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 = {
|
const PASSWORD_CONFIG = {
|
||||||
minLength: 8, // Reduced from 12 to 8 for better usability
|
minLength: 8, // Reduced from 12 to 8 for better usability
|
||||||
requireUppercase: true,
|
requireUppercase: true,
|
||||||
@@ -35,6 +50,18 @@ function validatePassword(password, options = {}) {
|
|||||||
const config = { ...PASSWORD_CONFIG, ...options };
|
const config = { ...PASSWORD_CONFIG, ...options };
|
||||||
const errors = [];
|
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
|
// Check if password exists
|
||||||
if (!password || typeof password !== 'string') {
|
if (!password || typeof password !== 'string') {
|
||||||
return {
|
return {
|
||||||
@@ -366,6 +393,7 @@ function logPasswordValidationFailure(context, errors, metadata = {}) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
module.exports = {
|
module.exports = {
|
||||||
|
MAX_PASSWORD_LENGTH,
|
||||||
validatePassword,
|
validatePassword,
|
||||||
validatePasswordInContext,
|
validatePasswordInContext,
|
||||||
generateSecurePassword,
|
generateSecurePassword,
|
||||||
|
|||||||
Reference in New Issue
Block a user