From 8316bb978fd95c3fb571736628551dda0a251e7e Mon Sep 17 00:00:00 2001
From: Lloyd Hilaiel <lloyd@hilaiel.com>
Date: Tue, 1 May 2012 18:58:14 -0600
Subject: [PATCH] require authentication on complete_* wsapis to reduce risk
now that password is provided before email verification. issue #290 and a
pre-requisite for issue #1000
---
lib/db.js | 17 +++---
lib/db/json.js | 19 +++++--
lib/db/mysql.js | 50 ++++++++---------
lib/httputils.js | 4 ++
lib/wsapi/complete_email_addition.js | 51 ++++++++++++-----
lib/wsapi/complete_user_creation.js | 82 +++++++++++++++++++++-------
lib/wsapi/email_for_token.js | 19 ++++++-
lib/wsapi/stage_user.js | 6 +-
tests/db-test.js | 4 +-
tests/stalled-mysql-test.js | 3 +-
10 files changed, 173 insertions(+), 82 deletions(-)
diff --git a/lib/db.js b/lib/db.js
index 6765c28fb..1f08f6e99 100644
--- a/lib/db.js
+++ b/lib/db.js
@@ -73,20 +73,21 @@ exports.onReady = function(f) {
// these are read only database calls
[
+ 'authForVerificationSecret',
+ 'checkAuth',
+ 'emailForVerificationSecret',
'emailKnown',
- 'userKnown',
- 'isStaged',
+ 'emailToUID',
+ 'emailType',
'emailsBelongToSameAccount',
- 'emailForVerificationSecret',
'haveVerificationSecret',
- 'verificationSecretForEmail',
- 'checkAuth',
- 'listEmails',
+ 'isStaged',
'lastStaged',
+ 'listEmails',
'ping',
- 'emailType',
+ 'userKnown',
'userOwnsEmail',
- 'emailToUID'
+ 'verificationSecretForEmail'
].forEach(function(fn) {
exports[fn] = function() {
checkReady();
diff --git a/lib/db/json.js b/lib/db/json.js
index 50bac38e2..e49615c1f 100644
--- a/lib/db/json.js
+++ b/lib/db/json.js
@@ -236,15 +236,26 @@ exports.emailForVerificationSecret = function(secret, cb) {
process.nextTick(function() {
sync();
if (!db.staged[secret]) return cb("no such secret");
+ cb(null, db.staged[secret].email, db.staged[secret].existing_user);
+ });
+};
+
+exports.authForVerificationSecret = function(secret, cb) {
+ process.nextTick(function() {
+ sync();
+ if (!db.staged[secret]) return cb("no such secret");
+
+ if (db.staged[secret].passwd) {
+ return cb(null, db.staged[secret].passwd, db.staged[secret].existing_user);
+ }
+
exports.checkAuth(db.staged[secret].existing_user, function (err, hash) {
- cb(err, {
- email: db.staged[secret].email,
- needs_password: !hash
- });
+ cb(err, hash, db.staged[secret].existing_user);
});
});
};
+
exports.verificationSecretForEmail = function(email, cb) {
setTimeout(function() {
sync();
diff --git a/lib/db/mysql.js b/lib/db/mysql.js
index 0af86c661..5e5dcf98e 100644
--- a/lib/db/mysql.js
+++ b/lib/db/mysql.js
@@ -265,7 +265,20 @@ exports.haveVerificationSecret = function(secret, cb) {
exports.emailForVerificationSecret = function(secret, cb) {
client.query(
- "SELECT * FROM staged WHERE secret = ?", [ secret ],
+ "SELECT email, existing_user FROM staged WHERE secret = ?", [ secret ],
+ function(err, rows) {
+ if (err) return cb("database unavailable");
+
+ // if the record was not found, fail out
+ if (!rows || rows.length != 1) return cb("no such secret");
+
+ cb(null, rows[0].email, rows[0].existing_user);
+ });
+};
+
+exports.authForVerificationSecret = function(secret, cb) {
+ client.query(
+ "SELECT existing_user, passwd FROM staged WHERE secret = ?", [ secret ],
function(err, rows) {
if (err) return cb("database unavailable");
@@ -274,34 +287,15 @@ exports.emailForVerificationSecret = function(secret, cb) {
var o = rows[0];
- // if the record was found and this is for a new_acct, return the email
- if (o.new_acct) return cb(null, { email: o.email, needs_password: false });
-
- // we need a userid. the old schema had an 'existing' field which was an email
- // address. the new schema has an 'existing_user' field which is a userid.
- // this is transitional code so outstanding verification links continue working
- // and can be removed in feb 2012 some time. maybe for valentines day?
- if (typeof o.existing_user === 'number') doCheckAuth(o.existing_user);
- else if (typeof o.existing === 'string') {
- exports.emailToUID(o.existing, function(err, uid) {
- if (err || uid === undefined) return cb('acct associated with staged email doesn\'t exist');
- doCheckAuth(uid);
- });
- }
+ // if there is a hashed passwd in the result, we're done
+ if (o.passwd) return cb(null, o.passwd, o.existing_user);
- function doCheckAuth(uid) {
- // if the account is being added to an existing account, let's find
- // out if the account has a password set (if only primary email addresses
- // are associated with the acct at the moment, then there will not be a
- // password set and the user will need to set one with the addition of
- // this addresss)
- exports.checkAuth(uid, function(err, hash) {
- cb(err, {
- email: o.email,
- needs_password: !hash
- });
- });
- }
+ // otherwise, let's get the passwd from the user record
+ if (!o.existing_user) cb("no password for user");
+
+ exports.checkAuth(o.existing_user, function(err, hash) {
+ cb(err, hash, o.existing_user);
+ });
});
};
diff --git a/lib/httputils.js b/lib/httputils.js
index 81e68334d..cbc7a8c65 100644
--- a/lib/httputils.js
+++ b/lib/httputils.js
@@ -28,6 +28,10 @@ exports.serviceUnavailable = function(resp, reason) {
sendResponse(resp, "Service Unavailable", reason, 503);
};
+exports.authRequired = function(resp, reason) {
+ sendResponse(resp, "Authentication Required", reason, 401);
+};
+
exports.badRequest = function(resp, reason) {
sendResponse(resp, "Bad Request", reason, 400);
};
diff --git a/lib/wsapi/complete_email_addition.js b/lib/wsapi/complete_email_addition.js
index 0b705abcb..fcff72813 100644
--- a/lib/wsapi/complete_email_addition.js
+++ b/lib/wsapi/complete_email_addition.js
@@ -5,11 +5,11 @@
const
db = require('../db.js'),
logger = require('../logging.js').logger,
-wsapi = require('../wsapi.js');
+wsapi = require('../wsapi.js'),
+brycpt = require('../bcrypt.js');
exports.method = 'post';
exports.writes_db = true;
-// XXX: see issue #290 - we want to require authentication here and update frontend code
exports.authed = false;
// NOTE: this API also takes a 'pass' parameter which is required
// when a user has a null password (only primaries on their acct)
@@ -17,19 +17,44 @@ exports.args = ['token'];
exports.i18n = false;
exports.process = function(req, res) {
- db.emailForVerificationSecret(req.body.token, function(err, r) {
- if (err === 'database unavailable') {
+ // in order to complete an email addition, one of the following must be true:
+ //
+ // 1. you must already be authenticated as the user who initiated the verification
+ // 2. you must provide the password of the initiator.
+ //
+ db.authForVerificationSecret(req.body.token, function(err, initiator_hash, initiator_uid) {
+ if (err) {
+ logger.info("unknown verification secret: " + err);
return wsapi.databaseDown(res, err);
}
- db.gotVerificationSecret(req.body.token, function(e, email, uid) {
- if (e) {
- logger.warn("couldn't complete email verification: " + e);
- wsapi.databaseDown(res, e);
- } else {
- wsapi.authenticateSession(req.session, uid, 'password');
- res.json({ success: true });
- }
- });
+ if (req.session.userid === initiator_uid) {
+ postAuthentication();
+ } else if (typeof req.body.pass === 'string') {
+ bcrypt.compare(req.body.pass, initiator_hash, function (err, success) {
+ if (err) {
+ logger.warn("max load hit, failing on auth request with 503: " + err);
+ return httputils.serviceUnavailable(res, "server is too busy");
+ } else if (!success) {
+ return httputils.authRequired(res, "password mismatch");
+ } else {
+ postAuthentication();
+ }
+ });
+ } else {
+ return httputils.authRequired(res, "password required");
+ }
+
+ function postAuthentication() {
+ db.gotVerificationSecret(req.body.token, function(e, email, uid) {
+ if (e) {
+ logger.warn("couldn't complete email verification: " + e);
+ wsapi.databaseDown(res, e);
+ } else {
+ wsapi.authenticateSession(req.session, uid, 'password');
+ res.json({ success: true });
+ }
+ });
+ };
});
};
diff --git a/lib/wsapi/complete_user_creation.js b/lib/wsapi/complete_user_creation.js
index 720dd3282..e507e0f95 100644
--- a/lib/wsapi/complete_user_creation.js
+++ b/lib/wsapi/complete_user_creation.js
@@ -6,7 +6,8 @@ const
db = require('../db.js'),
wsapi = require('../wsapi.js'),
httputils = require('../httputils'),
-logger = require('../logging.js').logger;
+logger = require('../logging.js').logger,
+bcrypt = require('../bcrypt');
exports.method = 'post';
exports.writes_db = true;
@@ -15,27 +16,68 @@ exports.args = ['token'];
exports.i18n = false;
exports.process = function(req, res) {
- // at the time the email verification is performed, we'll clear the pendingCreation
- // data on the session.
- delete req.session.pendingCreation;
+ // in order to complete a user creation, one of the following must be true:
+ //
+ // 1. you are using the same browser to complete the email verification as you
+ // used to start it
+ // 2. you have provided the password chosen by the initiator of the verification
+ // request
+ //
+ // These protections guard against the case where an attacker can send out a bunch
+ // of verification emails, wait until a distracted internet user clicks on one,
+ // and then control a browserid account that they can use to prove they own
+ // the email address of the attacked.
- db.haveVerificationSecret(req.body.token, function(err, known) {
- if (err) return wsapi.databaseDown(res, err);
-
- if (!known) return res.json({ success: false} );
-
- db.gotVerificationSecret(req.body.token, function(err, email, uid) {
+ // is this the same browser?
+ if (typeof req.session.pendingCreation === 'string' &&
+ req.body.token === req.session.pendingCreation) {
+ postAuthentication();
+ }
+ // is a password provided?
+ else if (typeof req.body.pass === 'string') {
+ return db.authForVerificationSecret(req.body.token, function(err, hash) {
if (err) {
- logger.warn("couldn't complete email verification: " + err);
- wsapi.databaseDown(res, err);
- } else {
- // FIXME: not sure if we want to do this (ba)
- // at this point the user has set a password associated with an email address
- // that they've verified. We create an authenticated session.
- wsapi.authenticateSession(req.session, uid, 'password',
- config.get('ephemeral_session_duration_ms'));
- res.json({ success: true });
+ logger.warn("couldn't get password for verification secret: " + err);
+ return wsapi.databaseDown(res, err);
}
+ bcrypt.compare(req.body.pass, hash, function (err, success) {
+ if (err) {
+ logger.warn("max load hit, failing on auth request with 503: " + err);
+ return httputils.serviceUnavailable(res, "server is too busy");
+ } else if (!success) {
+ return httputils.authRequired(res, "password mismatch");
+ } else {
+ postAuthentication();
+ }
+ });
+ });
+ } else {
+ return httputils.authRequired(res, 'Provide your password');
+ }
+
+ function postAuthentication() {
+ // the time the email verification is performed, we'll clear the pendingCreation
+ // data on the session.
+ delete req.session.pendingCreation;
+
+ db.haveVerificationSecret(req.body.token, function(err, known) {
+ if (err) return wsapi.databaseDown(res, err);
+
+ if (!known) return res.json({ success: false} );
+
+ db.gotVerificationSecret(req.body.token, function(err, email, uid) {
+ if (err) {
+ logger.warn("couldn't complete email verification: " + err);
+ wsapi.databaseDown(res, err);
+ } else {
+ // FIXME: not sure if we want to do this (ba)
+ // at this point the user has set a password associated with an email address
+ // that they've verified. We create an authenticated session.
+ wsapi.authenticateSession(req.session, uid, 'password',
+ config.get('ephemeral_session_duration_ms'));
+ res.json({ success: true });
+ }
+ });
});
- });
+ }
};
diff --git a/lib/wsapi/email_for_token.js b/lib/wsapi/email_for_token.js
index bfb122a74..f492bcff5 100644
--- a/lib/wsapi/email_for_token.js
+++ b/lib/wsapi/email_for_token.js
@@ -19,7 +19,7 @@ exports.args = ['token'];
exports.i18n = false;
exports.process = function(req, res) {
- db.emailForVerificationSecret(req.query.token, function(err, r) {
+ db.emailForVerificationSecret(req.query.token, function(err, email, uid) {
if (err) {
if (err === 'database unavailable') {
httputils.serviceUnavailable(res, err);
@@ -30,10 +30,23 @@ exports.process = function(req, res) {
});
}
} else {
+ // must the user authenticate? This is true if they are not authenticated
+ // as the uid who initiated the verification, and they are not on the same
+ // browser as the initiator
+ var must_auth = true;
+
+ if (uid && req.session.userid === uid) {
+ must_auth = false;
+ }
+ else if (!uid && typeof req.session.pendingCreation === 'string' &&
+ req.query.token === req.session.pendingCreation) {
+ must_auth = false;
+ }
+
res.json({
success: true,
- email: r.email,
- needs_password: r.needs_password
+ email: email,
+ must_auth: must_auth
});
}
});
diff --git a/lib/wsapi/stage_user.js b/lib/wsapi/stage_user.js
index 6d40868a6..b7f36ee8c 100644
--- a/lib/wsapi/stage_user.js
+++ b/lib/wsapi/stage_user.js
@@ -25,9 +25,6 @@ exports.i18n = true;
exports.process = function(req, resp) {
var langContext = wsapi.langContext(req);
- // staging a user logs you out.
- wsapi.clearAuthenticatedUser(req.session);
-
// validate
try {
sanitize(req.body.email).isEmail();
@@ -54,6 +51,9 @@ exports.process = function(req, resp) {
return httputils.throttled(resp, "Too many emails sent to that address, try again later.");
}
+ // staging a user logs you out.
+ wsapi.clearAuthenticatedUser(req.session);
+
// now bcrypt the password
wsapi.bcryptPassword(req.body.pass, function (err, hash) {
if (err) {
diff --git a/tests/db-test.js b/tests/db-test.js
index e43237858..6c8ec1ea5 100755
--- a/tests/db-test.js
+++ b/tests/db-test.js
@@ -85,8 +85,8 @@ suite.addBatch({
topic: function(err, secret) {
db.emailForVerificationSecret(secret, this.callback);
},
- "matches expected email": function(err, r) {
- assert.strictEqual(r.email, 'lloyd@nowhe.re');
+ "matches expected email": function(err, email, uid) {
+ assert.strictEqual(email, 'lloyd@nowhe.re');
}
},
"fetch secret for email": {
diff --git a/tests/stalled-mysql-test.js b/tests/stalled-mysql-test.js
index 864c87225..e75fbad67 100755
--- a/tests/stalled-mysql-test.js
+++ b/tests/stalled-mysql-test.js
@@ -129,7 +129,8 @@ suite.addBatch({
},
"complete_user_creation": {
topic: wsapi.post('/wsapi/complete_user_creation', {
- token: 'bogus'
+ token: 'bogus',
+ pass: 'alsobogus'
}),
"fails with 503": function(err, r) {
assert.strictEqual(r.code, 503);
--
GitLab