From 01a3a5038d5a415241e4957ee0fcd7ec6acd7e17 Mon Sep 17 00:00:00 2001 From: "P. Douglas Reeder" Date: Wed, 17 Jul 2024 16:46:57 -0400 Subject: [PATCH] Adds rate-limiting to defend against buggy and malicious clients --- lib/appFactory.js | 27 ++++++++++++---- lib/middleware/rateLimiterMiddleware.js | 42 +++++++++++++++++++++++++ lib/middleware/sanityCheckUsername.js | 4 ++- lib/routes/admin.js | 6 +++- lib/routes/login.js | 6 ++-- lib/routes/oauth.js | 3 ++ package-lock.json | 11 +++++++ package.json | 1 + spec/modular/m_not_found.spec.js | 11 +++++-- spec/not_found.spec.js | 4 +-- 10 files changed, 101 insertions(+), 14 deletions(-) create mode 100644 lib/middleware/rateLimiterMiddleware.js diff --git a/lib/appFactory.js b/lib/appFactory.js index d2f388d..0d00a0a 100644 --- a/lib/appFactory.js +++ b/lib/appFactory.js @@ -15,6 +15,7 @@ const accountRouterFactory = require('./routes/account'); const adminFactory = require('./routes/admin'); const session = require('express-session'); const MemoryStore = require('memorystore')(session); +const { rateLimiterPenalty, rateLimiterBlock, rateLimiterMiddleware } = require('./middleware/rateLimiterMiddleware'); module.exports = async function ({ hostIdentity, jwtSecret, accountMgr, storeRouter, basePath = '' }) { if (basePath && !basePath.startsWith('/')) { basePath = '/' + basePath; } @@ -34,7 +35,7 @@ module.exports = async function ({ hostIdentity, jwtSecret, accountMgr, storeRou app.set('accountMgr', accountMgr); - // web browsers ask for this way too often + // web browsers ask for this way too often, so doesn't log this app.get('/favicon.ico', (req, res, _next) => { res.set('Cache-Control', 'public, max-age=31536000'); res.status(404).end(); @@ -42,6 +43,10 @@ module.exports = async function ({ hostIdentity, jwtSecret, accountMgr, storeRou app.use(loggingMiddleware); + if (process.env.NODE_ENV === 'production') { + app.use(rateLimiterMiddleware); + } + app.use(robots(path.join(__dirname, 'robots.txt'))); const helmetStorage = helmet({ @@ -100,9 +105,10 @@ module.exports = async function ({ hostIdentity, jwtSecret, accountMgr, storeRou app.use(`${basePath}/assets`, express.static(path.join(__dirname, 'assets'), { fallthrough: true, index: false, maxAge: '25m' })); - app.use(`${basePath}/assets`, (req, res, next) => { + app.use(`${basePath}/assets`, async (req, res, _next) => { res.set('Cache-Control', 'public, max-age=1500'); res.status(404).end(); + await rateLimiterPenalty(req.ip); }); app.use(`${basePath}/signup`, requestInviteRouter(storeRouter)); @@ -148,27 +154,36 @@ module.exports = async function ({ hostIdentity, jwtSecret, accountMgr, storeRou app.use(`${basePath}/admin`, admin); // catches paths not handled and returns Not Found - app.use(basePath, function (req, res) { + app.use(basePath, async function (req, res) { if (!res.get('Cache-Control')) { res.set('Cache-Control', 'max-age=1500'); } + const subpath = req.path.slice(basePath.length).split('/')?.[1]; const name = req.path.slice(1); - errorPage(req, res, 404, { title: 'Not Found', message: `“${name}” doesn't exist` }); + if (['storage', 'oauth', 'account', 'admin'].includes(subpath)) { // reasonable + errorPage(req, res, 404, { title: 'Not Found', message: `“${name}” doesn't exist` }); + } else { // probably hostile + res.logNotes.add(`“${name}” shouldn't and doesn't exist`); + res.status(404).end(); + await rateLimiterBlock(req.ip, 61); + } }); // redirect for paths outside the app - app.use(function (req, res) { + app.use(async function (req, res) { res.status(308).set('Location', basePath).end(); + await rateLimiterPenalty(req.ip); }); // error handler - app.use(function (err, req, res, _next) { + app.use(async function (err, req, res, _next) { const message = err?.message || err?.errors?.find(e => e.message).message || err?.cause?.message || 'indescribable error'; errorPage(req, res, err.status || 500, { title: shorten(message, 30), message, error: req.app.get('env') === 'development' ? err : {} }); + await rateLimiterPenalty(req.ip); }); setTimeout(() => { diff --git a/lib/middleware/rateLimiterMiddleware.js b/lib/middleware/rateLimiterMiddleware.js new file mode 100644 index 0000000..c25a493 --- /dev/null +++ b/lib/middleware/rateLimiterMiddleware.js @@ -0,0 +1,42 @@ +const { RateLimiterMemory, BurstyRateLimiter } = require('rate-limiter-flexible'); + +const SUSTAINED_REQ_PER_SEC = 8; +const MAX_BURST = 50; // remotestorage.js appears to keep requesting until 10 failures + +const rateLimiterSustained = new RateLimiterMemory({ + points: SUSTAINED_REQ_PER_SEC, + duration: 1 +}); + +const rateLimiterBurst = new RateLimiterMemory({ + keyPrefix: 'burst', + points: MAX_BURST - SUSTAINED_REQ_PER_SEC, + duration: 10 +}); + +async function rateLimiterPenalty (key, points = 1) { + await rateLimiterSustained.penalty(key, points); + await rateLimiterBurst.penalty(key, points); +} + +async function rateLimiterBlock (key, secDuration) { + await rateLimiterSustained.block(key, secDuration); + await rateLimiterBurst.block(key, secDuration); +} + +const rateLimiter = new BurstyRateLimiter( + rateLimiterSustained, + rateLimiterBurst +); + +const rateLimiterMiddleware = async (req, res, next) => { + try { + await rateLimiter.consume(req.ip); + next(); + } catch (err) { + res.set({ 'Retry-After': Math.ceil(err.msBeforeNext / 1000) }); + res.status(429).end(); + } +}; + +module.exports = { rateLimiterPenalty, rateLimiterBlock, rateLimiterMiddleware }; diff --git a/lib/middleware/sanityCheckUsername.js b/lib/middleware/sanityCheckUsername.js index 94ad169..217df03 100644 --- a/lib/middleware/sanityCheckUsername.js +++ b/lib/middleware/sanityCheckUsername.js @@ -1,5 +1,6 @@ /** sanity check of username, to defend against ".." and whatnot */ -module.exports = function sanityCheckUsername (req, res, next) { +const { rateLimiterBlock } = require('./rateLimiterMiddleware'); +module.exports = async function sanityCheckUsername (req, res, next) { const username = req.params.username || req.data.username || ''; if (username.length > 0 && !/\/|^\.+$/.test(username) && /[\p{L}\p{Nd}]{1,63}/u.test(username)) { return next(); @@ -7,4 +8,5 @@ module.exports = function sanityCheckUsername (req, res, next) { res.logNotes.add('invalid user'); res.status(400).end(); + await rateLimiterBlock(req.ip, 61); }; diff --git a/lib/routes/admin.js b/lib/routes/admin.js index 1f8b44d..347b59d 100644 --- a/lib/routes/admin.js +++ b/lib/routes/admin.js @@ -12,6 +12,7 @@ const { initProtocols, assembleContactURL, calcContactURL, protocolOptions } = r const nameFromUseragent = require('../util/nameFromUseragent'); const removeUserDataFromSession = require('../util/removeUserDataFromSession'); const ParameterError = require('../util/ParameterError'); +const { rateLimiterBlock, rateLimiterPenalty } = require('../middleware/rateLimiterMiddleware'); /* eslint no-unused-vars: ["warn", { "varsIgnorePattern": "^_", "argsIgnorePattern": "^_" }] */ /* eslint-disable no-case-declarations */ @@ -140,6 +141,7 @@ module.exports = async function (hostIdentity, jwtSecret, accountMgr, storeRoute }); if (!verified) { + await rateLimiterPenalty(req.ip); throw new Error('Verification failed.'); } @@ -398,6 +400,7 @@ module.exports = async function (hostIdentity, jwtSecret, accountMgr, storeRoute if (!(req.session.privileges?.ADMIN || (req.session.user && req.body.contacturl === req.session.user?.contactURL))) { res.logNotes.add('session does not have ADMIN privilege'); res.status(401).type('text/plain').send('Ask an admin to send the invite'); + await rateLimiterBlock(req.ip, 61); return; } @@ -441,9 +444,10 @@ module.exports = async function (hostIdentity, jwtSecret, accountMgr, storeRoute res.logNotes.add('session does not have ADMIN privilege'); if (['GET', 'HEAD'].includes(req.method)) { res.redirect(307, './login'); - } else { + } else { // TODO consider deleting this unused code res.logNotes.add('session lacks ADMIN privilege'); res.status(401).end(); + // await rateLimiterBlock(req.ip, 61); } } } diff --git a/lib/routes/login.js b/lib/routes/login.js index b22355b..93370ac 100644 --- a/lib/routes/login.js +++ b/lib/routes/login.js @@ -5,6 +5,7 @@ const loginOptsWCreds = require('../util/loginOptsWCreds'); const removeUserDataFromSession = require('../util/removeUserDataFromSession'); const verifyCredential = require('../util/verifyCredential'); const updateSessionPrivileges = require('../util/updateSessionPrivileges'); +const { rateLimiterPenalty } = require('../middleware/rateLimiterMiddleware'); module.exports = async function (hostIdentity, jwtSecret, account, isAdminLogin) { const rpID = hostIdentity; @@ -78,10 +79,11 @@ module.exports = async function (hostIdentity, jwtSecret, account, isAdminLogin) errToMessages(err, res.logNotes); if (['Error', 'NoSuchUserError'].includes(err.name)) { - return res.status(401).json({ msg: 'Your passkey could not be validated' }); + res.status(401).json({ msg: 'Your passkey could not be validated' }); } else { - return res.status(500).json({ msg: 'If this problem persists, contact an administrator.' }); + res.status(500).json({ msg: 'If this problem persists, contact an administrator.' }); } + await rateLimiterPenalty(req.ip); } }); diff --git a/lib/routes/oauth.js b/lib/routes/oauth.js index 083f796..24ef605 100644 --- a/lib/routes/oauth.js +++ b/lib/routes/oauth.js @@ -14,6 +14,7 @@ const verifyCredential = require('../util/verifyCredential'); const removeUserDataFromSession = require('../util/removeUserDataFromSession'); const updateSessionPrivileges = require('../util/updateSessionPrivileges'); const errorPage = require('../util/errorPage'); +const { rateLimiterPenalty, rateLimiterBlock } = require('../middleware/rateLimiterMiddleware'); const accessStrings = { r: 'Read', rw: 'Read/write' }; @@ -83,6 +84,7 @@ module.exports = function (hostIdentity, jwtSecret, account) { await updateSessionPrivileges(req, user, false); if (!req.session.privileges.STORE) { + await rateLimiterPenalty(req.ip); throw new Error('STORE privilege required to grant access'); } @@ -90,6 +92,7 @@ module.exports = function (hostIdentity, jwtSecret, account) { try { redirectOrigin = new URL(req.session.oauthParams.redirect_uri).origin; } catch (err) { + await rateLimiterBlock(req.ip, 61); throw new Error('Application origin is bad', { cause: err }); } diff --git a/package-lock.json b/package-lock.json index 37c72fc..e9ef258 100644 --- a/package-lock.json +++ b/package-lock.json @@ -28,6 +28,7 @@ "mkdirp": "^1.0.4", "node-mocks-http": "^1.14.1", "proquint": "^0.0.1", + "rate-limiter-flexible": "^5.0.3", "robots.txt": "^1.1.0", "winston": "^3.11.0", "yaml": "^2.4.0" @@ -5712,6 +5713,11 @@ "node": ">= 0.6" } }, + "node_modules/rate-limiter-flexible": { + "version": "5.0.3", + "resolved": "https://registry.npmjs.org/rate-limiter-flexible/-/rate-limiter-flexible-5.0.3.tgz", + "integrity": "sha512-lWx2y8NBVlTOLPyqs+6y7dxfEpT6YFqKy3MzWbCy95sTTOhOuxufP2QvRyOHpfXpB9OUJPbVLybw3z3AVAS5fA==" + }, "node_modules/raw-body": { "version": "2.5.2", "resolved": "https://registry.npmjs.org/raw-body/-/raw-body-2.5.2.tgz", @@ -11132,6 +11138,11 @@ "resolved": "https://registry.npmjs.org/range-parser/-/range-parser-1.2.1.tgz", "integrity": "sha512-Hrgsx+orqoygnmhFbKaHE6c296J+HTAQXoxEF6gNupROmmGJRoyzfG3ccAveqCBrwr/2yxQ5BVd/GTl5agOwSg==" }, + "rate-limiter-flexible": { + "version": "5.0.3", + "resolved": "https://registry.npmjs.org/rate-limiter-flexible/-/rate-limiter-flexible-5.0.3.tgz", + "integrity": "sha512-lWx2y8NBVlTOLPyqs+6y7dxfEpT6YFqKy3MzWbCy95sTTOhOuxufP2QvRyOHpfXpB9OUJPbVLybw3z3AVAS5fA==" + }, "raw-body": { "version": "2.5.2", "resolved": "https://registry.npmjs.org/raw-body/-/raw-body-2.5.2.tgz", diff --git a/package.json b/package.json index 6aab090..33a7d04 100644 --- a/package.json +++ b/package.json @@ -41,6 +41,7 @@ "mkdirp": "^1.0.4", "node-mocks-http": "^1.14.1", "proquint": "^0.0.1", + "rate-limiter-flexible": "^5.0.3", "robots.txt": "^1.1.0", "winston": "^3.11.0", "yaml": "^2.4.0" diff --git a/spec/modular/m_not_found.spec.js b/spec/modular/m_not_found.spec.js index 9376deb..26f96eb 100644 --- a/spec/modular/m_not_found.spec.js +++ b/spec/modular/m_not_found.spec.js @@ -30,14 +30,21 @@ describe('Nonexistant resource (modular)', function () { /** This extends the test in shouldHandleNonexistingResource */ it('should say cacheable for 25 minutes', async function () { - const res = await chai.request(this.app).get('/fizbin'); + const res = await chai.request(this.app).get('/account/wildebeest/'); expect(res).to.have.status(404); expect(res).to.have.header('Cache-Control', /max-age=\d{4}/); expect(res).to.have.header('Strict-Transport-Security', /^max-age=/); expect(res).to.have.header('ETag'); expect(res.text).to.contain('Not Found — Armadietto'); - expect(res.text).to.contain('>“fizbin” doesn't exist<'); + expect(res.text).to.contain('>“account/wildebeest/” doesn't exist<'); + }); + + it('should curtly & cache-ably refuse to serve unlikely paths', async function () { + const res = await chai.request(this.app).get('/_profiler/phpinfo'); + expect(res).to.have.status(404); + expect(res).to.have.header('Cache-Control', /max-age=\d{4}/); + expect(res.text).to.equal(''); }); /** This tests that 404 for nonexistent assets is cache-able */ diff --git a/spec/not_found.spec.js b/spec/not_found.spec.js index c255fdd..31c9753 100644 --- a/spec/not_found.spec.js +++ b/spec/not_found.spec.js @@ -7,7 +7,7 @@ chai.use(chaiHttp); exports.shouldHandleNonexistingResource = function () { it('should return 404 Not Found', async function () { - const res = await chai.request(this.app).get('/zorp/gnu/'); + const res = await chai.request(this.app).get('/account/wildebeest/'); expect(res).to.have.status(404); expect(res).to.have.header('Content-Security-Policy', /sandbox.*default-src 'self'/); expect(res).to.have.header('Referrer-Policy', 'no-referrer'); @@ -21,7 +21,7 @@ exports.shouldHandleNonexistingResource = function () { // expect(res.text).to.contain('Not Found — Armadietto'); expect(res.text).to.match(/Something went wrong<\/h\d>/); expect(res.text).to.contain('>404<'); - expect(res.text).to.contain('>“zorp/gnu/” doesn't exist<'); + expect(res.text).to.contain('>“account/wildebeest/” doesn't exist<'); // navigation expect(res.text).to.match(/]*href="\/"[^>]*>Home<\/a>/);