From b80a37173efe592c07c89e2e16a7fe6602441ed5 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Fri, 11 Sep 2020 19:26:26 -0400 Subject: [PATCH] security: Fix authorization bypass vulnerability Before, a malicious user could bypass authorization restrictions imposed by the authorize hook: * Step 1: Fetch any resource that the malicious user is authorized to access (e.g., static content). * Step 2: Use the signed express_sid cookie generated in step 1 to create a socket.io connection. * Step 3: Perform the CLIENT_READY handshake for the desired pad. * Step 4: Profit! Now the authorization decision made by the authorize hook is propagated to SecurityManager so that it can approve or reject socket.io messages as appropriate. This also sets up future support for per-user read-only and modify-only (no create) authorization levels. --- doc/api/hooks_server-side.md | 27 +++++++++++--------- src/node/db/SecurityManager.js | 21 ++++++++++++---- src/node/hooks/express/webaccess.js | 38 +++++++++++++++++++++++------ tests/backend/specs/socketio.js | 2 +- 4 files changed, 63 insertions(+), 25 deletions(-) diff --git a/doc/api/hooks_server-side.md b/doc/api/hooks_server-side.md index c249696dd..b4ef1e525 100644 --- a/doc/api/hooks_server-side.md +++ b/doc/api/hooks_server-side.md @@ -228,24 +228,27 @@ following are true: different plugin has not already caused the post-authentication authorization to pass or fail. -For pre-authentication invocations of your authorize function, calling the -provided callback with `[true]` will immediately grant access without requiring -the user to authenticate. Calling the provided callback with `[false]` will -trigger authentication unless authentication is not required. Calling the -provided callback with `[]` or `undefined` will defer the decision to the next -authorization plugin (if any, otherwise it is the same as calling with -`[false]`). +For pre-authentication invocations of your authorize function, you can pass the +following values to the provided callback: + +* `[true]` or `['create']` will immediately grant access without requiring the + user to authenticate. +* `[false]` will trigger authentication unless authentication is not required. +* `[]` or `undefined` will defer the decision to the next authorization plugin + (if any, otherwise it is the same as calling with `[false]`). **WARNING:** Your authorize function can be called for an `/admin` page even if the user has not yet authenticated. It is your responsibility to fail or defer authorization if you do not want to grant admin privileges to the general public. -For post-authentication invocations of your authorize function, calling the -provided callback with `[true]` or `[false]` will cause access to be granted or -denied, respectively. Calling the callback with `[]` or `undefined` will defer -the authorization decision to the next authorization plugin (if any, otherwise -deny). +For post-authentication invocations of your authorize function, you can pass the +following values to the provided callback: + +* `[true]` or `['create']` will grant access. +* `[false]` will deny access. +* `[]` or `undefined` will defer the authorization decision to the next + authorization plugin (if any, otherwise deny). Example: diff --git a/src/node/db/SecurityManager.js b/src/node/db/SecurityManager.js index f4c7af880..781f47e85 100644 --- a/src/node/db/SecurityManager.js +++ b/src/node/db/SecurityManager.js @@ -23,6 +23,7 @@ var hooks = require("ep_etherpad-lite/static/js/pluginfw/hooks.js"); var padManager = require("./PadManager"); var sessionManager = require("./SessionManager"); var settings = require("../utils/Settings"); +const webaccess = require('../hooks/express/webaccess'); var log4js = require('log4js'); var authLogger = log4js.getLogger("auth"); @@ -57,11 +58,21 @@ exports.checkAccess = async function(padID, sessionCookie, token, password, user return DENY; } - // Make sure the user has authenticated if authentication is required. The caller should have - // already performed this check, but it is repeated here just in case. - if (settings.requireAuthentication && userSettings == null) { - authLogger.debug('access denied: authentication is required'); - return DENY; + if (settings.requireAuthentication) { + // Make sure the user has authenticated if authentication is required. The caller should have + // already performed this check, but it is repeated here just in case. + if (userSettings == null) { + authLogger.debug('access denied: authentication is required'); + return DENY; + } + // Check whether the user is authorized. Note that userSettings.padAuthorizations will still be + // populated even if settings.requireAuthorization is false. + const padAuthzs = userSettings.padAuthorizations || {}; + const level = webaccess.normalizeAuthzLevel(padAuthzs[padID]); + if (!level) { + authLogger.debug('access denied: unauthorized'); + return DENY; + } } // allow plugins to deny access diff --git a/src/node/hooks/express/webaccess.js b/src/node/hooks/express/webaccess.js index 1f92bbea0..b83fbbd00 100644 --- a/src/node/hooks/express/webaccess.js +++ b/src/node/hooks/express/webaccess.js @@ -8,6 +8,19 @@ const stats = require('ep_etherpad-lite/node/stats'); const sessionModule = require('express-session'); const cookieParser = require('cookie-parser'); +exports.normalizeAuthzLevel = (level) => { + if (!level) return false; + switch (level) { + case true: + return 'create'; + case 'create': + return level; + default: + httpLogger.warn(`Unknown authorization level '${level}', denying access`); + } + return false; +}; + exports.checkAccess = (req, res, next) => { const hookResultMangle = (cb) => { return (err, data) => { @@ -21,17 +34,28 @@ exports.checkAccess = (req, res, next) => { // Do not require auth for static paths and the API...this could be a bit brittle if (req.path.match(/^\/(static|javascripts|pluginfw|api)/)) return next(); + const grant = (level) => { + level = exports.normalizeAuthzLevel(level); + if (!level) return fail(); + const user = req.session.user; + if (user == null) return next(); // This will happen if authentication is not required. + const padID = (req.path.match(/^\/p\/(.*)$/) || [])[1]; + if (padID == null) return next(); + // The user was granted access to a pad. Remember the authorization level in the user's + // settings so that SecurityManager can approve or deny specific actions. + if (user.padAuthorizations == null) user.padAuthorizations = {}; + user.padAuthorizations[padID] = level; + return next(); + }; + if (req.path.toLowerCase().indexOf('/admin') !== 0) { - if (!settings.requireAuthentication) return next(); - if (!settings.requireAuthorization && req.session && req.session.user) return next(); + if (!settings.requireAuthentication) return grant('create'); + if (!settings.requireAuthorization && req.session && req.session.user) return grant('create'); } - if (req.session && req.session.user && req.session.user.is_admin) return next(); + if (req.session && req.session.user && req.session.user.is_admin) return grant('create'); - hooks.aCallFirst('authorize', {req, res, next, resource: req.path}, hookResultMangle((ok) => { - if (ok) return next(); - return fail(); - })); + hooks.aCallFirst('authorize', {req, res, next, resource: req.path}, hookResultMangle(grant)); }; /* Authentication OR authorization failed. */ diff --git a/tests/backend/specs/socketio.js b/tests/backend/specs/socketio.js index b38aa1f6b..9db6bdbeb 100644 --- a/tests/backend/specs/socketio.js +++ b/tests/backend/specs/socketio.js @@ -178,7 +178,7 @@ describe('socket.io access checks', () => { settings.requireAuthentication = true; await assert.rejects(connect(null), {message: /signed express_sid cookie is required/i}); }); - xit('authorization bypass attempt -> error', async () => { + it('authorization bypass attempt -> error', async () => { plugins.hooks.authorize = [{hook_fn: (hookName, {req}, cb) => { if (req.session.user == null) return cb([]); // Hasn't authenticated yet. // Only allowed to access /p/pad.