Page MenuHomePhabricator

Prevent inbound processing of the "void/placeholder" address and other reserved addresses
ClosedPublic

Authored by epriestley on Thu, Jan 3, 11:59 PM.

Details

Summary

Depends on D19952. Ref T13222. Never process mail targets if they match:

  • The "default" address which we send mail "From".
  • The "void" address which we use as a placholder "To" when we only have "CC" addresses.
  • Any address from a list of reserved/administrative names.

The first two prevent loops. The third one prevents abuse.

There's a reasonably well-annotated list of reservations and reasons here:

https://webmasters.stackexchange.com/questions/104811/is-there-any-list-of-email-addresses-reserved-because-of-security-concerns-for-a

Stuff like support@ seems fine; stuff like ssladmin@ might let you get SSL certs issued for a domain you don't control.

Also, forbid users from creating application emails with these reserved addresses.

Finally, build the default and void addresses somewhat more cleverly.

Test Plan

Added unit tests, tried to configured reserved addresses, hit the default/void cases manually with bin/mail receive-test.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Thu, Jan 3, 11:59 PM
epriestley requested review of this revision.Fri, Jan 4, 12:01 AM
amckinley accepted this revision.Fri, Jan 4, 10:25 PM
amckinley added inline comments.
src/applications/config/option/PhabricatorMetaMTAConfigOptions.php
197

Why was this ever the default?

Also, maybe this option should only let you specify the user portion of the default, and always construct the domain part correctly (as we now do below in the case when this isn't set).

src/applications/metamta/storage/PhabricatorMetaMTAMail.php
1466

I'm sure there's a very good reason for the existence of this, but I'm also pretty sure the answer would horrify me.

This revision is now accepted and ready to land.Fri, Jan 4, 10:25 PM
epriestley added inline comments.Fri, Jan 4, 10:56 PM
src/applications/config/option/PhabricatorMetaMTAConfigOptions.php
197

I think there are some possible cases where you have dev.acme-internal.com vs acme.com vs acmecorp.com in different roles, or you don't set up inbound mail but already have some noreply@ configured elsewhere, so I think there are probably some use cases for using a domain other than the one we'd automatically pick, although I'm not sure any have a very strong argument.

With the new code, you can (probably) specify a display name ("Acme Phabricator" <noreply@acme-corp.com>) and it should work properly since we're carrying everything down the stack as a PhutilEmailAddress. I haven't really tested this and haven't documented it, but this seems like a maybe-nice-ish thing and it's tricker if we don't let you specify the whole thing (although we could just forcefully overwrite the domain part).

This was a pretty bad default, but the option is very old and it took a while after 2011 to get Config/Setup into a reasonable place. I imagine it was just faithfully upgraded from some default.config.template.example, although I didn't spelunk for an origin.

src/applications/metamta/storage/PhabricatorMetaMTAMail.php
1466

Phabricator on dev.acme.com, employee mail through alice@acmecorp.net, like at Facebook circa 2011. I'm reasonably sympathetic to not being able to put MX records on the Phabricator domain, or wanting to reuse an existing setup on some other domain for convenience.

This revision was automatically updated to reflect the committed changes.