code review items

This commit is contained in:
Andrew Wilcox
2014-06-02 19:42:12 -04:00
committed by Emily Stark
parent 644dde0382
commit ef697a6fa7
7 changed files with 73 additions and 438 deletions

View File

@@ -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();
}
}
});
};

View File

@@ -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)

View File

@@ -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');

View File

@@ -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");
}
});

View File

@@ -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']);
});

View File

@@ -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);
};

View File

@@ -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');
});