Merge pull request #2325 from overleaf/ta-http-auth-constant-time

Use Constant Time Comparison in for HTTP Authentication

GitOrigin-RevId: dc9604c18831293c6da3e96dd2b0488daaa81946

services/web/app/src/Features/Authentication/AuthenticationController.js

@@ -6,6 +6,7 @@ const logger = require('logger-sharelatex')
 const querystring = require('querystring')
 const Settings = require('settings-sharelatex')
 const basicAuth = require('basic-auth-connect')
+const crypto = require('crypto')
 const UserHandler = require('../User/UserHandler')
 const UserSessionsManager = require('../User/UserSessionsManager')
 const SessionStoreManager = require('../../infrastructure/SessionStoreManager')
@@ -362,7 +363,11 @@ const AuthenticationController = (module.exports = {
   },
 
   httpAuth: basicAuth(function(user, pass) {
-    const isValid = Settings.httpAuthUsers[user] === pass
+    let expectedPassword = Settings.httpAuthUsers[user]
+    const isValid =
+      expectedPassword &&
+      expectedPassword.length === pass.length &&
+      crypto.timingSafeEqual(Buffer.from(expectedPassword), Buffer.from(pass))
     if (!isValid) {
       logger.err({ user, pass }, 'invalid login details')
     }

services/web/test/unit/src/Authentication/AuthenticationControllerTests.js

@@ -13,6 +13,11 @@ describe('AuthenticationController', function() {
   beforeEach(function() {
     tk.freeze(Date.now())
     this.UserModel = { findOne: sinon.stub() }
+    this.httpAuthUsers = {
+      'valid-test-user': Math.random()
+        .toString(16)
+        .slice(2)
+    }
     this.AuthenticationController = SandboxedModule.require(modulePath, {
       globals: {
         console: console
@@ -40,7 +45,10 @@ describe('AuthenticationController', function() {
           error: sinon.stub(),
           err: sinon.stub()
         }),
-        'settings-sharelatex': { siteUrl: 'http://www.foo.bar' },
+        'settings-sharelatex': {
+          siteUrl: 'http://www.foo.bar',
+          httpAuthUsers: this.httpAuthUsers
+        },
         passport: (this.passport = {
           authenticate: sinon.stub().returns(sinon.stub())
         }),
@@ -781,6 +789,56 @@ describe('AuthenticationController', function() {
     })
   })
 
+  describe('httpAuth', function() {
+    describe('with http auth', function() {
+      it('should error with incorrect user', function(done) {
+        this.req.headers = {
+          authorization: `Basic ${Buffer.from('user:nope').toString('base64')}`
+        }
+        this.req.end = status => {
+          expect(status).to.equal('Unauthorized')
+          done()
+        }
+        this.AuthenticationController.httpAuth(this.req, this.req)
+      })
+
+      it('should error with incorrect password', function(done) {
+        this.req.headers = {
+          authorization: `Basic ${Buffer.from('valid-test-user:nope').toString(
+            'base64'
+          )}`
+        }
+        this.req.end = status => {
+          expect(status).to.equal('Unauthorized')
+          done()
+        }
+        this.AuthenticationController.httpAuth(this.req, this.req)
+      })
+
+      it('should fail with empty pass', function(done) {
+        this.req.headers = {
+          authorization: `Basic ${Buffer.from(`invalid-test-user:`).toString(
+            'base64'
+          )}`
+        }
+        this.req.end = status => {
+          expect(status).to.equal('Unauthorized')
+          done()
+        }
+        this.AuthenticationController.httpAuth(this.req, this.req)
+      })
+
+      it('should succeed with correct user/pass', function(done) {
+        this.req.headers = {
+          authorization: `Basic ${Buffer.from(
+            `valid-test-user:${this.httpAuthUsers['valid-test-user']}`
+          ).toString('base64')}`
+        }
+        this.AuthenticationController.httpAuth(this.req, this.res, done)
+      })
+    })
+  })
+
   describe('_redirectToLoginOrRegisterPage', function() {
     beforeEach(function() {
       this.middleware = this.AuthenticationController.requireLogin(

services/web/test/unit/src/helpers/MockRequest.js

@@ -20,6 +20,7 @@ class MockRequest {
     }
     this.prototype.route = { path: '' }
     this.prototype.accepts = () => {}
+    this.prototype.setHeader = () => {}
   }
   param(param) {
     return this.params[param]