From 490b595a01c5824ff63ffb87f0efdfca95f4bf3b Mon Sep 17 00:00:00 2001 From: Chocobozzz Date: Thu, 29 Mar 2018 10:58:24 +0200 Subject: Prevent brute force login attack --- server/controllers/api/users.ts | 14 +++++- server/controllers/api/videos/index.ts | 2 +- server/initializers/checker.ts | 1 + server/initializers/constants.ts | 9 ++++ server/initializers/installer.ts | 2 +- server/tests/api/server/reverse-proxy.ts | 82 ++++++++++++++++++++++++++++++++ server/tests/utils/videos/videos.ts | 11 +++-- 7 files changed, 114 insertions(+), 7 deletions(-) create mode 100644 server/tests/api/server/reverse-proxy.ts (limited to 'server') diff --git a/server/controllers/api/users.ts b/server/controllers/api/users.ts index 583376c38..5e96d789e 100644 --- a/server/controllers/api/users.ts +++ b/server/controllers/api/users.ts @@ -2,12 +2,13 @@ import * as express from 'express' import 'multer' import { extname, join } from 'path' import * as uuidv4 from 'uuid/v4' +import * as RateLimit from 'express-rate-limit' import { UserCreate, UserRight, UserRole, UserUpdate, UserUpdateMe, UserVideoRate as FormattedUserVideoRate } from '../../../shared' import { retryTransactionWrapper } from '../../helpers/database-utils' import { processImage } from '../../helpers/image-utils' import { logger } from '../../helpers/logger' import { createReqFiles, getFormattedObjects } from '../../helpers/utils' -import { AVATARS_SIZE, CONFIG, IMAGE_MIMETYPE_EXT, sequelizeTypescript } from '../../initializers' +import { AVATARS_SIZE, CONFIG, IMAGE_MIMETYPE_EXT, RATES_LIMIT, sequelizeTypescript } from '../../initializers' import { updateActorAvatarInstance } from '../../lib/activitypub' import { sendUpdateActor } from '../../lib/activitypub/send' import { Emailer } from '../../lib/emailer' @@ -43,6 +44,11 @@ import { OAuthTokenModel } from '../../models/oauth/oauth-token' import { VideoModel } from '../../models/video/video' const reqAvatarFile = createReqFiles([ 'avatarfile' ], IMAGE_MIMETYPE_EXT, { avatarfile: CONFIG.STORAGE.AVATARS_DIR }) +const loginRateLimiter = new RateLimit({ + windowMs: RATES_LIMIT.LOGIN.WINDOW_MS, + max: RATES_LIMIT.LOGIN.MAX, + delayMs: 0 +}) const usersRouter = express.Router() @@ -136,7 +142,11 @@ usersRouter.post('/:id/reset-password', asyncMiddleware(resetUserPassword) ) -usersRouter.post('/token', token, success) +usersRouter.post('/token', + loginRateLimiter, + token, + success +) // TODO: Once https://github.com/oauthjs/node-oauth2-server/pull/289 is merged, implement revoke token route // --------------------------------------------------------------------------- diff --git a/server/controllers/api/videos/index.ts b/server/controllers/api/videos/index.ts index c0a8ac118..552e5edac 100644 --- a/server/controllers/api/videos/index.ts +++ b/server/controllers/api/videos/index.ts @@ -353,7 +353,7 @@ function getVideo (req: express.Request, res: express.Response) { async function viewVideo (req: express.Request, res: express.Response) { const videoInstance = res.locals.video - const ip = req.headers['x-real-ip'] as string || req.ip + const ip = req.ip const exists = await Redis.Instance.isViewExists(ip, videoInstance.uuid) if (exists) { logger.debug('View for ip %s and video %s already exists.', ip, videoInstance.uuid) diff --git a/server/initializers/checker.ts b/server/initializers/checker.ts index cd93f19a9..45f1d79c3 100644 --- a/server/initializers/checker.ts +++ b/server/initializers/checker.ts @@ -20,6 +20,7 @@ function checkConfig () { function checkMissedConfig () { const required = [ 'listen.port', 'webserver.https', 'webserver.hostname', 'webserver.port', + 'trust_proxy', 'database.hostname', 'database.port', 'database.suffix', 'database.username', 'database.password', 'redis.hostname', 'redis.port', 'redis.auth', 'smtp.hostname', 'smtp.port', 'smtp.username', 'smtp.password', 'smtp.tls', 'smtp.from_address', diff --git a/server/initializers/constants.ts b/server/initializers/constants.ts index 284acf8f3..986fed099 100644 --- a/server/initializers/constants.ts +++ b/server/initializers/constants.ts @@ -127,6 +127,7 @@ const CONFIG = { URL: '', HOST: '' }, + TRUST_PROXY: config.get('trust_proxy'), LOG: { LEVEL: config.get('log.level') }, @@ -234,6 +235,13 @@ const CONSTRAINTS_FIELDS = { } } +const RATES_LIMIT = { + LOGIN: { + WINDOW_MS: 5 * 60 * 1000, // 5 minutes + MAX: 10 // 10 attempts + } +} + let VIDEO_VIEW_LIFETIME = 60000 * 60 // 1 hour const VIDEO_TRANSCODING_FPS = { MIN: 10, @@ -468,6 +476,7 @@ export { USER_PASSWORD_RESET_LIFETIME, IMAGE_MIMETYPE_EXT, SCHEDULER_INTERVAL, + RATES_LIMIT, JOB_COMPLETED_LIFETIME, VIDEO_VIEW_LIFETIME } diff --git a/server/initializers/installer.ts b/server/initializers/installer.ts index d2f6c7c8c..f0adf8c9e 100644 --- a/server/initializers/installer.ts +++ b/server/initializers/installer.ts @@ -112,7 +112,7 @@ async function createOAuthAdminIfNotExist () { // Our password is weak so do not validate it validatePassword = false } else { - password = passwordGenerator(8, true) + password = passwordGenerator(16, true) } const userData = { diff --git a/server/tests/api/server/reverse-proxy.ts b/server/tests/api/server/reverse-proxy.ts new file mode 100644 index 000000000..aa4b3ae81 --- /dev/null +++ b/server/tests/api/server/reverse-proxy.ts @@ -0,0 +1,82 @@ +/* tslint:disable:no-unused-expression */ + +import 'mocha' +import * as chai from 'chai' +import { About } from '../../../../shared/models/server/about.model' +import { CustomConfig } from '../../../../shared/models/server/custom-config.model' +import { deleteCustomConfig, getAbout, getVideo, killallServers, login, reRunServer, uploadVideo, userLogin, viewVideo } from '../../utils' +const expect = chai.expect + +import { + getConfig, + flushTests, + runServer, + registerUser, getCustomConfig, setAccessTokensToServers, updateCustomConfig +} from '../../utils/index' + +describe('Test application behind a reverse proxy', function () { + let server = null + let videoId + + before(async function () { + this.timeout(30000) + + await flushTests() + server = await runServer(1) + await setAccessTokensToServers([ server ]) + + const { body } = await uploadVideo(server.url, server.accessToken, {}) + videoId = body.video.uuid + }) + + it('Should view a video only once with the same IP by default', async function () { + await viewVideo(server.url, videoId) + await viewVideo(server.url, videoId) + + const { body } = await getVideo(server.url, videoId) + expect(body.views).to.equal(1) + }) + + it('Should view a video 2 times with the X-Forwarded-For header set', async function () { + await viewVideo(server.url, videoId, 204, '0.0.0.1,127.0.0.1') + await viewVideo(server.url, videoId, 204, '0.0.0.2,127.0.0.1') + + const { body } = await getVideo(server.url, videoId) + expect(body.views).to.equal(3) + }) + + it('Should view a video only once with the same client IP in the X-Forwarded-For header', async function () { + await viewVideo(server.url, videoId, 204, '0.0.0.4,0.0.0.3,::ffff:127.0.0.1') + await viewVideo(server.url, videoId, 204, '0.0.0.5,0.0.0.3,127.0.0.1') + + const { body } = await getVideo(server.url, videoId) + expect(body.views).to.equal(4) + }) + + it('Should view a video two times with a different client IP in the X-Forwarded-For header', async function () { + await viewVideo(server.url, videoId, 204, '0.0.0.8,0.0.0.6,127.0.0.1') + await viewVideo(server.url, videoId, 204, '0.0.0.8,0.0.0.7,127.0.0.1') + + const { body } = await getVideo(server.url, videoId) + expect(body.views).to.equal(6) + }) + + it('Should rate limit logins', async function () { + const user = { username: 'root', password: 'fail' } + + for (let i = 0; i < 9; i++) { + await userLogin(server, user, 400) + } + + await userLogin(server, user, 429) + }) + + after(async function () { + process.kill(-server.app.pid) + + // Keep the logs if the test failed + if (this['ok']) { + await flushTests() + } + }) +}) diff --git a/server/tests/utils/videos/videos.ts b/server/tests/utils/videos/videos.ts index 424f41ed8..9bda53371 100644 --- a/server/tests/utils/videos/videos.ts +++ b/server/tests/utils/videos/videos.ts @@ -85,13 +85,18 @@ function getVideo (url: string, id: number | string, expectedStatus = 200) { .expect(expectedStatus) } -function viewVideo (url: string, id: number | string, expectedStatus = 204) { +function viewVideo (url: string, id: number | string, expectedStatus = 204, xForwardedFor?: string) { const path = '/api/v1/videos/' + id + '/views' - return request(url) + const req = request(url) .post(path) .set('Accept', 'application/json') - .expect(expectedStatus) + + if (xForwardedFor) { + req.set('X-Forwarded-For', xForwardedFor) + } + + return req.expect(expectedStatus) } function getVideoWithToken (url: string, token: string, id: number | string, expectedStatus = 200) { -- cgit v1.2.3