From 6408d2313cf61ebe59a63b236bc8cf05ee8327be Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Wed, 28 Oct 2020 17:38:46 -0400
Subject: [PATCH] webaccess: Be extra paranoid about nullish password

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.
---
 src/node/hooks/express/webaccess.js |  4 ++--
 tests/backend/specs/webaccess.js    | 18 +++++++++++++++++-
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/src/node/hooks/express/webaccess.js b/src/node/hooks/express/webaccess.js
index f527ce22c..707babe68 100644
--- a/src/node/hooks/express/webaccess.js
+++ b/src/node/hooks/express/webaccess.js
@@ -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;
diff --git a/tests/backend/specs/webaccess.js b/tests/backend/specs/webaccess.js
index cf5dfcf85..4d0793d65 100644
--- a/tests/backend/specs/webaccess.js
+++ b/tests/backend/specs/webaccess.js
@@ -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() {