Disallow email addresses which will overflow MySQL storage
ClosedPublic
Actions

Authored by epriestley on Feb 23 2014, 5:40 PM.

Details

Reviewers

arice
btrahan

Commits

Restricted Diffusion Commit
rP7cf0358dda11: Disallow email addresses which will overflow MySQL storage

Summary

Via HackerOne. An attacker can bypass auth.email-domains by registering with an email like:

aaaaa...aaaaa@evil.com@company.com

We'll validate the full string, then insert it into the database where it will be truncated, removing the @company.com part. Then we'll send an email to @evil.com.

Instead, reject email addresses which won't fit in the table.

STRICT_ALL_TABLES stops this attack, I'm going to add a setup warning encouraging it.

Test Plan

Set auth.email-domains to @company.com.
Registered with aaa...aaa@evil.com@company.com. Previously this worked, now it is rejected.
Did a valid registration.
Tried to add aaa...aaaa@evil.com@company.com as an email address. Previously this worked, now it is rejected.
Did a valid email add.
Added and executed unit tests.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

very nice

Here's the new error on registration:

{F117037}

arice accepted this revision.Feb 23 2014, 6:12 PM

arice added inline comments.

src/applications/people/storage/PhabricatorUserEmail.php
35–41	Is it intentional that fully qualified addresses are not required? I'd assume so given that Phabricator might be running on an internal MTA capable of resolving partial addresses. If not, this probably introduced a good opportunity to add user-friendly validation for valid chars and the presence of a domain. But that's not relevant to this issue.

It's intentional that we don't try to validate the format of the address, yeah. I did consider adding basic validation here (e.g., requiring it to contain @) but since this is a security issue I didn't want to conflate things by glomming it on here. I also suspect very few users would be helped by a warning like that, and it's plausible (if unlikely) that Phabricator might be configured for local delivery.

(An approach I've seen elsewhere is looking for gaiml.com, etc., and suggesting a correction to gmail.com -- which seems more likely to me to catch errors than checking for @ and . -- but as far as I'm aware users do not generally have difficulty entering their email address correctly. Most installs use some other identity provider as an authority anyway, and we just read the address out of that, so they'd have to be pathologically bad at forms to get it wrong.)