From 98a0c540040e3ad98f392f6d2cf568fc4550bf8b Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 17 Jun 2016 12:11:39 +0100 Subject: [PATCH 1/2] use parameter for bcrypt rounds, rehash passwords on login if necessary --- .../AuthenticationManager.coffee | 16 +++++++-- services/web/config/settings.defaults.coffee | 1 + .../AuthenticationManagerTests.coffee | 33 ++++++++++++++++++- 3 files changed, 47 insertions(+), 3 deletions(-) diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee index 201643e88e..bfcd55855d 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee @@ -1,8 +1,11 @@ +Settings = require "settings-sharelatex" User = require("../../models/User").User {db, ObjectId} = require("../../infrastructure/mongojs") crypto = require 'crypto' bcrypt = require 'bcrypt' +BCRYPT_ROUNDS = Settings?.security?.bcryptRounds or 12 + module.exports = AuthenticationManager = authenticate: (query, password, callback = (error, user) ->) -> # Using Mongoose for legacy reasons here. The returned User instance @@ -15,7 +18,9 @@ module.exports = AuthenticationManager = bcrypt.compare password, user.hashedPassword, (error, match) -> return callback(error) if error? if match - callback null, user + AuthenticationManager.checkRounds user, user.hashedPassword, password, (err) -> + return callback(err) if err? + callback null, user else callback null, null else @@ -24,7 +29,7 @@ module.exports = AuthenticationManager = callback null, null setUserPassword: (user_id, password, callback = (error) ->) -> - bcrypt.genSalt 7, (error, salt) -> + bcrypt.genSalt BCRYPT_ROUNDS, (error, salt) -> return callback(error) if error? bcrypt.hash password, salt, (error, hash) -> return callback(error) if error? @@ -35,3 +40,10 @@ module.exports = AuthenticationManager = $unset: password: true }, callback) + checkRounds: (user, hashedPassword, password, callback = (error) ->) -> + # check current number of rounds and rehash if necessary + currentRounds = bcrypt.getRounds hashedPassword + if currentRounds < BCRYPT_ROUNDS + AuthenticationManager.setUserPassword user._id, password, callback + else + callback() diff --git a/services/web/config/settings.defaults.coffee b/services/web/config/settings.defaults.coffee index beb933e4c9..1950141518 100644 --- a/services/web/config/settings.defaults.coffee +++ b/services/web/config/settings.defaults.coffee @@ -137,6 +137,7 @@ module.exports = # -------- security: sessionSecret: sessionSecret + bcryptRounds: 14 httpAuthUsers: httpAuthUsers diff --git a/services/web/test/UnitTests/coffee/Authentication/AuthenticationManagerTests.coffee b/services/web/test/UnitTests/coffee/Authentication/AuthenticationManagerTests.coffee index f4d5e72c26..2805527259 100644 --- a/services/web/test/UnitTests/coffee/Authentication/AuthenticationManagerTests.coffee +++ b/services/web/test/UnitTests/coffee/Authentication/AuthenticationManagerTests.coffee @@ -16,6 +16,7 @@ describe "AuthenticationManager", -> users: {} ObjectId: ObjectId "bcrypt": @bcrypt = {} + "settings-sharelatex": { security: { bcryptRounds: 12 } } @callback = sinon.stub() describe "authenticate", -> @@ -31,6 +32,7 @@ describe "AuthenticationManager", -> beforeEach (done) -> @user.hashedPassword = @hashedPassword = "asdfjadflasdf" @bcrypt.compare = sinon.stub().callsArgWith(2, null, true) + @bcrypt.getRounds = sinon.stub().returns 12 @AuthenticationManager.authenticate email: @email, @unencryptedPassword, (error, user) => @callback(error, user) done() @@ -54,6 +56,35 @@ describe "AuthenticationManager", -> it "should not return the user", -> @callback.calledWith(null, null).should.equal true + describe "when the hashed password matches but the number of rounds is too low", -> + beforeEach (done) -> + @user.hashedPassword = @hashedPassword = "asdfjadflasdf" + @bcrypt.compare = sinon.stub().callsArgWith(2, null, true) + @bcrypt.getRounds = sinon.stub().returns 7 + @AuthenticationManager.setUserPassword = sinon.stub().callsArgWith(2, null) + @AuthenticationManager.authenticate email: @email, @unencryptedPassword, (error, user) => + @callback(error, user) + done() + + it "should look up the correct user in the database", -> + @User.findOne.calledWith(email: @email).should.equal true + + it "should check that the passwords match", -> + @bcrypt.compare + .calledWith(@unencryptedPassword, @hashedPassword) + .should.equal true + + it "should check the number of rounds", -> + @bcrypt.getRounds.called.should.equal true + + it "should set the users password (with a higher number of rounds)", -> + @AuthenticationManager.setUserPassword + .calledWith("user-id", @unencryptedPassword) + .should.equal true + + it "should return the user", -> + @callback.calledWith(null, @user).should.equal true + describe "when the user does not exist in the database", -> beforeEach -> @User.findOne = sinon.stub().callsArgWith(1, null, null) @@ -87,7 +118,7 @@ describe "AuthenticationManager", -> it "should hash the password", -> @bcrypt.genSalt - .calledWith(7) + .calledWith(12) .should.equal true @bcrypt.hash .calledWith(@password, @salt) From 0906bef5f9ba01b5d6914e59d8749b4019f51889 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 17 Jun 2016 13:50:32 +0100 Subject: [PATCH 2/2] change default bcrypt rounds to 12, to match default in AuthenticationManager --- services/web/config/settings.defaults.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/web/config/settings.defaults.coffee b/services/web/config/settings.defaults.coffee index 1950141518..80e4c9588e 100644 --- a/services/web/config/settings.defaults.coffee +++ b/services/web/config/settings.defaults.coffee @@ -137,7 +137,7 @@ module.exports = # -------- security: sessionSecret: sessionSecret - bcryptRounds: 14 + bcryptRounds: 12 # number of rounds used to hash user passwords (raised to power 2) httpAuthUsers: httpAuthUsers