Closed Bug 1208480 Opened 9 years ago Closed 9 years ago

persona.org vulnerable to Open redirect vulnerability

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: yaaboukir, Assigned: rfkelly)

Details

(Keywords: sec-moderate, wsec-impersonation) 

Hi,

I found out that Persona is vulnerable to an open redirection vulnerability.

Impact : The user may be subjected to phishing attacks by being redirected to an untrusted and attacjer controlled web page that appears to be a trusted web site. The phishers may then steal the user’s credentials and then use these credentials to access the legitimate web site. Because the server name in the modified link is identical to the original site, phishing attempts have a more trustworthy appearance.

Proof Of Concept :  https://login.persona.org///evilzone.org/%2E%2E

Mitigation : Update your Express framework.

Kind regards.
The redirect is indeed unfortunate. I can imagine that careless users would enter their credentials on the redirected site.

:rfkelly - could you have someone take a look at this?
Assignee: nobody → rfkelly
Status: UNCONFIRMED → RESOLVED
Closed: 9 years ago
Component: Other → Server: Identity
Flags: needinfo?(rfkelly)
Keywords: sec-low, wsec-impersonation
Product: Websites → Cloud Services
Resolution: --- → FIXED
Version: Production → unspecified
Sorry, didn't mean to mark this as Resolved, but Assigned.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Flags: needinfo?(rfkelly)
Flags: sec-bounty?
whoops, didn't mean to clear my ni? on this one
Flags: needinfo?(rfkelly)
> Mitigation : Update your Express framework.

Easier said than done, I'm afraid - the persona codebase appears to be pinned to express==2.5.0, two full major versions behind the current release.  I've no idea what breaking API changes may have taken place in the meantime.

I'm assuming this is CVE-2015-1164 [1], in which case we *may* be able to mitigate without updating express by disabling redirects, like this:

  app.use(express.static(static_root)) => app.use(express.static(static_root, { redirect: false }))

But who knows what other vulnerabilities might be unpatched in this ancient version of express.

:callahad, can you please help with triaging this?  Was there any existing work to get persona.org updated to a newer version of express?

[1] http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2015-1164

> Flags: sec-bounty?

I don't know what the criteria are around this, but it's certainly a legit open-redirect vuln.
Flags: needinfo?(rfkelly) → needinfo?(dan.callahan)
BTW the "disable redirects" mitigation was suggested at:

   https://nodesecurity.io/advisories/serve-static-open-redirect
No existing work that I know of to upgrade the deps. :(

The Yahoo bridge is also vulnerable: https://yahoo.login.persona.org///evilzone.org/%2E%2E

The Gmail bridge is on Express 4.x and not vulnerable.
Flags: needinfo?(dan.callahan)
I believe disabling redirects will be a fine mitigation.

This is predicated on the belief that the only reason to use redirects is to go from `/foo` to `/foo/`, which implies `/foo/index.html`. As far as I remember, and as far as I can tell from looking at our repos, we never used the static mount that way: it was only for images and scripts, not end-user visible HTML.

Case in point: even our effective static /about page doesn't have its index served by the static middleware: https://login.persona.org/about works without redirecting to `about/`.

Repos to update:

- https://github.com/mozilla/persona/
- https://github.com/mozilla/persona-yahoo-bridge/

We have private repos that you can use (they may need to get sync'd to the upstream, first) at:

- https://github.com/mozilla/browserid_private/ (Persona)
- https://github.com/mozilla/browserid-bigtent_private/ (Yahoo)
I tested this out locally and `{ redirect: false }` does indeed seem to mitigate the issue with affecting the functionality of the site (although you have to watch out for the browser caching the 302 redirect).

I prepared two private PRs with the changes:

  https://github.com/mozilla/browserid_private/pull/33
  https://github.com/mozilla/browserid-bigtent_private/pull/6

But they're probably not against the right branch or tag, because:

> (they may need to get sync'd to the upstream, first)

It's not obvious to me what's the right branch name to sync with in the corresponding public repos, e.g. the persona repo has things like "future" and "prod".  :callahad can you advise?
Flags: needinfo?(dan.callahan)
The team disintegrated right around when we were attempting to reorganize the repo's structure, so it's a bit of a mess. :(

You can figure out what revision is running in prod by hitting /ver.txt. Right now, Prod is on cc7cc58, which is the tip of the train-2014.07.19 branch.

I'd suggest confirming with :relud on this, but I think the best bet is probably to PR against both "train-2014.07.19" *and* "future" for Persona

Looks like the Yahoo bridge is on 3f02c86, which points to "train-2013.05.29-hotfix-9-24v3". I'd PR against that and against "master" for Yahoo.

Hooray for software archaeology! :)
Flags: needinfo?(dan.callahan) → needinfo?(dthornton)
I need it merged against the branches currently deployed, which match callahad's footwork. Merging it against the other branches would also be good practice, but is not a requirement for getting it deployed.

As for testing (as mentioned by shane on github), I could deploy the PR without merging it, and we could test it in stage for verification.
Flags: needinfo?(dthornton)
"deploy the PR without merging it" to stage, not to prod
Adding Shane to this bug, for review purposes.
> I think the best bet is probably to PR against both "train-2014.07.19" *and* "future" for Persona

Done, PRs at:

  https://github.com/mozilla/browserid_private/pull/34
  https://github.com/mozilla/browserid_private/pull/35
> Looks like the Yahoo bridge is on 3f02c86, which points to "train-2013.05.29-hotfix-9-24v3".
> I'd PR against that and against "master" for Yahoo.

Done, PRs at:

  https://github.com/mozilla/browserid-bigtent_private/pull/7
  https://github.com/mozilla/browserid-bigtent_private/pull/8
:relud, if you're happy to deploy these to stage for manual verification, that sounds like by far the easiest option.  The two PRs against current prod branch are:

  https://github.com/mozilla/browserid_private/pull/34
  https://github.com/mozilla/browserid-bigtent_private/pull/7
Flags: needinfo?(dthornton)
I think we also need a PR against the persona-gmail-bridge [1].

[1] - https://github.com/mozilla/persona-gmail-bridge/blob/master/bin/persona-gmail-bridge.js#L327
The bug was fixed in later versions of Express, so the Gmail bridge is OK. The Gmail bridge is using Express 4, which is not vulnerable, while the Yahoo bridge and main site are on Express 2, which is.

Compare, for example:

https://yahoo.login.persona.org///evilzone.org/%2E%2E

https://gmail.login.persona.org///evilzone.org/%2E%2E
This is a "low" rated security vulnerability on a site not on the list of available sites so I'm minusing this for bounty. If this winds up rated higher, please let us know.
Flags: sec-bounty? → sec-bounty-
Keywords: sec-low → sec-moderate
Re-nominating for bounty so we can examine it again as the security rating has changed.
Flags: sec-bounty- → sec-bounty?
adding :ckarlof, I probably should have cc'd him onto this earlier just for context...
this is deployed to persona and the yahoo bridge in stage.
Flags: needinfo?(dthornton)
Thanks :relud!  :callahad, do we have a standard QA testing procedure for stage?
Flags: needinfo?(dan.callahan)
(In reply to Ryan Kelly [:rfkelly] from comment #22)
> do we have a standard QA testing procedure for stage?

We used to have docs on WikiMo, but everything I can find there is way out of date. In the past, our testing regime was:

- Unit tests from the repo (Travis does this)
- Selenium tests from the repo (we had a SauceLabs account)
- Load tests (run by QA, monitored by ops)
- Manual smoke tests (run by QA)

For a change of such small magnitude, as long as Travis is green, I'd just do some manual prodding and make sure things work before promoting.

The stage environment lives at https://login.anosrep.org and https://yahoo.login.anosrep.org.

The RP at http://beta.123done.org points to stage, so you can test there.
Flags: needinfo?(dan.callahan)
I successfully logged in to http://beta.123done.org using both a yahoo and non-yahoo account, and everything seemed to work fine.  I clicked around in the https://login.anosrep.org a little and didn't notice anything out of place.  And I confirmed that the following do not redirect, confirming the bug has been fixed:

  https://login.anosrep.org///evilzone.org/%2E%2E
  https://yahoo.login.anosrep.org///evilzone.org/%2E%2E

That's about as much as I can do.  Over to :stomlinson as the reviewer on those PRs, for any additional testing he considers appropriate.
Flags: needinfo?(stomlinson)
Testing all *seems* fine. 

My concerns:
* I am unable to reproduce the original problem on prod. 
* I forget how to test Mozilla LDAP integration on stage. If we push and realize LDAP integration fails, a lot of folks will be unhappy.

:callahad, do you remember how to test Mozilla LDAP on stage?
Flags: needinfo?(stomlinson) → needinfo?(dan.callahan)
Ahha, I am able to recreate the original problem on prod, thanks for the links callahad.
Since the IdPs are deliberately designed as independent entities, I'd go ahead and push this to prod. It shouldn't be possible for MozIdP to be broken so long as we have evidence that another IdP (Gmail, Yahoo, MockMyID, etc) works.

(That's my polite way of saying "damn, I can't remember the staging MozIdP domain either")
Flags: needinfo?(dan.callahan)
mozidp doesn't work in staging, and hasn't for at least a year iirc
I'm hearing "shipit"; ni? shane for final merge on those PRs.

(sorry for playing constant ni? tag in this bug but I think it's the only way to keep track of this as we all go about our day jobs...)
Flags: needinfo?(stomlinson)
(off topic: I've *really* liked the frequent ni? callouts here. Seems like it made things work exceptionally smoothly!)
I have merged the two PRs referenced in [1]. :relud, as keeper of the trains, are [2] and [3] needed as well?


[1] - https://bugzilla.mozilla.org/show_bug.cgi?id=1208480#c15
[2] - https://github.com/mozilla/browserid_private/pull/35
[3] - https://github.com/mozilla/browserid-bigtent_private/pull/8
Flags: needinfo?(stomlinson) → needinfo?(dthornton)
2 and 3 aren't needed for deploying to prod.
Flags: needinfo?(dthornton)
We should merge them for completeness, but it can wait until after deploy.  So IIUC the remaining tasks here are:

* Deploy the now-updated branches to production
* Merge [2] and [3] in the private repo
* Copy all four branches back into the public repo
* Make some sort of announcement?
for the first bullet: what kind of qa are we looking at for deploy time. Is 'Daniel can log in to gmail/mozilla/yahoo/fallback' sufficient for this one?
also, for timeline, assuming the above testing plan is sufficient, does tomorrow, 10/15/15, at 2pm PDT sound like an acceptable deploy time?
Above testing plan is good. I'll defer to Ryan on the time window.
SGTM
Excellent! Thanks a lot for the report, and thank you team for fixing it. I'm resolving as fixed, and the bounty team will reevaluate during their next pass.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Thanks to all involved for pulling together to get this resolved.  Just to ensure we tie up the lose ends here:

> * Merge [2] and [3] in the private repo
> * Copy all four branches back into the public repo

I can take care of this; is there any reason to wait longer before doing so?

> * Make some sort of announcement?

:callahad, I think you're best positioned to advise on what we should do w.r.t. public communication here.
Flags: needinfo?(dan.callahan)
For public communication, we've tended to post on the Mozilla Security blog. E.g.,:

https://blog.mozilla.org/security/2013/10/02/bug-bounty-program-finds-and-helps-resolve-security-vulnerability-in-persona/

https://blog.mozilla.org/security/2014/04/08/heartbleed-security-advisory/

I don't believe an open redirect quite rises to the bar of needing a public disclosure, but I'm happy to be overruled if your gut says otherwise.

Either way, now that we're verified fixed in production and we've soaked for a few days without disruption, I think you can safely merge the private branches back into the public repos.

One additional action:

* Now that the bug is resolved, mark it as public?
Flags: needinfo?(dan.callahan)
Also, :rfkelly, I'm in favor of renaming the private repos to "pubname-private" instead of the codenames they have right now. Any objections before I up-and-do-it? GitHub does set up HTTP redirects, and access via git will continue to work as well.
> I'm in favor of renaming the private repos to "pubname-private" instead of the codenames
> they have right now. Any objections...

No objections.
(In reply to Dan Callahan [:callahad] from comment #41)
> I don't believe an open redirect quite rises to the bar of needing a public
> disclosure, but I'm happy to be overruled if your gut says otherwise.

I would agree that posting on the security blog is overkill here.
Flags: sec-bounty? → sec-bounty+
> I would agree that posting on the security blog is overkill here.

I'm happy with that.

If it's sec-bounty+ it feels like we should say something *somewhere* but I don't have an appropriate forum to suggest, so I'm going to leave it alone.

> One additional action:
> * Now that the bug is resolved, mark it as public?

I don't have the necessary privs to do that; :ulfr?
Group: websites-security
Summary: persona.org vulnerable to Open redirect vulnreability → persona.org vulnerable to Open redirect vulnerability
You need to log in before you can comment on or make changes to this bug.