From ef697a6fa7b7ed5cacafea2be3231d9de4151380 Mon Sep 17 00:00:00 2001 From: Andrew Wilcox Date: Mon, 2 Jun 2014 19:42:12 -0400 Subject: [PATCH] code review items --- packages/accounts-password/password_client.js | 12 +- packages/accounts-password/password_server.js | 94 +++---- packages/accounts-password/password_tests.js | 22 +- .../accounts-password/password_tests_setup.js | 8 + packages/srp/package.js | 7 - packages/srp/srp.js | 253 +----------------- packages/srp/srp_tests.js | 115 -------- 7 files changed, 73 insertions(+), 438 deletions(-) delete mode 100644 packages/srp/srp_tests.js diff --git a/packages/accounts-password/password_client.js b/packages/accounts-password/password_client.js index 4d87fbeb69..80c755698e 100644 --- a/packages/accounts-password/password_client.js +++ b/packages/accounts-password/password_client.js @@ -33,22 +33,22 @@ Meteor.loginWithPassword = function (selector, password, callback) { hashedPassword: SHA256(password), }], userCallback: function (error, result) { - if (error && error.error === 400 && error.reason === 'old password format') { + if (error && error.error === 400 && + error.reason === 'old password format') { var details; try { details = EJSON.parse(error.details); - } - catch (e) { - } + } catch (e) {} if (!(details && details.format === 'srp')) callback(new Error("unknown old password format")); else srpUpgradePath(selector, password, details.identity, callback); } - else if (error) + else if (error) { callback(error); - else + } else { callback(); + } } }); }; diff --git a/packages/accounts-password/password_server.js b/packages/accounts-password/password_server.js index 88981cfc5f..20ca0cfe20 100644 --- a/packages/accounts-password/password_server.js +++ b/packages/accounts-password/password_server.js @@ -7,20 +7,20 @@ var bcryptCompare = Meteor._wrapAsync(bcrypt.compare); // Salt the password that was hashed on the client for storage in the // database. // -var saltPassword = function (hashedPassword) { - return bcryptHash(hashedPassword, 10); +var hashPassword = function (password) { + return bcryptHash(password, 10 /* number of rounds */); }; // Check whether the provided hashed password matches the salted // password in the database user record. // -var checkPassword = function (user, hashedPassword) { +var checkPassword = function (user, password) { var result = { userId: user._id }; - if (! bcryptCompare(hashedPassword, user.services.password.bcrypt)) + if (! bcryptCompare(password, user.services.password.bcrypt)) result.error = new Meteor.Error(403, "Incorrect password"); return result; @@ -73,14 +73,27 @@ var userQueryValidator = Match.Where(function (user) { }); // Handler to login with a password. +// +// The Meteor client uses options.hashedPassword, the password hashed +// with SHA256. +// +// For other DDP clients which don't have access to SHA, the handler +// also accepts the plaintext password in options.plaintextPassword. +// +// (It might be nice if servers could turn the plaintext password +// option off. Or maybe it should be opt-in, not opt-out? +// Accounts.config option?) +// +// Note that neither password option is secure without SSL. +// Accounts.registerLoginHandler("password", function (options) { - if (!options.hashedPassword || options.srp) + if (!(options.hashedPassword || options.plaintextPassword) || options.srp) return undefined; // don't handle - check(options, { - user: userQueryValidator, - hashedPassword: String - }); + check(options, Match.OneOf( + {user: userQueryValidator, hashedPassword: String}, + {user: userQueryValidator, plaintextPassword: String} + )); var user = findUserFromUserQuery(options.user); @@ -96,22 +109,30 @@ Accounts.registerLoginHandler("password", function (options) { })); } - return checkPassword(user, options.hashedPassword); + return checkPassword( + user, + options.hashedPassword || SHA256(options.plaintextPassword) + ); }); // Handler to login using the SRP upgrade path. Accounts.registerLoginHandler("password", function (options) { - if (!options.srp || !options.hashedPassword) + if (!options.srp || !(options.hashedPassword || options.plaintextPassword)) return undefined; // don't handle - check(options, { - user: userQueryValidator, - srp: String, - hashedPassword: String - }); + check(options, Match.OneOf( + {user: userQueryValidator, srp: String, hashedPassword: String}, + {user: userQueryValidator, srp: String, plaintextPassword: String} + )); + var password = options.hashedPassword || SHA256(options.plaintextPassword); var user = findUserFromUserQuery(options.user); + // Check to see if another simultaneous login has already upgraded + // the user record to bcrypt. + if (user.services && user.services.password && user.services.password.bcrypt) + return checkPassword(user, password); + if (!(user.services && user.services.password && user.services.password.srp)) throw new Meteor.Error(403, "User has no password set"); @@ -130,7 +151,7 @@ Accounts.registerLoginHandler("password", function (options) { }; // Upgrade to bcrypt on successful login. - var salted = saltPassword(options.hashedPassword); + var salted = hashPassword(password); Meteor.users.update( user._id, { @@ -142,34 +163,6 @@ Accounts.registerLoginHandler("password", function (options) { return {userId: user._id}; }); -// Handler to login with plaintext password. -// -// The meteor client doesn't use this, it is for other DDP clients who -// haven't implemented hashing passwords. Since it sends the password -// in plaintext over the wire, it should only be run over SSL! -// -// XXX The above comment suggests regular logins without SSL *are* -// secure? -// -// Also, it might be nice if servers could turn this off. Or maybe it -// should be opt-in, not opt-out? Accounts.config option? -Accounts.registerLoginHandler("password", function (options) { - if (!options.password || !options.user) - return undefined; // don't handle - - check(options, {user: userQueryValidator, password: String}); - - var user = findUserFromUserQuery(options.user); - - if (!user.services || !user.services.password || !user.services.password.bcrypt) - return { - userId: user._id, - error: new Meteor.Error(403, "User has no password set") - }; - - return checkPassword(user, SHA256(options.password)) -}); - /// /// CHANGING @@ -195,7 +188,7 @@ Meteor.methods({changePassword: function (oldPassword, newPassword) { if (result.error) throw result.error; - var salted = saltPassword(newPassword); + var salted = hashPassword(newPassword); // It would be better if this removed ALL existing tokens and replaced // the token for the current connection with a new one, but that would @@ -217,7 +210,7 @@ Meteor.methods({changePassword: function (oldPassword, newPassword) { // Force change the users password. -Accounts.setPassword = function (userId, newPassword) { +Accounts.setPassword = function (userId, newPlaintextPassword) { var user = Meteor.users.findOne(userId); if (!user) throw new Meteor.Error(403, "User not found"); @@ -225,7 +218,7 @@ Accounts.setPassword = function (userId, newPassword) { Meteor.users.update( {_id: user._id}, { $unset: {'services.password.srp': 1}, - $set: {'services.password.bcrypt': saltPassword(SHA256(newPassword))} } + $set: {'services.password.bcrypt': hashPassword(SHA256(newPlaintextPassword))} } ); }; @@ -361,7 +354,7 @@ Meteor.methods({resetPassword: function (token, newPassword) { error: new Meteor.Error(403, "Token has invalid email address") }; - var salted = saltPassword(newPassword); + var salted = hashPassword(newPassword); // NOTE: We're about to invalidate tokens on the user, who we might be // logged in as. Make sure to avoid logging ourselves out if this @@ -529,7 +522,6 @@ var createUser = function (options) { username: Match.Optional(String), email: Match.Optional(String), password: Match.Optional(String), - srp: Match.Optional(SRP.matchVerifier), hashedPassword: Match.Optional(String) })); @@ -549,7 +541,7 @@ var createUser = function (options) { var user = {services: {}}; if (options.hashedPassword) { - var salted = saltPassword(options.hashedPassword); + var salted = hashPassword(options.hashedPassword); user.services.password = { bcrypt: salted }; } if (username) diff --git a/packages/accounts-password/password_tests.js b/packages/accounts-password/password_tests.js index c6618c756a..1dd33fdc3f 100644 --- a/packages/accounts-password/password_tests.js +++ b/packages/accounts-password/password_tests.js @@ -137,7 +137,7 @@ if (Meteor.isClient) (function () { function (test, expect) { Accounts.callLoginMethod({ // wrong password - methodArguments: [{user: {username: this.username}, password: 'wrong'}], + methodArguments: [{user: {username: this.username}, plaintextPassword: 'wrong'}], userCallback: expect(function (error) { test.isTrue(error); test.isFalse(Meteor.user()); @@ -147,7 +147,7 @@ if (Meteor.isClient) (function () { Accounts.callLoginMethod({ // right password methodArguments: [{user: {username: this.username}, - password: this.password}], + plaintextPassword: this.password}], userCallback: loggedInAs(this.username, test, expect) }); }, @@ -212,7 +212,7 @@ if (Meteor.isClient) (function () { self.secondConn = DDP.connect(Meteor.absoluteUrl()); self.secondConn.call('login', - { user: { username: self.username }, password: self.password }, + { user: { username: self.username }, plaintextPassword: self.password }, expect(function (err, result) { test.isFalse(err); self.secondConn.setUserId(result.id); @@ -742,17 +742,21 @@ if (Meteor.isClient) (function () { }, // We are able to login with the old style credentials in the database. function (test, expect) { - Meteor.loginWithPassword('srptestuser', 'abcdef', function (error) { - console.log('error', error); + Meteor.loginWithPassword('srptestuser', 'abcdef', expect(function (error) { test.isFalse(error); - }); + })); + }, + function (test, expect) { + Meteor.call("testSRPUpgrade", expect(function (error) { + test.isFalse(error); + })); }, logoutStep, // After the upgrade to bcrypt we're still able to login. function (test, expect) { - Meteor.loginWithPassword('srptestuser', 'abcdef', function (error) { + Meteor.loginWithPassword('srptestuser', 'abcdef', expect(function (error) { test.isFalse(error); - }); + })); }, logoutStep ]); @@ -846,7 +850,7 @@ if (Meteor.isServer) (function () { }); var result = clientConn.call('login', { user: {username: username}, - password: 'password' + plaintextPassword: 'password' }); test.isTrue(result); var token = Accounts._getAccountData(serverConn.id, 'loginToken'); diff --git a/packages/accounts-password/password_tests_setup.js b/packages/accounts-password/password_tests_setup.js index 0ecbb79fa5..d1418b81f6 100644 --- a/packages/accounts-password/password_tests_setup.js +++ b/packages/accounts-password/password_tests_setup.js @@ -131,5 +131,13 @@ Meteor.methods({ "verifier" : "2e8bce266b1357edf6952cc56d979db19f699ced97edfb2854b95972f820b0c7006c1a18e98aad40edf3fe111b87c52ef7dd06b320ce452d01376df2d560fdc4d8e74f7a97bca1f67b3cfaef34dee34dd6c76571c247d762624dc166dab5499da06bc9358528efa75bf74e2e7f5a80d09e60acf8856069ae5cfb080f2239ee76" } } } ); + }, + + testSRPUpgrade: function () { + var user = Meteor.users.findOne({username: 'srptestuser'}); + if (user.services && user.services.password && user.services.password.srp) + throw new Error("srp wasn't removed"); + if (!(user.services && user.services.password && user.services.password.bcrypt)) + throw new Error("bcrypt wasn't added"); } }); diff --git a/packages/srp/package.js b/packages/srp/package.js index e9388f918c..5f344b6204 100644 --- a/packages/srp/package.js +++ b/packages/srp/package.js @@ -10,10 +10,3 @@ Package.on_use(function (api) { api.add_files(['biginteger.js', 'srp.js'], ['client', 'server']); }); - -Package.on_test(function (api) { - api.use('tinytest'); - api.use('srp', ['client', 'server']); - api.use('underscore'); - api.add_files(['srp_tests.js'], ['client', 'server']); -}); diff --git a/packages/srp/srp.js b/packages/srp/srp.js index ec1724fcc4..4268691f7a 100644 --- a/packages/srp/srp.js +++ b/packages/srp/srp.js @@ -1,6 +1,7 @@ -SRP = {}; +// This package contains just enough of the original SRP code to +// support the backwards-compatibility upgrade path. -/////// PUBLIC CLIENT +SRP = {}; /** * Generate a new SRP verifier. Password is the plaintext password. @@ -44,249 +45,6 @@ SRP.matchVerifier = { }; -/** - * Generate a new SRP client object. Password is the plaintext password. - * - * options is optional and can include: - * - a: client's private ephemeral value. String or - * BigInteger. Normally, this is picked randomly, but it can be - * passed in for testing. - * - SRP parameters (see _defaults and paramsFromOptions below) - */ -SRP.Client = function (password, options) { - var self = this; - self.params = paramsFromOptions(options); - self.password = password; - - // shorthand - var N = self.params.N; - var g = self.params.g; - - // construct public and private keys. - var a, A; - if (options && options.a) { - if (typeof options.a === "string") - a = new BigInteger(options.a, 16); - else if (options.a instanceof BigInteger) - a = options.a; - else - throw new Error("Invalid parameter: a"); - - A = g.modPow(a, N); - - if (A.mod(N) === 0) - throw new Error("Invalid parameter: a: A mod N == 0."); - - } else { - while (!A || A.mod(N) === 0) { - a = randInt(); - A = g.modPow(a, N); - } - } - - self.a = a; - self.A = A; - self.Astr = A.toString(16); -}; - - -/** - * Initiate an SRP exchange. - * - * returns { A: 'client public ephemeral key. hex encoded integer.' } - */ -SRP.Client.prototype.startExchange = function () { - var self = this; - - return { - A: self.Astr - }; -}; - -/** - * Respond to the server's challenge with a proof of password. - * - * challenge is an object with - * - B: server public ephemeral key. hex encoded integer. - * - identity: user's identity (SRP username). - * - salt: user's salt. - * - * returns { M: 'client proof of password. hex encoded integer.' } - * throws an error if it got an invalid challenge. - */ -SRP.Client.prototype.respondToChallenge = function (challenge) { - var self = this; - - // shorthand - var N = self.params.N; - var g = self.params.g; - var k = self.params.k; - var H = self.params.hash; - - // XXX check for missing / bad parameters. - self.identity = challenge.identity; - self.salt = challenge.salt; - self.Bstr = challenge.B; - self.B = new BigInteger(self.Bstr, 16); - - if (self.B.mod(N) === 0) - throw new Error("Server sent invalid key: B mod N == 0."); - - var u = new BigInteger(H(self.Astr + self.Bstr), 16); - var x = new BigInteger( - H(self.salt + H(self.identity + ":" + self.password)), 16); - - var kgx = k.multiply(g.modPow(x, N)); - var aux = self.a.add(u.multiply(x)); - var S = self.B.subtract(kgx).modPow(aux, N); - var M = H(self.Astr + self.Bstr + S.toString(16)); - var HAMK = H(self.Astr + M + S.toString(16)); - - self.S = S; - self.HAMK = HAMK; - - return { - M: M - }; -}; - - -/** - * Verify server's confirmation message. - * - * confirmation is an object with - * - HAMK: server's proof of password. - * - * returns true or false. - */ -SRP.Client.prototype.verifyConfirmation = function (confirmation) { - var self = this; - - return (self.HAMK && (confirmation.HAMK === self.HAMK)); -}; - - - -/////// PUBLIC SERVER - - -/** - * Generate a new SRP server object. - * - * options is optional and can include: - * - b: server's private ephemeral value. String or - * BigInteger. Normally, this is picked randomly, but it can be - * passed in for testing. - * - SRP parameters (see _defaults and paramsFromOptions below) - */ -SRP.Server = function (verifier, options) { - var self = this; - self.params = paramsFromOptions(options); - self.verifier = verifier; - - // shorthand - var N = self.params.N; - var g = self.params.g; - var k = self.params.k; - var v = new BigInteger(self.verifier.verifier, 16); - - // construct public and private keys. - var b, B; - if (options && options.b) { - if (typeof options.b === "string") - b = new BigInteger(options.b, 16); - else if (options.b instanceof BigInteger) - b = options.b; - else - throw new Error("Invalid parameter: b"); - - B = k.multiply(v).add(g.modPow(b, N)).mod(N); - - if (B.mod(N) === 0) - throw new Error("Invalid parameter: b: B mod N == 0."); - - } else { - while (!B || B.mod(N) === 0) { - b = randInt(); - B = k.multiply(v).add(g.modPow(b, N)).mod(N); - } - } - - self.b = b; - self.B = B; - self.Bstr = B.toString(16); - -}; - - -/** - * Issue a challenge to the client. - * - * Takes a request from the client containing: - * - A: hex encoded int. - * - * Returns a challenge with: - * - B: server public ephemeral key. hex encoded integer. - * - identity: user's identity (SRP username). - * - salt: user's salt. - * - * Throws an error if issued a bad request. - */ -SRP.Server.prototype.issueChallenge = function (request) { - var self = this; - - // XXX check for missing / bad parameters. - self.Astr = request.A; - self.A = new BigInteger(self.Astr, 16); - - if (self.A.mod(self.params.N) === 0) - throw new Error("Client sent invalid key: A mod N == 0."); - - // shorthand - var N = self.params.N; - var H = self.params.hash; - - // Compute M and HAMK in advance. Don't send to client yet. - var u = new BigInteger(H(self.Astr + self.Bstr), 16); - var v = new BigInteger(self.verifier.verifier, 16); - var avu = self.A.multiply(v.modPow(u, N)); - self.S = avu.modPow(self.b, N); - self.M = H(self.Astr + self.Bstr + self.S.toString(16)); - self.HAMK = H(self.Astr + self.M + self.S.toString(16)); - - return { - identity: self.verifier.identity, - salt: self.verifier.salt, - B: self.Bstr - }; -}; - - -/** - * Verify a response from the client and return confirmation. - * - * Takes a challenge response from the client containing: - * - M: client proof of password. hex encoded int. - * - * Returns a confirmation if the client's proof is good: - * - HAMK: server proof of password. hex encoded integer. - * OR null if the client's proof doesn't match. - */ -SRP.Server.prototype.verifyResponse = function (response) { - var self = this; - - if (response.M !== self.M) - return null; - - return { - HAMK: self.HAMK - }; -}; - - - -/////// INTERNAL - /** * Default parameter values for SRP. * @@ -337,8 +95,3 @@ var paramsFromOptions = function (options) { return ret; }; - - -var randInt = function () { - return new BigInteger(Random.hexString(36), 16); -}; diff --git a/packages/srp/srp_tests.js b/packages/srp/srp_tests.js deleted file mode 100644 index d1ea3edc35..0000000000 --- a/packages/srp/srp_tests.js +++ /dev/null @@ -1,115 +0,0 @@ -Tinytest.add("srp - good exchange", function(test) { - var password = 'hi there!'; - var verifier = SRP.generateVerifier(password); - - var C = new SRP.Client(password); - var S = new SRP.Server(verifier); - - var request = C.startExchange(); - var challenge = S.issueChallenge(request); - var response = C.respondToChallenge(challenge); - var confirmation = S.verifyResponse(response); - - test.isTrue(confirmation); - test.isTrue(C.verifyConfirmation(confirmation)); - -}); - -Tinytest.add("srp - bad exchange", function(test) { - var verifier = SRP.generateVerifier('one password'); - - var C = new SRP.Client('another password'); - var S = new SRP.Server(verifier); - - var request = C.startExchange(); - var challenge = S.issueChallenge(request); - var response = C.respondToChallenge(challenge); - var confirmation = S.verifyResponse(response); - - test.isFalse(confirmation); -}); - - -Tinytest.add("srp - fixed values", function(test) { - // Test exact values during the exchange. We have to be very careful - // about changing the SRP code, because changes could render - // people's existing user database unusable. This test is - // intentionally brittle to catch change that could affect the - // validity of user passwords. - - var identity = "b73d9af9-4e74-4ce0-879c-484828b08436"; - var salt = "85f8b9d3-744a-487d-8982-a50e4c9f552a"; - var password = "95109251-3d8a-4777-bdec-44ffe8d86dfb"; - var a = "dc99c646fa4cb7c24314bb6f4ca2d391297acd0dacb0430a13bbf1e37dcf8071"; - var b = "cf878e00c9f2b6aa48a10f66df9706e64fef2ca399f396d65f5b0a27cb8ae237"; - - var verifier = SRP.generateVerifier( - password, {identity: identity, salt: salt}); - - var C = new SRP.Client(password, {a: a}); - var S = new SRP.Server(verifier, {b: b}); - - var request = C.startExchange(); - test.equal(request.A, "8a75aa61471a92d4c3b5d53698c910af5ef013c42799876c40612d1d5e0dc41d01f669bc022fadcd8a704030483401a1b86b8670191bd9dfb1fb506dd11c688b2f08e9946756263954db2040c1df1894af7af5f839c9215bb445268439157e65e8f100469d575d5d0458e19e8bd4dd4ea2c0b30b1b3f4f39264de4ec596e0bb7"); - - var challenge = S.issueChallenge(request); - test.equal(challenge.B, "77ab0a40ef428aa2fa2bc257c905f352c7f75fbcfdb8761393c9dc0f730bbb0270ba9f837545b410c955c3f761494b329ad23c6efdec7e63509e538c2f68a3526e072550a11dac46017718362205e0c698b5bed67d6ff475aa92c191ca169f865c81a1a577373c449b98df720c7b7ff50536f9919d781e698025fd7164932ba7"); - - var response = C.respondToChallenge(challenge); - test.equal(response.M, "8705d31bb61497279adf44eef6c167dcb7e03aa7a42102c1ea7e73025fbd4cd9"); - - var confirmation = S.verifyResponse(response); - test.equal(confirmation.HAMK, "07a0f200392fa9a084db7acc2021fbc174bfb36956b46835cc12506b68b27bba"); - - test.isTrue(C.verifyConfirmation(confirmation)); -}); - - -Tinytest.add("srp - options", function(test) { - // test that all options are respected. - // - // Note, all test strings here should be hex, because the 'hash' - // function needs to output numbers. - - var baseOptions = { - hash: function (x) { return x; }, - N: 'b', - g: '2', - k: '1' - }; - var verifierOptions = _.extend({ - identity: 'a', - salt: 'b' - }, baseOptions); - var clientOptions = _.extend({ - a: "2" - }, baseOptions); - var serverOptions = _.extend({ - b: "2" - }, baseOptions); - - var verifier = SRP.generateVerifier('c', verifierOptions);; - - test.equal(verifier.identity, 'a'); - test.equal(verifier.salt, 'b'); - test.equal(verifier.verifier, '3'); - - var C = new SRP.Client('c', clientOptions); - var S = new SRP.Server(verifier, serverOptions); - - var request = C.startExchange(); - test.equal(request.A, '4'); - - var challenge = S.issueChallenge(request); - test.equal(challenge.identity, 'a'); - test.equal(challenge.salt, 'b'); - test.equal(challenge.B, '7'); - - var response = C.respondToChallenge(challenge); - test.equal(response.M, '471'); - - var confirmation = S.verifyResponse(response); - test.isTrue(confirmation); - test.equal(confirmation.HAMK, '44711'); - -});