libretic/etherpad-lite
6
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

SecurityManager: Refactor checkAccess for readability, correctness

Browse source 
* Move session validity check and session author ID fetch to a
    separate function. This separate function can be used by hooks,
    making it easier for them to properly determine the author ID.
  * Rewrite the remainder of checkAccess. Benefits:
      - The function is more readable and maintainable now.
      - Vulnerability fix: Before, the session IDs in sessionCookie
        were not validated when checking settings.requireSession. Now,
        sessionCookie must identify a valid session for the
        settings.requireSession test to pass.
      - Bug fix: Before, checkAccess would sometimes use the author ID
        associated with the token even if sessionCookie identified a
        valid session. Now it always uses the author ID associated
        with the session if available.
This commit is contained in:
Richard Hansen 2020-09-10 12:47:59 -04:00 committed by John McLear
parent 8756fed80d
commit 8b0baa9679
3 changed files with 136 additions and 210 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

253
src/node/db/SecurityManager.js
View file

@ -48,234 +48,77 @@ const NEED_PASSWORD = Object.freeze({accessStatus: 'needPassword'});
*
* WARNING: Tokens and session IDs MUST be kept secret, otherwise users will be able to impersonate
* each other (which might allow them to gain privileges).
*
* TODO: Add a hook so that plugins can make access decisions.
*/
exports.checkAccess = async function(padID, sessionCookie, token, password)
{
if (!padID) {
authLogger.debug('access denied: missing padID');
return DENY;
}
// allow plugins to deny access
var deniedByHook = hooks.callAll("onAccessCheck", {'padID': padID, 'password': password, 'token': token, 'sessionCookie': sessionCookie}).indexOf(false) > -1;
if (deniedByHook) {
const isFalse = (x) => x === false;
if (hooks.callAll('onAccessCheck', {padID, password, token, sessionCookie}).some(isFalse)) {
authLogger.debug('access denied: an onAccessCheck hook function returned false');
return DENY;
}
// start to get author for this token
let p_tokenAuthor = authorManager.getAuthor4Token(token);
// start fetching the info we may need
const p_sessionAuthorID = sessionManager.findAuthorID(padID.split('$')[0], sessionCookie);
const p_tokenAuthorID = authorManager.getAuthor4Token(token);
const p_padExists = padManager.doesPadExist(padID);
// start to check if pad exists
let p_padExists = padManager.doesPadExist(padID);
const padExists = await p_padExists;
if (!padExists && settings.editOnly) {
authLogger.debug('access denied: user attempted to create a pad, which is prohibited');
return DENY;
}
if (settings.requireSession) {
// a valid session is required (api-only mode)
if (!sessionCookie) {
// without sessionCookie, access is denied
const sessionAuthorID = await p_sessionAuthorID;
if (settings.requireSession && !sessionAuthorID) {
authLogger.debug('access denied: HTTP API session is required');
return DENY;
}
const grant = {
accessStatus: 'grant',
authorID: (sessionAuthorID != null) ? sessionAuthorID : await p_tokenAuthorID,
};
if (!padID.includes('$')) {
// Only group pads can be private or have passwords, so there is nothing more to check for this
// non-group pad.
return grant;
}
if (!padExists) {
if (sessionAuthorID == null) {
authLogger.debug('access denied: must have an HTTP API session to create a group pad');
return DENY;
}
} else {
// a session is not required, so we'll check if it's a public pad
if (padID.indexOf("$") === -1) {
// it's not a group pad, means we can grant access
if (settings.editOnly && !(await p_padExists)) return DENY;
return {accessStatus: 'grant', authorID: await p_tokenAuthor};
}
// Creating a group pad, so there is no password or public status to check.
return grant;
}
let validSession = false;
let sessionAuthor;
let isPublic;
let isPasswordProtected;
let passwordStatus = password == null ? "notGiven" : "wrong"; // notGiven, correct, wrong
const pad = await padManager.getPad(padID);
// get information about all sessions contained in this cookie
if (sessionCookie) {
let groupID = padID.split("$")[0];
/*
* Sometimes, RFC 6265-compliant web servers may send back a cookie whose
* value is enclosed in double quotes, such as:
*
* Set-Cookie: sessionCookie="s.37cf5299fbf981e14121fba3a588c02b,s.2b21517bf50729d8130ab85736a11346"; Version=1; Path=/; Domain=localhost; Discard
*
* Where the double quotes at the start and the end of the header value are
* just delimiters. This is perfectly legal: Etherpad parsing logic should
* cope with that, and remove the quotes early in the request phase.
*
* Somehow, this does not happen, and in such cases the actual value that
* sessionCookie ends up having is:
*
* sessionCookie = '"s.37cf5299fbf981e14121fba3a588c02b,s.2b21517bf50729d8130ab85736a11346"'
*
* As quick measure, let's strip the double quotes (when present).
* Note that here we are being minimal, limiting ourselves to just removing
* quotes at the start and the end of the string.
*
* Fixes #3819.
* Also, see #3820.
*/
let sessionIDs = sessionCookie.replace(/^"|"$/g, '').split(',');
// was previously iterated in parallel using async.forEach
try {
let sessionInfos = await Promise.all(sessionIDs.map(sessionID => {
return sessionManager.getSessionInfo(sessionID);
}));
// seperated out the iteration of sessioninfos from the (parallel) fetches from the DB
for (let sessionInfo of sessionInfos) {
// is it for this group?
if (sessionInfo.groupID != groupID) {
authLogger.debug("Auth failed: wrong group");
continue;
}
// is validUntil still ok?
let now = Math.floor(Date.now() / 1000);
if (sessionInfo.validUntil <= now) {
authLogger.debug("Auth failed: validUntil");
continue;
}
// fall-through - there is a valid session
validSession = true;
sessionAuthor = sessionInfo.authorID;
break;
}
} catch (err) {
// skip session if it doesn't exist
if (err.message == "sessionID does not exist") {
authLogger.debug("Auth failed: unknown session");
} else {
throw err;
}
}
if (!pad.getPublicStatus() && sessionAuthorID == null) {
authLogger.debug('access denied: must have an HTTP API session to access private group pads');
return DENY;
}
let padExists = await p_padExists;
if (padExists) {
let pad = await padManager.getPad(padID);
// is it a public pad?
isPublic = pad.getPublicStatus();
// is it password protected?
isPasswordProtected = pad.isPasswordProtected();
// is password correct?
if (isPasswordProtected && password && pad.isCorrectPassword(password)) {
passwordStatus = "correct";
}
}
// - a valid session for this group is avaible AND pad exists
if (validSession && padExists) {
let authorID = sessionAuthor;
let grant = Object.freeze({ accessStatus: "grant", authorID });
if (!isPasswordProtected) {
// - the pad is not password protected
// --> grant access
return grant;
}
if (settings.sessionNoPassword) {
// - the setting to bypass password validation is set
// --> grant access
return grant;
}
if (isPasswordProtected && passwordStatus === "correct") {
// - the pad is password protected and password is correct
// --> grant access
return grant;
}
if (isPasswordProtected && passwordStatus === "wrong") {
// - the pad is password protected but wrong password given
// --> deny access, ask for new password and tell them that the password is wrong
return WRONG_PASSWORD;
}
if (isPasswordProtected && passwordStatus === "notGiven") {
// - the pad is password protected but no password given
// --> ask for password
const passwordExempt = settings.sessionNoPassword && sesionAuthorID != null;
const requirePassword = pad.isPasswordProtected() && !passwordExempt;
if (requirePassword) {
if (password == null) {
authLogger.debug('access denied: password required');
return NEED_PASSWORD;
}
throw new Error("Oops, something wrong happend");
}
if (validSession && !padExists) {
// - a valid session for this group avaible but pad doesn't exist
// --> grant access by default
let accessStatus = "grant";
let authorID = sessionAuthor;
// --> deny access if user isn't allowed to create the pad
if (settings.editOnly) {
authLogger.debug("Auth failed: valid session & pad does not exist");
return DENY;
}
return { accessStatus, authorID };
}
if (!validSession && padExists) {
// there is no valid session avaiable AND pad exists
let authorID = await p_tokenAuthor;
let grant = Object.freeze({ accessStatus: "grant", authorID });
if (isPublic && !isPasswordProtected) {
// -- it's public and not password protected
// --> grant access, with author of token
return grant;
}
if (isPublic && isPasswordProtected && passwordStatus === "correct") {
// - it's public and password protected and password is correct
// --> grant access, with author of token
return grant;
}
if (isPublic && isPasswordProtected && passwordStatus === "wrong") {
// - it's public and the pad is password protected but wrong password given
// --> deny access, ask for new password and tell them that the password is wrong
if (!password || !pad.isCorrectPassword(password)) {
authLogger.debug('access denied: wrong password');
return WRONG_PASSWORD;
}
if (isPublic && isPasswordProtected && passwordStatus === "notGiven") {
// - it's public and the pad is password protected but no password given
// --> ask for password
return NEED_PASSWORD;
}
if (!isPublic) {
// - it's not public
authLogger.debug("Auth failed: invalid session & pad is not public");
// --> deny access
return DENY;
}
throw new Error("Oops, something wrong happend");
}
// there is no valid session avaiable AND pad doesn't exist
authLogger.debug("Auth failed: invalid session & pad does not exist");
return DENY;
}
return grant;
};

54
src/node/db/SessionManager.js
View file

@ -19,11 +19,65 @@
*/
var customError = require("../utils/customError");
const promises = require('../utils/promises');
var randomString = require("../utils/randomstring");
var db = require("./DB");
var groupManager = require("./GroupManager");
var authorManager = require("./AuthorManager");
/**
* Finds the author ID for a session with matching ID and group.
*
* @param groupID identifies the group the session is bound to.
* @param sessionCookie contains a comma-separated list of IDs identifying the sessions to search.
* @return If there is a session that is not expired, has an ID matching one of the session IDs in
* sessionCookie, and is bound to a group with the given ID, then this returns the author ID
* bound to the session. Otherwise, returns undefined.
*/
exports.findAuthorID = async (groupID, sessionCookie) => {
if (!sessionCookie) return undefined;
/*
* Sometimes, RFC 6265-compliant web servers may send back a cookie whose
* value is enclosed in double quotes, such as:
*
* Set-Cookie: sessionCookie="s.37cf5299fbf981e14121fba3a588c02b,s.2b21517bf50729d8130ab85736a11346"; Version=1; Path=/; Domain=localhost; Discard
*
* Where the double quotes at the start and the end of the header value are
* just delimiters. This is perfectly legal: Etherpad parsing logic should
* cope with that, and remove the quotes early in the request phase.
*
* Somehow, this does not happen, and in such cases the actual value that
* sessionCookie ends up having is:
*
* sessionCookie = '"s.37cf5299fbf981e14121fba3a588c02b,s.2b21517bf50729d8130ab85736a11346"'
*
* As quick measure, let's strip the double quotes (when present).
* Note that here we are being minimal, limiting ourselves to just removing
* quotes at the start and the end of the string.
*
* Fixes #3819.
* Also, see #3820.
*/
const sessionIDs = sessionCookie.replace(/^"|"$/g, '').split(',');
const sessionInfoPromises = sessionIDs.map(async (id) => {
try {
return await exports.getSessionInfo(id);
} catch (err) {
if (err.message === 'sessionID does not exist') {
console.debug(`SessionManager getAuthorID: no session exists with ID ${id}`);
} else {
throw err;
}
}
return undefined;
});
const now = Math.floor(Date.now() / 1000);
const isMatch = (si) => (si != null && si.groupID === groupID && si.validUntil <= now);
const sessionInfo = await promises.firstSatisfies(sessionInfoPromises, isMatch);
if (sessionInfo == null) return undefined;
return sessionInfo.authorID;
};
exports.doesSessionExist = async function(sessionID)
{
//check if the database entry of this session exists

39
src/node/utils/promises.js
View file

@ -2,7 +2,40 @@
* Helpers to manipulate promises (like async but for promises).
*/
var timesLimit = function (ltMax, concurrency, promiseCreator) {
// Returns a Promise that resolves to the first resolved value from `promises` that satisfies
// `predicate`. Resolves to `undefined` if none of the Promises satisfy `predicate`, or if
// `promises` is empty. If `predicate` is nullish, the truthiness of the resolved value is used as
// the predicate.
exports.firstSatisfies = (promises, predicate) => {
if (predicate == null) predicate = (x) => x;
// Transform each original Promise into a Promise that never resolves if the original resolved
// value does not satisfy `predicate`. These transformed Promises will be passed to Promise.race,
// yielding the first resolved value that satisfies `predicate`.
const newPromises = promises.map(
(p) => new Promise((resolve, reject) => p.then((v) => predicate(v) && resolve(v), reject)));
// If `promises` is an empty array or if none of them resolve to a value that satisfies
// `predicate`, then `Promise.race(newPromises)` will never resolve. To handle that, add another
// Promise that resolves to `undefined` after all of the original Promises resolve.
//
// Note: If all of the original Promises simultaneously resolve to a value that satisfies
// `predicate` (perhaps they were already resolved when this function was called), then this
// Promise will resolve too, and with a value of `undefined`. There is no concern that this
// Promise will win the race and thus cause an erroneous `undefined` result. This is because
// a resolved Promise's `.then()` function is scheduled for execution -- not executed right away
// -- and ES guarantees in-order execution of the enqueued invocations. Each of the above
// transformed Promises has a `.then()` chain of length one, while the Promise added here has a
// `.then()` chain of length two or more (at least one `.then()` that is internal to
// `Promise.all()`, plus the `.then()` function added here). By the time the `.then()` function
// added here executes, all of the above transformed Promises will have already resolved and one
// will have been chosen as the winner.
newPromises.push(Promise.all(promises).then(() => {}));
return Promise.race(newPromises);
};
exports.timesLimit = function(ltMax, concurrency, promiseCreator) {
var done = 0
var current = 0
@ -26,7 +59,3 @@ var timesLimit = function (ltMax, concurrency, promiseCreator) {
addAnother()
}
}
module.exports = {
timesLimit: timesLimit
}