NoLog.cz/etherpad
6
0
Fork 0
Code Issues Pull requests Projects Releases Packages 1 Wiki Activity

webaccess: Be extra paranoid about nullish password

Browse source 
If `settings.json` contains a user without a `password` property then
nobody should be able to log in as that user using the built-in HTTP
basic authentication. This is true both with and without this change,
but before this change it wasn't immediately obvious that a malicious
user couldn't use an empty or null password to log in as such a user.
This commit adds an explicit nullish check and some unit tests to
ensure that an empty or null password will not work if the `password`
property is null or undefined.
This commit is contained in:
Richard Hansen 2020-10-28 17:38:46 -04:00 committed by John McLear
parent 98de2b0899
commit 6408d2313c
2 changed files with 19 additions and 3 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
src/node/hooks/express/webaccess.js
View file

@ -153,8 +153,8 @@ exports.checkAccess = (req, res, next) => {
hooks.aCallFirst('authenticate', ctx, hookResultMangle((ok) => {
if (!ok) {
// Fall back to HTTP basic auth.
if (!httpBasicAuth || !(ctx.username in settings.users) ||
settings.users[ctx.username].password !== ctx.password) {
const {[ctx.username]: {password} = {}} = settings.users;
if (!httpBasicAuth || password == null || password !== ctx.password) {
httpLogger.info(`Failed authentication from IP ${req.ip}`);
return hooks.aCallFirst('authnFailure', {req, res}, hookResultMangle((ok) => {
if (ok) return;

18
tests/backend/specs/webaccess.js
View file

@ -1,4 +1,4 @@
/* global __dirname, __filename, afterEach, before, beforeEach, describe, it, require */
/* global __dirname, __filename, Buffer, afterEach, before, beforeEach, describe, it, require */
function m(mod) { return __dirname + '/../../../src/' + mod; }
@ -91,6 +91,22 @@ describe(__filename, function() {
settings.requireAuthorization = true;
await agent.get('/admin/').auth('admin', 'admin-password').expect(200);
});
describe('login fails if password is nullish', function() {
for (const adminPassword of [undefined, null]) {
// https://tools.ietf.org/html/rfc7617 says that the username and password are sent as
// base64(username + ':' + password), but there's nothing stopping a malicious user from
// sending just base64(username) (no colon). The lack of colon could throw off credential
// parsing, resulting in successful comparisons against a null or undefined password.
for (const creds of ['admin', 'admin:']) {
it(`admin password: ${adminPassword} credentials: ${creds}`, async function() {
settings.users.admin.password = adminPassword;
const encCreds = Buffer.from(creds).toString('base64');
await agent.get('/admin/').set('Authorization', `Basic ${encCreds}`).expect(401);
});
}
}
});
});
describe('webaccess: preAuthorize, authenticate, and authorize hooks', function() {