1255697 - Limit characters allowed in email addresses by browserid-local-verify

Status: RESOLVED FIXED

(Cloud Services :: Server: Identity, defect)

Description

[Ryan Kelly [:rfkelly]]

Sorry :francois, you can't escape this thing just yet :-)

We recently had some trouble with the hosted verifier at https://verifier.login.persona.org/ allowing though malicious assertions with things like newlines and null bytes in the email address, which could be used to exploit reliers like e.g. Bug 1253795.

We fixed it by tightening the validation in the verifier, so that it rejects assertions that don't contain a well-formed email address, see Bug 1253495.

As a precautionary measure, I'd like to similarly tighten the validation in browserid-verifier and get it deployed to https://verifier.accounts.firefox.com before we make any public disclosure of the issue.  Mind a quick r?

This patch runs the asserted email address through a very simple regex that checks:

* there's only a single "@" symbol
* the hostname only contains chars allowed in a hostname
* the username only contains chars allowed in a username and unlikely to cause problems for reliers

The sets of allowed characters are taken from the version of the "validator" library currently used in the persona.org codebase.  I erred on the side of keeping the regex simple to understand, rather than trying to strictly validate all the various RFC rules about what is and is not a valid email address.

I'll also note that despite removing it from the `extractDomainFromEmail` function, `validateAuthority` stills gets called on the hostname extract from the email, since `browserid.lookup` always calls it before attempting network access.

Comment 1

[Benson Wong [:mostlygeek]]

It's 2016, couldn't we just NPM a module for this?

Comment 2

[Ryan Kelly [:rfkelly]]

We could use (an updated version of) the package that persona.org uses [1] but I've just been in the habit of keeping changes to these modules to an absolute minimum.

[1] https://www.npmjs.com/package/validator

Comment 3

[Ryan Kelly [:rfkelly]]

Hmm, validator seems to both reject things that we currently treat as valid, such an explicit port number in the host component (used extensively in the test suite):

  > validator = require('validator')
  > validator.isEmail('[e-mail]:8080')
  false

And to allow things that we want to reject, like newlines and "@"-signs if they happen to be in a quoted username component:

  > validator.isEmail('"[e-mail]\n"@rfk.evil.com')

So that one seems like a non-starter for our purposes here.

Comment 4

[Benson Wong [:mostlygeek]]

Didn't mean to derail. I guess it's 2015 and we *do* need better email validators.

Comment 5

[François Marier [:francois]]

Comment on attachment 8729344 [details] [diff] [review]
bid-local-verify-email-validation.diff

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

r- because I'm worried about the handling of the "-" character in the username part of the regexp. The rest are just nits.

::: lib/validation.js
@@ +5,5 @@
>  const urlparse = require('url').parse;
>  
> +function validateEmail(email) {
> +  // Emails cannot be longer than 254 characters.
> +  if (email.length >= 255) {

Does it make sense to also check for `email.length < 3`?

@@ +14,5 @@
> +  // The set of allows chars is taken from the validation rules in persona.
> +  // This excludes some technically-valid things (e.g. quoted user portions)
> +  // and allows others (e.g. multiple consecutive dots) but is enough
> +  // to ensure there's nothing malicious going on.
> +  var match = email.match(/^([\w!#$%&'*+-/=?^`{|}~.]+)@([a-zA-Z0-9.\-]+(:[0-9]{1,5})?)$/);

I think the "-" might need to be escaped here to ensure it doesn't get used as an interval ("+ to /"). I usually escape the hyphen and then put it last in my regexps just to make sure I don't end up with a surprise.

Suggestion: maybe add support for commas in the username part? I've seen that before I think.

Are underscores allowed in hostnames?

Comment 6

[Ryan Kelly [:rfkelly]]

> Didn't mean to derail.

Heh, s'okay - I'm also coming at this fresh from patching in persona so I've got my "oh god dont change too much or you might break something" hat on, which probably doesn't apply quite so much in this codebase :-)

> I think the "-" might need to be escaped here to ensure it doesn't get used as an interval ("+ to /")

Nice catch, thanks, I'll update.

Comment 7

[Ryan Kelly [:rfkelly]]

> Are underscores allowed in hostnames?

They are not.

> maybe add support for commas in the username part?

This doesn't seem to be supported by the validation on login.persona.org so I'm going to leave it out here as well, for consistency.

I could, for full consistency, just copy the regex currently used by login.persona.org, which is:

  /^(?:[\w\!\#\$\%\&\'\*\+\-\/\=\?\^\`\{\|\}\~]+\.)*[\w\!\#\$\%\&\'\*\+\-\/\=\?\^\`\{\|\}\~]+@(?:(?:(?:[a-zA-Z0-9](?:[a-zA-Z0-9\-](?!\.)){0,61}[a-zA-Z0-9]?\.)+[a-zA-Z0-9](?:[a-zA-Z0-9\-](?!$)){0,61}[a-zA-Z0-9]?)|(?:\[(?:(?:[01]?\d{1,2}|2[0-4]\d|25[0-5])\.){3}(?:[01]?\d{1,2}|2[0-4]\d|25[0-5])\]))$/

But these big old inscrutable regexes scare me a little.

Comment 8

[Ryan Kelly [:rfkelly]]

Attachment: bid-local-verify-email-validation.diff

Comment 9

[John Morrison [:jrgm]]

(In reply to Ryan Kelly [:rfkelly] from comment #7)
> > Are underscores allowed in hostnames?
> 
> They are not.
> 

Just noting for completeness: history https://github.com/mozilla/persona/pull/1456

Comment 10

[John Morrison [:jrgm]]

> Just noting for completeness: history https://github.com/mozilla/persona/pull/1456

Sorry. Should have not just left a link. They are allowed in practice, despite RFCs.

Comment 11

[Ryan Kelly [:rfkelly]]

Attachment: bid-local-verify-email-validation.diff

Actually, we have an existing validateAuthority() function that's specifically for checking our expectations of the hostname portion, I've updated my patch to just call it directly.

Comment 12

[Ryan Kelly [:rfkelly]]

> we have an existing validateAuthority() function

Which does, AFAICT, accept underscores in the hostname :-)

Comment 13

[Ryan Kelly [:rfkelly]]

:jrgm, :mostlygeek, if possible I'd like to deploy this to https://verifier.accounts.firefox.com from a private commit, so we can make it public at the same time as the equivalent fix on persona.org.  Is that doable?

It does not need to be deployed with any urgency to the private verifiers used by tokenserver and fxa-oauth-server; both of those do an explicit issuer check which will prevent them from accepting malicious assertions.

Comment 14

[Ryan Kelly [:rfkelly]]

I've made a v0.4.0 in a private repo here:

  https://github.com/mozilla/browserid-local-verify-private/commit/b4dd5599976f2ef23aa4ea71a7a260276a57c52d

Will I need to prepare a matching new version of browserid-verifier in a matching private repo?  Who's going to be deploying this thing anyway? I *think* I heard that :mostlygeek was still in charge of this one but I could easily have mis-understood :-)

Comment 15

[Benson Wong [:mostlygeek]]

I think jrgm is the primary for all things FxA now. 
When it merges into the public repo I will update tokenserver accordingly.

Comment 16

[Ryan Kelly [:rfkelly]]

Since we have no reason to believe there's an active vulnerability due to the lack of validation here, we've decided to proceed with public disclosure of Bug 1253795 without blocking on this deployment.  Given that, I've made the changes public and we can deploy from a public tag:

  https://github.com/mozilla/browserid-verifier/releases/tag/0.6.0

Comment 17

[Ryan Kelly [:rfkelly]]

Deploy will be tracked in Bug 1271856