Closed Bug 793579 Opened 12 years ago Closed 12 years ago

primary.verifyAssertion doesn't check whether issuer matches email

Categories

(Cloud Services :: Server: Identity, defect)

Product:
Cloud Services
Component:
Server: Identity
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: rfkelly, Assigned: warner)

Details

Attachments

(2 files, 2 obsolete files)

fix validation of .issuser
6.52 KB, patch
Details | Diff | Splinter Review
fix primate.verifyAssertion, leave verifier broken, less invasive
4.72 KB, text/plain
Details 
In browserid/lib/primary.js, the "verifyAssertion" function does not check whether the certificate issuer matches the domain of the asserted email address.  It will therefore accept bogus assertions, such as an assertion for "rfkelly@mozilla.com" that is signed by the mockmyid.com private key.

It should use the same logic as lib/verifier/certassertion.js for checking that the issuer matches the domain.

Exploiting this bug, I was able to gain unauthorized access to my own account on login.persona.org at the "assertion" authorization level.  This let me remove emails, add emails and reset the account password.  It did not let me generate assertions for existing emails attached to the account.
Ryan, how do you mean 'reset the account password'. Change it?
(In reply to John Morrison [:jrgm] from comment #1)
> Ryan, how do you mean 'reset the account password'. Change it?

I was able to add a new email address, then go through the "forgot password" flow to reset the password to something of my choosing, with the confirmation email sent to the new address.

Fortunately, the password reset process has a bunch of logic to protect the integrity of the account - it invalidates any existing sessions and marks all other emails linked to the account as unverified.  So even after resetting the account password to something of my choosing, I was not able to fraudulently generate assertions for the addresses associated with the account.

Another limitation: this only works against persona.org accounts that have a primary-supported email on file.  I was able to fraudulently access my account because it has "rfkelly@eyedee.me" associated with it.  But this limitation seems to be almost accidental, due to unimplemented logic here:  https://github.com/mozilla/browserid/blob/dev/lib/wsapi/auth_with_assertion.js#L60

So ISTM that the scope of this bug is limited to doing a bit of account vandalism on a small number of accounts.  Which isn't great, but is a lot better than it could have been.
thanks Ryan, this is super helpful.

jrgm: I think this is important but does not require a hotfix this week. Let's loop in Brian Warner, he can prepare a private patch. Oh, Brian, it looks like you're already cc'ed?
(In reply to Ryan Kelly [:rfkelly] from comment #2)
> 
> So ISTM that the scope of this bug is limited to doing a bit of account
> vandalism on a small number of accounts.  Which isn't great, but is a lot
> better than it could have been.

I take it back.  This can be escalated into generating fraudulent assertions, via a different attack channel.

  1) create a normal everyday persona.org account using a legitimate address
  2) call the "add_email_with_assertion" WSAPI endpoint using a bogus assertion, triggering
     this bug to fraudulently add an email  I don't own onto my account
  3) call the "cert_key" WSAPI endpoing to generate an identity certificate for this fraudulent address.  Persona.org will comply, despite the fact that the address is marked as primary in the database.
  4) use the identity certificate to generate an assertion for the fraudlent address.  The hosted verifier will accept this secondary-backed assertion, even for addresses that have primary browserid support.
Attaching a python script that can generate fraudulent assertions by exploiting this bug.
Ok, so it sounds like three distinct bugs:

1: lib/primary.js verifyAssertion(), or a subfunction, isn't checking the
   first certificate's .iss (issuer) field for compliance with the intended
   policy, which is:
 * convert the principal.email into a domain A, fetch A/.well-known/browserid
 * if that 404's, accept .iss == SECONDARY
 * if that returns a delegation record, accept .iss == DELEGATED-TO
 * else require .iss = A

2: the /wsapi/cert_key API should refuse to generate certs for addresses
   marked "primary" in the database, because these addresses were established
   via a primary-backed assertion (and a fallback-backed assertion is
   insufficient), therefore these addresses fall under the control of a
   primary IdP, and we should get out of the way. No verifier should accept
   fallback-backed assertions for them, so the fallback should not create
   them.

3: the hosted verifier should not accept fallback-backed assertions for
   domains that have published themselves as primaries

The first bug enables attacks by anybody who runs an IdP (on any domain),
plus anyone who uses a service like mockmyid.com. The second bug is a
nuisance, but wouldn't be a problem except for the third bug. The third bug
gives persona.org too much power.
(In reply to Brian Warner [:warner :bwarner] from comment #6)
> 
> The first bug enables attacks by anybody who runs an IdP (on any domain),
> plus anyone who uses a service like mockmyid.com.

In the interests of clarity: the first bug enables attacks by anybody, fullstop.

Anyone anywhere can use mockmyid.com's private key to claim ownership of any email address of their choosing.  For example, the 'add_email_with_assertion' call would happily reassign bwarner@mozilla.com from your account to mine if I provided it with an appropriate bogus assertion.
Sorry, yeah, didn't mean to downplay the severity. This is a complete break.

I've added a first-draft patch to the private repo in the "793579-issuer" branch (patch against train-2012.08.17 attached). This
[oops, submitted too early]

Sorry, yeah, didn't mean to downplay the severity. This is a complete break.

I've added a first-draft patch to the private repo in the "793579-issuer"
branch (patch against train-2012.08.17 attached). This changes both the
persona.org account-manager and the hosted verifier to tighten up .issuer
handling, which in theory should fix both the 1st and 3rd bugs. In addition
to requiring a proper delegation (from the domain of the principal.email to
the .issuer), it also stops accepting secondary-backed certs when the domain
in question is running a primary IdP.

The patch isn't complete: it doesn't have any new tests yet, and needs at
least one for both of those situations. It also fails at least one existing
test: proxy-idp-test.js. I *think* that's because my new code isn't handling
SHIMMED_PRIMARIES, in which case it's a shallow fix. It might be deeper than
that.

The new verifier code also performs an HTTPS .well-known fetch every time it
checks an assertion, whereas the previous code would only fetch the
.well-known file if the .issuer was neither the secondary's domain name nor
the principal.email's domain name. The fetch is absolutely necessary to avoid
the excess authority (if the verifier doesn't check for a primary IdP, it
can't correctly reject a secondary-backed assertion), but this will be a new
performance hit. We should be allowed to cache the result for a while, which
will help, but we need to finish the discussion in
https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.identity/ArXmGF3OnW4
about transitions between primary and non-primary status to nail down those
caching rules exactly.
preliminary patch (no tests, causes existing tests to fail).
Assignee: nobody → warner-bugzilla
At rfkelly's suggestion, this fix only addresses the first bug, leaving the verifier broken (ignoring primary IdPs and accepting secondary-backed assertions anyways). It's less invasive and somewhat less risky.

proxy-idp-test.js still fails, in the same way, so there must be something deeper going on. I'll look into that next.

I'll push this to the private repo as 793579-issuer-smaller.
this patch fixes proxy-idp-test, although I'm not sure it's the best fix. It's still minimal, leaving the verifier less strict than it should be.
Attachment #664366 - Attachment is obsolete: true
The last bit of attachment 664370 [details] [diff] [review] looks fine to me.

I haven't really got a good grasp of the proxy IDP stuff yeto have an opinion on that. Maybe we should get Austin to look at it tomorrow?

BTW, the verifier bug is also filed here: https://github.com/mozilla/browserid/issues/1501
Yeah, since the loose-verifier is old news, I don't think we need to include it in this hotfix. I'll focus on the smaller patch. Next step is to work out a test.

CCing Austin: will the changes in this patch break the existing proxyIDP work?
CCing Gene: heads up, hotfix for prod coming your way soon.
(In reply to Brian Warner [:warner :bwarner] from comment #14)
Yes, it does appear to break BigTent.

browserid (9533): error: uncaughtException
stack=[Error: Can't set headers after they are sent.
at ServerResponse.json (/home/ozten/browserid/bin/browserid:97:18),
at /home/ozten/browserid/lib/wsapi/auth_with_assertion.js:55:24,
at /home/ozten/browserid/lib/wsapi.js:109:5,
at Array.0 (/home/ozten/browserid/lib/db/json.js:437:33),
at EventEmitter._tickCallback (node.js:190:38)]

I think this is *okay* to get a hotfix out, and I'll work on fixing the patch. I'll circle back with you (bwarner) outside of this bug.
New patch: same code change as before, but this adds a unit test to exercise the problem.
Attachment #664370 - Attachment is obsolete: true
I've checked the verifier too, there's already a test for this sort of problem in verifier-test.js . So I think this patch is ready for review. It breaks bigtent, and doesn't address the long-standing loose verifier (which accepts secondary certs when it shouldn't), but I think both are fine for a hotfix.
CCing atoll since gene's out: heads up, hotfix coming down the pipe at you.
I have disabled BigTent in dev (login.dev.anosrep.org)
My testing with an awsbox has all passed, as well as the unit tests passing. We can move to get this on stage as soon as it's feasible for ops.
FYI, Austin's bigtent fix was incorporated into the diff (we're not sure how it was working correctly without in the first place, it's one of those "obviously correct" things to do). So maybe bigtent will survive this hotfix.
I pushed a dev-suitable version of this patch to the "793579-dev" branch on the private repo. I plan to land it on dev just after we deploy the fix to prod. Is there another train this fix needs to go into, to avoid regressing the bug the next time a train is deployed?
The deployment ticket for this fix is bug 794268. Looking to get this out tomorrow as early as feasible tomorrow.
Deployed to production, git revision a9376e5da9529f9cc05f553f259c1d31dc15ad5b, finished about 3pm 26-Sep-2012. Note that the version pushed does *not* include Austin's bigtent fix, which also means that delegation is broken. We'll fix this in a second hotfix, probably in the second week of october since everyone will be at a workweek until then.

The two smaller bugs (/wsapi/cert_key ignores "primary" flag, verifier accepts secondary certs even for primary IdPs) will be filed and fixed in the usual way (github and pull requests to "dev").

I'll push the dev version of this patch (which *does* include the bigtent fix) to the public repo this afternoon.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
BTW, when this bug is unhidden, we should probably also unhide 795175, which investigates the question of "how many users were impacted by this bug".
Group: mozilla-services-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: