Skip to content
This repository has been archived by the owner on May 10, 2019. It is now read-only.

Commit

Permalink
Bug 793579: check issuer for primary-address operations.
Browse files Browse the repository at this point in the history
When "primary" addresses (which come from browserid-aware IdPs) are
associated with a Persona account, users can perform certain
operations (including add_email) with a primary-backed assertion. This
changes primary.verifyAssertion(), which checks such assertions, to make
sure their .issuer field is actually allowed to speak for the address in
question.

delegatesAuthority() was taught about proxyIDPs and g_shim_cache. This
shouldn't affect normal operations (which never have g_shim_cache), but
allows proxy-idp-test.js to pass. A pre-existing bug (which ran the
callback multiple times) was fixed too, to keep bigtent from breaking.

A unit test was added, exercising /wsapi/add_email_with_assertion. This
works by creating a cert (signed by one domain), and certifying a
principal at an unrelated domain. Anyone who accepts certs should reject
this.
  • Loading branch information
warner committed Sep 25, 2012
1 parent 6fcb9c3 commit a04db25
Show file tree
Hide file tree
Showing 2 changed files with 98 additions and 4 deletions.
36 changes: 32 additions & 4 deletions lib/primary.js
Expand Up @@ -266,13 +266,23 @@ exports.getPublicKey = function(domain, cb) {

// Does emailDomain actual delegate to the issuingDomain?
exports.delegatesAuthority = function (emailDomain, issuingDomain, cb) {
if (config.has('proxy_idps')) {
var proxyIDPs = config.get('proxy_idps');
if (proxyIDPs.hasOwnProperty(emailDomain))
if (g_shim_cache.hasOwnProperty(proxyIDPs[emailDomain])) {
var url = g_shim_cache[proxyIDPs[emailDomain]].origin + "/";
if (url.indexOf('://' + issuingDomain + ':') !== -1)
return cb(true);
}
}

exports.checkSupport(emailDomain, function(err, urls, publicKey) {
// Check http or https://{issuingDomain}/some/sign_in_path
if (! err && urls && urls.auth &&
urls.auth.indexOf('://' + issuingDomain + '/') !== -1) {
cb(true);
return cb(true);
}
cb(false);
return cb(false);
});
}

Expand All @@ -282,9 +292,11 @@ exports.verifyAssertion = function(assertion, cb) {
return process.nextTick(function() { cb("primary support disabled") });
}

var rootIssuer;
var getRoot = function(issuer, next) {
// allow assertions rooted in certs issued by us. this occurs in the proxy_idp case
// where we sign assertions for other domains.
rootIssuer = issuer; // remember for policy check later
if (issuer === HOSTNAME) {
next(null, PUBLIC_KEY);
} else {
Expand Down Expand Up @@ -312,7 +324,23 @@ exports.verifyAssertion = function(assertion, cb) {
return cb("can't log in with an assertion for '" + got.toString() + "'");
}

// all is well, get the principal from the last cert
cb(null, certParamsArray[certParamsArray.length-1].certParams.principal.email);
// principal is in the last cert
var principal = certParamsArray[certParamsArray.length - 1].certParams.principal;
var domainFromEmail = principal.email.replace(/^.*@/, '');

if (rootIssuer !== domainFromEmail)
{
exports.delegatesAuthority(domainFromEmail, rootIssuer, function (delegated) {
if (delegated) {
cb(null, principal.email);
} else {
return cb("issuer '" + rootIssuer + "' may not speak for emails from '"
+ domainFromEmail + "'");
}
});
} else {
// all is well, get the principal from the last cert
cb(null, principal.email);
}
});
};
66 changes: 66 additions & 0 deletions tests/add-email-with-assertion-test.js
Expand Up @@ -163,6 +163,72 @@ suite.addBatch({
}
});


// now create a lame cert: valid signature by the wrong party
const OTHER_EMAIL = 'otheruser@other.domain'; // *not* TEST_DOMAIN
var bad_cert, bad_assertion;

suite.addBatch({
"generating a lame certificate": {
topic: function() {
var expiration = new Date();
expiration.setTime(new Date().valueOf() + 60 * 60 * 1000);
jwcrypto.cert.sign({publicKey: g_keypair.publicKey,
principal: {email: OTHER_EMAIL}},
{issuer: TEST_DOMAIN,
expiresAt: expiration, issuedAt: new Date()},
null, g_privKey, this.callback);
},
"bad cert created": function(err, cert) {
assert.isString(cert);
assert.lengthOf(cert.split('.'), 3);
bad_cert = cert;
}
}
});

// generate an assertion using the lame cert
suite.addBatch({
"generating an assertion": {
topic: function() {
var self = this;
var expirationDate = new Date(new Date().getTime() + (2 * 60 * 1000));
jwcrypto.assertion.sign(
{},
{audience: TEST_ORIGIN,
issuer: TEST_DOMAIN, // huh, assertions don't have .iss, right?
expiresAt: expirationDate},
g_keypair.secretKey,
function(err, signedAssertion) {
self.callback(err, jwcrypto.cert.bundle([bad_cert], signedAssertion));
});
},
"succeeds": function(err, r) {
assert.isString(r);
bad_assertion = r;
}
}
});

suite.addBatch({
"adding this email via bad assertion": {
topic: function() {
wsapi.post('/wsapi/add_email_with_assertion', {
assertion: bad_assertion
}).call(this);
},
"fails due to bad issuer": function(err, r) {
assert.strictEqual(r.code, 200);
var respObj = JSON.parse(r.body);
assert.strictEqual(respObj.success, false);
assert.strictEqual(respObj.reason, "issuer 'example.domain' may not speak for emails from 'other.domain'");
}
}
});

// since the lame cert was rejected, we should only have the two original
// addresses

suite.addBatch({
"list emails": {
topic: wsapi.get('/wsapi/list_emails', {}),
Expand Down

0 comments on commit a04db25

Please sign in to comment.