WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 5 days ago

#25239 reviewing defect (bug)

$_SERVER['SERVER_NAME'] not a reliable when generating email host names

Reported by: layotte Owned by: SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version: 3.8
Component: Mail Keywords: has-patch dev-feedback needs-testing
Focuses: Cc:

Description (last modified by SergeyBiryukov)

For quite some time I have been having an issue with my comment notifications. The From address has been wordpress@_. I haven't paid much attention to this, but I had some spare time and decided to pursue the issue. Here it is...

I am running WPMS w/ domains (latest stable) with Nginx and PHP5-FPM. In WordPress the comment notifications from email addresses are being generated using $_SERVER['SERVER_NAME'] to get the current site's domain name.

e.g.

$wp_email = 'wordpress@' . preg_replace('#^www\.#', '', strtolower($_SERVER['SERVER_NAME']));

However, because of my environment my Nginx config for my site has the server_name set to "_" (underscore). Which is a catchall -- http://nginx.org/en/docs/http/server_names.html.

Site config in Nginx:

# Redirect everything to the main site.
server {
        listen [::]:80 default_server;
        listen [::]:443 ssl;

        ssl_certificate         /etc/nginx/ssl.crt/xxx.com.2012.crt;
        ssl_certificate_key     /etc/nginx/ssl.key/xxx.com.2012.key;

        server_name _;
        root /var/www/xxx.com;
        access_log /var/log/nginx/xxx.com.access.log;
        error_log /var/log/nginx/xxx.com.error.log;
        client_max_body_size 100m;

        if ($http_host = "www.xxx.com") {
                rewrite ^ http://xxx.com$request_uri permanent;
        }

        include global/restrictions.conf;

        # Additional rules go here.

        include global/wordpress-ms.conf;

}

The default fastcgi_params has this set:

fastcgi_param   SERVER_NAME             $server_name;

Thus, $_SERVER['SERVER_NAME'] is outputting "_" (underscore).

I propose we move away from $_SERVER['SERVER_NAME'] when generating the from email addresses and use the available $current_site global object which stores the domain variable ($current_site->domain). I have implemented this change on my own site and have included the patch here.

In the meantime, anyone else facing this problem can change their fastcgi_param to $host instead of $server_name. In my opinion, not the best solution, but it works for now.

Attachments (11)

server_name.diff (4.3 KB) - added by layotte 4 years ago.
Replace $_SERVERSERVER_NAME? w/ $current_site->domain
25239.diff (3.4 KB) - added by jesin 3 years ago.
Refreshed server_name.diff and corrected minor errors
pluggable.php.diff (700 bytes) - added by mt8.biz 9 months ago.
25239.2.diff (3.6 KB) - added by jesin 9 months ago.
Refreshed 25239.diff
25239.3.patch (3.5 KB) - added by desmith 8 months ago.
25239.4.patch (3.7 KB) - added by desmith 8 months ago.
25239.5.patch (3.9 KB) - added by desmith 7 months ago.
25239.6.patch (4.1 KB) - added by desmith 7 months ago.
pluggable.php.2.diff (441 bytes) - added by tsimmons 3 weeks ago.
Replaces domain part with the typical email domain for a site, which includes the last domain part + TLD
pluggable.php.3.diff (668 bytes) - added by tsimmons 3 weeks ago.
(Reuploading to use proper SVN diff) Replaces domain part with the typical email domain for a site, which includes the last domain part + TLD ... for example, if the site is blog.wordpress.org the email domain becomes wordpress.org
CVE-2017-8295.patch (941 bytes) - added by cloudstek 3 weeks ago.

Download all attachments as: .zip

Change History (100)

@layotte
4 years ago

Replace $_SERVERSERVER_NAME? w/ $current_site->domain

#1 @SergeyBiryukov
4 years ago

  • Description modified (diff)

Related: #16805

#2 @nacin
4 years ago

nginx has an odd quirk that, given:

server_name example.com blog.example.com microsite.example.com;

And:

fastcgi_param SERVER_NAME $server_name;

When accessing microsite.example.com, SERVER_NAME will be set to example.com. This is akin to always using ServerName in Apache, rather than using ServerAlias (which is what happens).

I agree that SERVER_NAME should not be used, as depending on the setup it is not going to match HTTP_HOST. That said, if you are using a catchall like _ (it is actually meaningless, any special character will do), you should consider doing something like set $name X in your server block to ensure $server_name is what you want it to be, or actuall change the fastcgi_param call to set SERVER_NAME to $host. That will fix your issues.

So, this isn't a duplicate of #16805 — rather, we should investigate and consider a switch to HTTP_HOST here.

#3 @layotte
4 years ago

Would HTTP_HOST be better than $current_site->domain?

Last edited 4 years ago by layotte (previous) (diff)

#4 @cmmarslender
4 years ago

  • Cc cmmarslender added

#5 @3flex
4 years ago

  • Cc 3flex added

This ticket was mentioned in IRC in #wordpress-dev by markoheijnen. View the logs.


3 years ago

#7 in reply to: ↑ description @sreedoap
3 years ago

I wanted to note my use case for this which caused me to run into this issue. I am calling wp_install from a command line script I made which is ran for our users who launch a WordPress site, so HTTP_HOST and SERVER_NAME were not set for me. I manually set it in my script and that fixed my issue but it would probably be wise for the wp_mail function to have a fallback if it is unable to get a valid value from HTTP_HOST or SERVER_NAME so emails don't appear to come from an "unknown sender".

Replying to layotte:

For quite some time I have been having an issue with my comment notifications. The From address has been wordpress@_. I haven't paid much attention to this, but I had some spare time and decided to pursue the issue. Here it is...

I am running WPMS w/ domains (latest stable) with Nginx and PHP5-FPM. In WordPress the comment notifications from email addresses are being generated using $_SERVER['SERVER_NAME'] to get the current site's domain name.

e.g.

$wp_email = 'wordpress@' . preg_replace('#^www\.#', '', strtolower($_SERVER['SERVER_NAME']));

However, because of my environment my Nginx config for my site has the server_name set to "_" (underscore). Which is a catchall -- http://nginx.org/en/docs/http/server_names.html.

Site config in Nginx:

# Redirect everything to the main site.
server {
        listen [::]:80 default_server;
        listen [::]:443 ssl;

        ssl_certificate         /etc/nginx/ssl.crt/xxx.com.2012.crt;
        ssl_certificate_key     /etc/nginx/ssl.key/xxx.com.2012.key;

        server_name _;
        root /var/www/xxx.com;
        access_log /var/log/nginx/xxx.com.access.log;
        error_log /var/log/nginx/xxx.com.error.log;
        client_max_body_size 100m;

        if ($http_host = "www.xxx.com") {
                rewrite ^ http://xxx.com$request_uri permanent;
        }

        include global/restrictions.conf;

        # Additional rules go here.

        include global/wordpress-ms.conf;

}

The default fastcgi_params has this set:

fastcgi_param   SERVER_NAME             $server_name;

Thus, $_SERVER['SERVER_NAME'] is outputting "_" (underscore).

I propose we move away from $_SERVER['SERVER_NAME'] when generating the from email addresses and use the available $current_site global object which stores the domain variable ($current_site->domain). I have implemented this change on my own site and have included the patch here.

In the meantime, anyone else facing this problem can change their fastcgi_param to $host instead of $server_name. In my opinion, not the best solution, but it works for now.

#8 @SergeyBiryukov
3 years ago

#27811 was marked as a duplicate.

#9 @szepe.viktor
3 years ago

Automatic updater sends an email but SERVER_NAME is not defined when wp-cron starts as a real Linux cron job.

Please uset get_option('site_url') as a fallback to determine the hostname.

@jesin
3 years ago

Refreshed server_name.diff and corrected minor errors

#10 @jesin
3 years ago

Tested 25239.diff on a multisite Nginx environment with the following code.

add_filter( 'pre_site_option_admin_email', function() {
	return '';
});
wpmu_signup_user_notification( 'jesin', 'my@email.com', 'abcd' );
wpmu_welcome_notification( 2, 1, 'password', 'Hello World' );
wpmu_welcome_user_notification( 1, 'password' );
wpmu_signup_blog_notification( 'example.com', '/', 'Hello World', 'jesin', 'my@email.com', 'abcd' );
wp_mail( 'my@email.com', 'Subject', 'A message without From: header' );

This patch uses home_url() for single site and $current_blog->domain for multisite inside functions wp_mail() and wp_notify_postauthor() because home_url() returns the subdomain even if domain mapping is configured.

#11 in reply to: ↑ description @kitchin
3 years ago

Replying to layotte:

I propose we move away from $_SERVER['SERVER_NAME'] when generating the from email addresses and use the available $current_site global object which stores the domain variable ($current_site->domain).

I suggest also moving away from $_SERVER['HTTP_HOST']. At first glance, HTTP_HOST seems worse because it's more closely derived from user input, but actually both can be buggy depending on the web server. Apache has had bugs. See http://stackoverflow.com/questions/2297403/http-host-vs-server-name for a pretty detailed write-up of SERVER_NAME vs. HTTP_HOST.

#12 @SergeyBiryukov
21 months ago

#33698 was marked as a duplicate.

#13 @SergeyBiryukov
21 months ago

  • Milestone changed from Awaiting Review to 4.4

This ticket was mentioned in Slack in #core by sergey. View the logs.


21 months ago

#15 @SergeyBiryukov
21 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

This ticket was mentioned in Slack in #core by sergey. View the logs.


20 months ago

#17 @wonderboymusic
19 months ago

  • Milestone changed from 4.4 to Future Release

This ticket was mentioned in Slack in #core by jfanjoy. View the logs.


15 months ago

#19 @SergeyBiryukov
12 months ago

#16805 was marked as a duplicate.

#20 @ocean90
10 months ago

#37548 was marked as a duplicate.

#21 @mensmaximus
9 months ago

Can we fix this for 4.7 please?

There are several places in WordPress where we rely on $_SERVER['SERVER_NAME'] or $_SERVER['HTTP_HOST']. While I personally never trust in $_SERVER['SERVER_NAME'] I understand some feel uncomfortable with $_SERVER['HTTP_HOST'] either.

Given the fact that home_url and blog_id are always set and can be trusted, introducing a new function might be a solution:

function get_current_domain() {
        return parse_url( get_home_url( get_current_blog_id() ), PHP_URL_HOST );
}

The function name follows the syntax from other get_ functions in WordPress. It's name says what it does. It relies on WordPress core functions and settings that always exist. It works with PHP down to 4.0.

So instead of using $_SERVER['SERVER_NAME'] or $_SERVER['HTTP_HOST'] you could use get_current_domain().

#22 @mensmaximus
9 months ago

To make the function more flexible and to strip the subdomain for email addresses we could pass some arguments:

function get_current_domain( $strip_subdomain = true, $blog_id = '' ) {
        $blog_id = empty( $blog_id ) ? get_current_blog_id() : $blog_id;
        $domain = parse_url( get_home_url( $blog_id ), PHP_URL_HOST );
        if ( true === $strip_subdomain ) {
                $domain_parts = explode ( ".", $domain );
                array_shift( $domain_parts );
                $domain = implode( '.', $domain_parts );
        }
        return $domain;
}

#23 @mt8.biz
9 months ago

From WordPress4.6, wp_mail uses phpmailer->setfrom, ”wordpress@_” is always results error.

https://core.trac.wordpress.org/changeset/38287

error message is:

Fatal error: Uncaught exception 'phpmailerException' with message 'Invalid address: wordpress@_' in /var/www/vhosts/i-XXXXXXXX/wp-includes/class-phpmailer.php:946
Stack trace:
#0 /var/www/wordpress/wp-includes/pluggable.php(352): PHPMailer->setFrom('wordpress@_', 'WordPress')
#1 /var/www/wordpress/wp-includes/pluggable.php(1726): wp_mail('hogehoge@...', '[SITE NAME]')
#2 /var/www/wordpress/wp-includes/user.php(2350): wp_new_user_notification(35, NULL, 'both')
#3 [internal function]: wp_send_new_user_notifications(35, 'both')
#4 /var/www/wordpress/wp-includes/plugin.php(524): call_user_func_array('wp_send_new_use...', Array)
#5 /var/www/wordpress/wp-admin/includes/user.php(196): do_action('edit_user_creat...', 35, 'both')
#6 /var/www/wordpress/wp-admin/user-new.php(116): edit_user()
#7 {main} thrown in /var/www/wordpress/wp-includes/class-phpmailer.php on line 946

Last edited 9 months ago by mt8.biz (previous) (diff)

#24 follow-up: @Grzegorz.Janoszka
9 months ago

As mentioned earlier - it completely doesn't work when system cron is used. Please fix it.

#25 in reply to: ↑ 24 ; follow-up: @mt8.biz
9 months ago

Replying to Grzegorz.Janoszka:

As mentioned earlier - it completely doesn't work when system cron is used. Please fix it.

How to reproduce?

#26 in reply to: ↑ 25 ; follow-up: @Grzegorz.Janoszka
9 months ago

Replying to mt8.biz:

As mentioned earlier - it completely doesn't work when system cron is used. Please fix it.

How to reproduce?

Wordpress + nginx + system cron sending emails.

Not sure if nginx is required, when you use system cron the variables coming from http daemon will not be set. How do you expect to get anything from $_SERVER['SERVER_NAME'] or $_SERVER['HTTP_HOST'] when you just call the php script from cron (php wp-cron.php)?

Last edited 9 months ago by Grzegorz.Janoszka (previous) (diff)

#27 in reply to: ↑ 26 ; follow-up: @mt8.biz
9 months ago

Replying to Grzegorz.Janoszka:

Replying to mt8.biz:

As mentioned earlier - it completely doesn't work when system cron is used. Please fix it.

How to reproduce?

Wordpress + nginx + system cron sending emails.

Not sure if nginx is required, when you use system cron the variables coming from http daemon will not be set. How do you expect to get anything from $_SERVER['SERVER_NAME'] or $_SERVER['HTTP_HOST'] when you just call the php script from cron (php wp-cron.php)?

Should use the DB settings instead of using the $ _SEREVER I think

#28 in reply to: ↑ 27 ; follow-up: @mensmaximus
9 months ago

Replying to mt8.biz:

Should use the DB settings instead of using the $ _SEREVER I think

I already posted a suggestion to use the home_url: https://core.trac.wordpress.org/ticket/25239#comment:22

#29 in reply to: ↑ 28 @mt8.biz
9 months ago

Replying to mensmaximus:

Replying to mt8.biz:

Should use the DB settings instead of using the $ _SEREVER I think

I already posted a suggestion to use the home_url: https://core.trac.wordpress.org/ticket/25239#comment:22

yes. I khow.

#30 @mt8.biz
9 months ago

This is a fatal problem from ver4.6. I think it should be fix quick.

Last edited 9 months ago by mt8.biz (previous) (diff)

#31 @cbutlerjr
9 months ago

@jesin's patch is a little more thorough in addressing the issue - it also covers the wp_notify_postauthor() function as well as ms-functions.php. But it probably needs a refresh.
https://core.trac.wordpress.org/attachment/ticket/25239/25239.diff

@jesin
9 months ago

Refreshed 25239.diff

#32 follow-up: @dd32
9 months ago

An issue affecting 4.6 installs is fixed in 4.6.1, see #37736

#33 in reply to: ↑ 32 @Grzegorz.Janoszka
9 months ago

Replying to dd32:

An issue affecting 4.6 installs is fixed in 4.6.1, see #37736

dd32: the one we are discussing here is completely different and unrelated. It is about using HTTP variables when they are not available (like when calling wp-cron script from system cron.

#34 @ocean90
9 months ago

#37945 was marked as a duplicate.

#35 @BjornW
9 months ago

@ocean90 & @dd32 is there a reason for not accepting @jesin's patch?

This ticket was mentioned in Slack in #cli by danielbachhuber. View the logs.


9 months ago

#37 follow-up: @neodjandre
8 months ago

I am facing the same problem using:

Nginx + Postfix + Real Cron Jobs

I am receiving these errors:

Error message
phpmailerException: Uncaught exception 'phpmailerException' with message 'Invalid address: wordpress@' in /var/www/html/wp-includes/class-phpmailer.php:946

Stack trace
in PHPMailer::setFrom called at /var/www/html/wp-includes/pluggable.php (352)
in wp_mail called at /var/www/html/wp-includes/user.php (1841)
in wp_update_user called at /var/www/html/wp-admin/includes/user.php (182)
in edit_user called at /var/www/html/wp-admin/user-edit.php (147)

#38 in reply to: ↑ 37 @BjornW
8 months ago

In my case I've fixed this with this small plugin. It uses the admin user's email address as a From address thus enforcing a working email address. For now, this is a workable solution for me at least until @jesin's patch or someone else's is accepted in WordPress. Hope this helps.

Replying to neodjandre:

I am facing the same problem using:

Nginx + Postfix + Real Cron Jobs

I am receiving these errors:

Error message
phpmailerException: Uncaught exception 'phpmailerException' with message 'Invalid address: wordpress@' in /var/www/html/wp-includes/class-phpmailer.php:946

Stack trace
in PHPMailer::setFrom called at /var/www/html/wp-includes/pluggable.php (352)
in wp_mail called at /var/www/html/wp-includes/user.php (1841)
in wp_update_user called at /var/www/html/wp-admin/includes/user.php (182)
in edit_user called at /var/www/html/wp-admin/user-edit.php (147)

#39 @Ipstenu
8 months ago

FYI, since we changed how phpMailer validates emails, this has caused a previously silent error in wp-cli to bomb out.

https://github.com/wp-cli/wp-cli/issues/3374

the tl;dr version is that because the email is being passed as wordpress@ without a domain (since php CLI can't grab that variable), and phpmailer now flags that as an error, no one can reset passwords on command line unless they use the --url parameter to force the domain in.

The change in 4.6 (and 4.6.1) highlights a problem that had been flying under the radar for a long time, I think.

#40 @ocean90
8 months ago

#38161 was marked as a duplicate.

#41 @danielbachhuber
8 months ago

At the very least, could we get a try... catch around the thrown Exception?

#42 @Ipstenu
8 months ago

From #38161

Using something like substr( home_url( '', 'http'),7) might be a suitable substitute when $_SERVER is unavailable. (If there's a better way to get a site's hostname than slicing up home_url() that of course would work too.)

We'd want to use parse_url() I think, since https is everywhere. Also some people use WWW and some don't :/

@desmith
8 months ago

#43 @desmith
8 months ago

This patch combines @jesin's "replace it in a bunch of spots" and @mensmaximus's "add a new function for this" techniques. I omitted the parameters for get_current_domain() because there's only one current domain at any given time. First patch, probably did everything wrong, et cetera.

#44 @desmith
8 months ago

Please ignore the above patch (which doesn't fix all the cases of this that I've encountered), in lieu of another one coming shortly. Apologies.

#45 @desmith
8 months ago

Okay, let's try this again.

This patch adds a new function in pluggable.php, get_current_domain(). Does what it says on the tin, returns the current blog domain name, with leading 'www.' stripped. It also modifies a few places where emails are sent, to use that function instead of $_SERVER['SERVER_NAME'], notably in the password reset email-send, which fixes the issue with wp-cli password reset emails.

There is a possible side effect. Since I replaced $_SERVER['SERVER_NAME'] in several email-related places, anyone that's depending on their emails' From: headers looking like support@… instead of support@theirsite will be disappointed. I could un-patch those instances, of course, but I think this behavior is cleaner overall.

@desmith
8 months ago

#46 follow-up: @joemcgill
7 months ago

Replacing $_SERVER['SERVER_NAME'] with output from a helper function in these instances seems like a good idea to me and could be useful outside of instances where we're building email addresses. I question whether this should be a pluggable function, though. This function feels more like get_site_url() or get_home_url() which live in wp-includes/link-template.php.

I also wonder if it was an oversight that none of the multisite functions were stripping www subdomain prefixes or if that was intentionally allowed? Either way, 25239.4.patch has a redundant preg_replace() call if that method is used inside the helper function.

#47 in reply to: ↑ 46 @jdgrimes
7 months ago

Replying to joemcgill:

I question whether this should be a pluggable function, though.

Yeah, I think that the consensus is that pluggable functions were a mistake, and filter and action hooks should be used to provide overridability/customizability. So no new pluggable functions are being added, as far as I know.

So the function should be moved to a different file as you've suggested, and we should consider the merits of adding a filter on the value that it returns.

@desmith
7 months ago

#48 follow-up: @desmith
7 months ago

As suggested, I've moved the new function to a more-appropriate place, and added an apply_filters call.

@joemcgill , you were right in that I overlooked a preg_replace call; thanks for the catch.

The question of whether we should remove leading 'www.' in names here is still open. It is a change, and while I'd argue it's a change for the better, it's a pretty weak argument.

#49 in reply to: ↑ 48 @mensmaximus
7 months ago

Replying to desmith:

The question of whether we should remove leading 'www.' in names here is still open. It is a change, and while I'd argue it's a change for the better, it's a pretty weak argument.

Please ounce again take a look at my suggestion: https://core.trac.wordpress.org/ticket/25239#comment:22

As you can see I have addressed the subdomain issue by passing a parameter to the function. I also passed a blog id parameter to make the function more universal.

#50 @Ipstenu
7 months ago

@joemcgill - Probably an oversight or an 'It didn't break, leave it alone.' which actually applies to this change as a whole ;)

Leading www's should be removed when we're talking about _email_ however. You don't email support@… after all. :) (Hopefully a better argument since part of the point of this ticket is making VALID email address)

#51 @joemcgill
7 months ago

@Ipstenu thanks for pointing out that those are for the From: emails. Even so, your point about "leave it alone" stands, so I wonder if we should only use home_url() here as a fallback whenever $_SERVER['SERVER_NAME'] isn't set—which is the reason for this ticket in the first place.

@mensmaximus Thanks for the reminder about your approach. If I understand your thinking, making this function more flexible would also allow it to be used instead of $_SERVER['HTTP_HOST'] as well, correct? If so, I worry about there becoming overlap/confusion between this function and already existing functions, like get_site_url(), get_home_url(), etc.

Is there a needed use case for a function that does any more than checks to see if $_SERVER['SERVER_NAME'] is set, and if not, returns home_url() (with a www subdomain stripped, if applicable)? If not, it may be better to keep the scope of this narrowly defined as a wrapper around $_SERVER['SERVER_NAME'] and to reinforce its purpose in the function name—so maybe something like wp_server_name() or get_server_name() instead.

@desmith
7 months ago

#52 @desmith
7 months ago

Let's reduce the scope a bit, then. Latest version just has a function called get_email_domain() that returns the domain name, leading www. stripped, and that's it. Function name is a bit more clear, at least. All the places it's being used are related to email, so the function name and its use make a bit more sense.

If we want to add an if{} to only act if $_SERVER['SERVER_NAME'] is unset, easy enough to do, but I'm not sure it's necessary. Thoughts/opinions?

This ticket was mentioned in Slack in #core by travisnorthcutt. View the logs.


6 months ago

#54 follow-up: @riasat
6 months ago

WP v4.6.1

My nginx conf server block:

server {
	listen         80;
	server_name    ~^(?<subdomain>.+?)\.(?<domain>.+)$;
	root  /var/www/$subdomain;
	include conf.d/server_block_common.cnf;
}

This is what I always get:

Fatal error:  Uncaught phpmailerException: Invalid address: wordpress@~^(?&lt;subdomain&gt;.+?)\\.(?&lt;domain&gt;.+)$ in 

I know I can fix it by using "wp_mail_from" but I still don't get it why you can not simply default to "wordpress@…" or even just give an OPTION in settings to set the SEND FROM field. It is ridiculous that people have to resort to code if they want to change wordpress portion of the SEND FROM field.

#55 in reply to: ↑ 54 @dd32
6 months ago

Replying to riasat:

server {
	listen         80;
	server_name    ~^(?<subdomain>.+?)\.(?<domain>.+)$;
	root  /var/www/$subdomain;
	include conf.d/server_block_common.cnf;
}

This is what I always get:

Fatal error:  Uncaught phpmailerException: Invalid address: wordpress@~^(?&lt;subdomain&gt;.+?)\\.(?&lt;domain&gt;.+)$ in 

@riasat Just focusing on your error here, rather than the ticket - Looks like you've got a error in your nginx config there, although that should work for matching (and appears to be), the matched name isn't be passed to PHP, instead it's passing the literal regex through.
You need to correct the value being passed to PHP through fastcgi_param SERVER_NAME ... in your nginx config, something like http://serverfault.com/questions/650560/nginx-regex-vhost-pattern-ends-up-as-php-server-name should set your on your way.

#56 @derekakelly
6 months ago

I am also getting this bug, although it's only happening on my "add-on domain". My main website also uses the same configuration but does not have this error. I am on a Cloud VPS hosting account. just thought it was odd so I'd point it out. This is my cronjob command

/usr/local/php56/bin/php-cli /home/xxxxxx/public_html/xxxxxxxx.com/wp-cron.php

This is also a new error, up until a week or two ago, i'd never seen it before.

Thanks!

#57 @dd32
5 months ago

In 39655:

Mail: Ensure that any phpmailerException exceptions generated by setFrom() are caught to avoid PHP Fatal errors.

This change avoids a PHP fatal error that can be encountered when the specified (or generated) source email is an invalid address, such as wordpress@_, it makes no effort to set a valid source, only avoid the fatal error.

See #25239 for correcting the email address.
Fixes #39360.

#58 @swissspidy
2 months ago

#40259 was marked as a duplicate.

#59 @lilmike
2 months ago

Hi,
hJust a quick recap of the ticket I submitted this morning that was a duplicate:
I have a postfix that's set to send emails from certain domains (including my server's fqdn) through an email deliverability service (in the case of the server's fqdn to stop my server notifications to me -- crons, etc -- from being marked as junk or dropped entirely). However, when WordPress sends an email, the envelope-from gets set to 'http', and then, by postfix, to http@<fqdn>. This causes postfix to try and relay through the afore-mentioned provider... only to get a bounce because the from: is not a configured sending domain. This would be a helpful fix, even though I have a plugin currently installed and activated in the sites with the problem that cause it to actually set the right envelope-from.
All this rambling to say -- please fix, it's certainly been a while since this ticket was opened :-(.
-Michael.

#60 @tsimmons
3 weeks ago

This seems to have escalated due to the release of https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-8295 and https://exploitbox.io/vuln/WordPress-Exploit-4-7-Unauth-Password-Reset-0day-CVE-2017-8295.html.

Allowing user-supplied input without check or verification is a terrible thing.

For now I'm using @BjornW 's plugin https://wordpress.org/plugins/admin-email-as-from-address/#description to mitigate the exposure, but we really should be pulling this information from the site_url and an officially sanctioned patch should be issued.

@tsimmons
3 weeks ago

Replaces domain part with the typical email domain for a site, which includes the last domain part + TLD

@tsimmons
3 weeks ago

(Reuploading to use proper SVN diff) Replaces domain part with the typical email domain for a site, which includes the last domain part + TLD ... for example, if the site is blog.wordpress.org the email domain becomes wordpress.org

#61 follow-up: @kitchin
3 weeks ago

@tsimmons, Have not reviewed all this bug, but looks like you would be sending a lot of mail to wordpress@co.uk. Does WP use something like the public suffix list? https://publicsuffix.org/

Last edited 3 weeks ago by kitchin (previous) (diff)

This ticket was mentioned in Slack in #forums by otto42. View the logs.


3 weeks ago

#63 follow-up: @robert.peake
3 weeks ago

@tsimmons @kitchin Indeed, seems like it might be most backwards-compatible with the current solution to just do:

<?php
...
if ( !isset( $from_email ) ) {
    // Get the site domain and get rid of www.
    $sitename = strtolower( parse_url( get_home_url( get_current_blog_id() ), PHP_URL_HOST ) );
    if ( substr( $sitename, 0, 4 ) == 'www.' ) {
        $sitename = substr( $sitename, 4 );
    }

    $from_email = 'wordpress@' . $sitename;
...

This ticket was mentioned in Slack in #core by robertpeake. View the logs.


3 weeks ago

#65 in reply to: ↑ 63 @tsimmons
3 weeks ago

Replying to robert.peake:

@tsimmons @kitchin Indeed, seems like it might be most backwards-compatible with the current solution to just do:

<?php
...
if ( !isset( $from_email ) ) {
    // Get the site domain and get rid of www.
    $sitename = strtolower( parse_url( get_home_url( get_current_blog_id() ), PHP_URL_HOST ) );
    if ( substr( $sitename, 0, 4 ) == 'www.' ) {
        $sitename = substr( $sitename, 4 );
    }

    $from_email = 'wordpress@' . $sitename;
...

@kitchin Y'all are exactly right. There are a vast number of domain suffixes that are not taken into account with my patch. @robert.peake 's fix is superior; I think I'm going to stick with the plugin from @BjornW for now. Probably the only way to properly handle this is to utilize something like https://github.com/jeremykendall/php-domain-parser in the domain parsing.

#66 follow-up: @DavidAnderson
3 weeks ago

Why is WP trying to come up with an email address at all, rather than using the one in 'General Settings'? The plugin https://wordpress.org/plugins/admin-email-as-from-address/ has been mentioned - but why isn't this the existing behaviour?

#67 in reply to: ↑ 66 @tsimmons
3 weeks ago

Replying to DavidAnderson:

Why is WP trying to come up with an email address at all, rather than using the one in 'General Settings'? The plugin https://wordpress.org/plugins/admin-email-as-from-address/ has been mentioned - but why isn't this the existing behaviour?

My answer would be because system generated messages (like password resets or notifications) aren't really from the blog author, but from the backend. I use the aforementioned plugin and use wordpress@… for admin messages. Of course someone else may have another opinion.

#68 @cloudstek
3 weeks ago

I see I'm a bit late to the party lol, doh!

@tsimmons interesting thought about removing the subdomain(s) from the email domain, didn't think of that. Though one could argue it might be desired for some sites to include the subdomain in the email domain. I'm not that familiar with multisite but wouldn't that be one of the cases having the subdomain in the from header is actually desired?

With all the patches supplied, can I assume this will be fixed soon asap?

#69 @Ipstenu
3 weeks ago

The point @tsimmons brings up, about emails like password resets that shouldn't come from the admin, actually touches back on why this is considered a security issue. Admins shouldn't get the bounced email with the link to password reset.

In addition, some servers block emails sent FROM anyone but the domain WP is installed on. So your personal email of foobardancingfox@… wouldn't get the messages :( Oh and lets not think about the spoofing of emailing from gmail while actually not being from gmail. Fast track to being blacklisted :(

#70 follow-up: @DavidAnderson
3 weeks ago

@tsimmons I'm thinking, having read this ticket, that I wonder if I'm alone in thinking that it's not clear what this email address is *for*. Taking up your comment about the person/backend distinction... since one can't have an email conversation with a backend, since it isn't human, then presumably the idea is that it's effectively a "noreply@" address? i.e. An address that isn't really intended to get replies, and is just for aesthetics? (Of course, a site admin can set it up so that he *does* get replies (or he may have a catch-all) - but then he may as well just use the admin address anyway in that case).

@Ipstenu I don't follow all of this. A few functions in wp-includes/ms-functions.php already do use the admin email as a From: address (I'm wondering whether there's a design in which emails do/don't, or if it's just a accumulation of unplanned independent decisions). Also, it can't be dangerous for the site admin to see password reset requests, can it? He can already reset as many passwords as he likes, and/or set up a wordpress@ email address to the replies anyway (or may already have a catch-all).

Last edited 3 weeks ago by DavidAnderson (previous) (diff)

#71 follow-up: @Ipstenu
3 weeks ago

I don't know why we aren't consistent about email-fromness. I thought that we were fairly consistent in that if it's a message from the system (updates, password links etc) they came from wordpress@ across the board. A quick look at ms-functions and it appears emails that should have a 'contact' back (like 'you've got a new blog!' on multisite), where there's a reasonable expectation to know who mom is are sent from the network admin.

Also, it can't be dangerous for the site admin see password reset requests, can it? He can already reset as many passwords as he likes, and/or set up a wordpress@ email address to the replies anyway (or may already have a catch-all).

It's a higher risk. Remember, risk isn't a 1/0 switch. There are gradients. Most people don't make a wordpress@ email, or even a catch-all. But also most people don't use 2FA or good passwords on their email (see Google and Yahoo). It's possible for someone to snipe emails and get your passwords/resets without raising a red flag like "Hey, I (the admin) didn't ask to rent my password..."

In short, it's not dangerous for the admin to have your password. It's dangerous for the uneducated and non-security conscious admin to clear-text read your bounced messages in a coffee shop :/ (Plus I bet the bounces would confuse a lot of people...)

Email's not very secure, is basically my point :) Or rather, people USING email aren't very secure in general, so if using a generic wordpress@ will protect more people at minimal cost, then we probably should do that.

I do find it interesting we have $admin_email = 'support@' . $_SERVER['SERVER_NAME']; in there as a fallback if there's no admin.

#72 in reply to: ↑ 71 ; follow-up: @cloudstek
3 weeks ago

Replying to Ipstenu:

I don't know why we aren't consistent about email-fromness. I thought that we were fairly consistent in that if it's a message from the system (updates, password links etc) they came from wordpress@ across the board. A quick look at ms-functions and it appears emails that should have a 'contact' back (like 'you've got a new blog!' on multisite), where there's a reasonable expectation to know who mom is are sent from the network admin.

Also, it can't be dangerous for the site admin see password reset requests, can it? He can already reset as many passwords as he likes, and/or set up a wordpress@ email address to the replies anyway (or may already have a catch-all).

It's a higher risk. Remember, risk isn't a 1/0 switch. There are gradients. Most people don't make a wordpress@ email, or even a catch-all. But also most people don't use 2FA or good passwords on their email (see Google and Yahoo). It's possible for someone to snipe emails and get your passwords/resets without raising a red flag like "Hey, I (the admin) didn't ask to rent my password..."

In short, it's not dangerous for the admin to have your password. It's dangerous for the uneducated and non-security conscious admin to clear-text read your bounced messages in a coffee shop :/ (Plus I bet the bounces would confuse a lot of people...)

Email's not very secure, is basically my point :) Or rather, people USING email aren't very secure in general, so if using a generic wordpress@ will protect more people at minimal cost, then we probably should do that.

I do find it interesting we have $admin_email = 'support@' . $_SERVER['SERVER_NAME']; in there as a fallback if there's no admin.

One reason to send email from the admin user instead of a non-existing wordpress@ email is that it will likely fail sender verification when it's enabled on the receiving server and thus be rejected and not delivered.

Also to prevent people from replying to it you can set the Reply-To header to noreply@ or if you like wordpress@. Though I find it more likely people won't create a noreply@ address than a wordpress@ address. Therefore I'd prefer the use of noreply@ as it makes clear you're not supposed to reply. We wouldn't need wordpress@ for aesthetic purposes as the Reply-To field is likely to be hidden by default in most email clients.

Preventing bounces is a little harder but according to Wikipedia there are a number of headers that can be sent in order to direct bounce emails to a specific (non-existing) address: https://en.wikipedia.org/wiki/Bounce_address.

#73 in reply to: ↑ 72 @DavidAnderson
3 weeks ago

@cloudstek @Ipstenu If the intention of the "wordpress@" address is that replies are meant to be as hard/invisible as possible, then there might be a case for a no-reply@… address that is a log-less black-hole (and people who don't trust wordpress.org (which makes no sense, unless they also turn off all manner of other things) can use a plugin or filter to set their own). The number of people with catch-alls must be a decent number. So, if this is the case, I'd say that the logical conclusion is a universally-the-same address like this.

@cloudstek Sender verification uses the SMTP envelope From, rather than the header From:. i.e. WordPress has been relying on these two being different (whether intentionally or not) to get these emails through sender validity checks. Which is to say, though, that it's basically just ornamental, and we could use "bob-the-giant@…" (that's why, despite using an address that often doesn't exist, they still get through - though, this is just the norm - I have no stats on how many mail servers will also verify the header From: as well as the SMTP envelope From). That point is also relevant to @Ipstenu's comment about email delivery restrictions on some servers - these are normally looking at the SMTP envelope. i.e. To the extent that the admin address can't be used because of server restrictions, to the same extent a wordpress@ address can't be used because of sender verification.

#74 in reply to: ↑ 61 @tsimmons
3 weeks ago

Replying to kitchin:

@tsimmons, Have not reviewed all this bug, but looks like you would be sending a lot of mail to wordpress@co.uk. Does WP use something like the public suffix list? https://publicsuffix.org/

I don't think so, and probably really shouldn't. It really should just be a configurable option (from address for admin messages) which the plugin provides. Automatically deriving from user supplied input DIRECTLY is bad, though, as the present code does.

#75 in reply to: ↑ 70 ; follow-up: @tsimmons
3 weeks ago

Replying to DavidAnderson:

since it isn't human, then presumably the idea is that it's effectively a "noreply@" address?

Exactly. it should be left up to the user to define what address they want system messages to come from, a simple configuration option, that defaults to the admin's address if omitted, IMO.

#76 in reply to: ↑ 75 ; follow-up: @DavidAnderson
3 weeks ago

Replying to tsimmons:

I agree with you. The default WP behaviour currently is to invent a new email address with an associated pseudo-persona for the site, without the site or domain owner's involvement. I can't see what the up-sides to that are.

#77 in reply to: ↑ 76 @tsimmons
3 weeks ago

Replying to DavidAnderson:

Replying to tsimmons:

I agree with you. The default WP behaviour currently is to invent a new email address with an associated pseudo-persona for the site, without the site or domain owner's involvement. I can't see what the up-sides to that are.

Yup. The security paranoid (I am one) should implement the plugin above until core is patched. The patch I supplied should not be used because it does a poor job of detecting TLD/public suffixes.

Last edited 3 weeks ago by tsimmons (previous) (diff)

#78 follow-up: @kitchin
3 weeks ago

Empty return-path is the age-old way of preventing bounces. Bounces themselves set an empty return-path, as do "out of the office" auto-responders. Return-path is part of the SMTP envelope, unlike message headers like return-to. Sorry if this information is bug spam for most people, but maybe it's not.

We could both try to stop bounces AND use the owner email address, which we can assume has been affirmatively set up during install. It won't catch everything, due to the variety of setups, pluggables, filters, etc. And there might be legacy blowback. Imagine someone has written a filter that detects wordpress@! Too bad.

Could send a security email advisory to the owner address during the upgrade.

#79 in reply to: ↑ 78 @tsimmons
3 weeks ago

Replying to kitchin:

Empty return-path is the age-old way of preventing bounces. Bounces themselves set an empty return-path, as do "out of the office" auto-responders. Return-path is part of the SMTP envelope, unlike message headers like return-to. Sorry if this information is bug spam for most people, but maybe it's not.

We could both try to stop bounces AND use the owner email address, which we can assume has been affirmatively set up during install. It won't catch everything, due to the variety of setups, pluggables, filters, etc. And there might be legacy blowback. Imagine someone has written a filter that detects wordpress@! Too bad.

Could send a security email advisory to the owner address during the upgrade.

Personally I'd prefer an option to set the from-address, so a user could use their own email or a null-routed noreply@ if desired. Or even a user-supplied account that can receive replies. It's up to the user to choose, just use reasonable defaults for the less tech-savvy.

#80 @Grzegorz.Janoszka
3 weeks ago

@DavidAnderson: There are servers on internet that verify the envelope-from. I used to send emails from non-existing wordpress@…, but after a few servers that refused to accept emails, I changed to an existing email (actually the admin email).

This ticket was mentioned in Slack in #slackhelp by pento. View the logs.


2 weeks ago

#82 follow-up: @pigdog234
2 weeks ago

Hi,

Seems like a very short term way to address the CVE is to just drop something like the following in:

add_filter( 'wp_mail_from', function( $email ) {
     return 'wordpress@example.com';
});

Right?

#83 in reply to: ↑ 82 @tsimmons
2 weeks ago

Replying to pigdog234:

Hi,

Seems like a very short term way to address the CVE is to just drop something like the following in:

add_filter( 'wp_mail_from', function( $email ) {
     return 'wordpress@example.com';
});

Right?

The problem with this approach is it doesn't work with a multi-domain site where the from address should be domain-specific.

#84 @pigdog234
2 weeks ago

Indeed. And so in a multidomain environment it seems to me that exposing $sitename for this purpose as a configuration element is useful.

BTW, it might be possible to test an outbound email by creating a little workflow along the following lines:

  1. User enters the address
  2. Address set to unverified (do not use yet)
  3. Email is sent to a drop box on some picky email gateway like Gmail
  4. Local installation sets an event to check validation with WP.com (or some service)
  5. WP.com (or some service) has a little process that pulls email from the box and validates DKIM/SPF and that the message got through
  6. Local installation polls WP to validate the address.

It can't happen all at once because of grey listing effects (that's a fair amount of work). It also may be more trouble than it's worth. At the end of the day it would be nice if notifications happened in a WP app.

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


2 weeks ago

#86 @pigdog234
2 weeks ago

Just for clarity, whose responsibility is it to police SERVER_NAME in a multi-domain configuration? Should it be Apache's or Wordpress'? Who knows what the valid values should be?

This ticket was mentioned in Slack in #core by clorith. View the logs.


13 days ago

#88 @BjornW
8 days ago

Maybe I've missed something in this thread, but what are the arguments against having an option for setting the email address with a sensible default?

#89 @rawrly
5 days ago

Because the ticket and this case is a bit old, and has begun to take on a lots of side-issues related to the same bit of code I wanted to summarize the issue(s) at hand and repeat the currently open questions and solutions provided i a concise manner for those just catching up:

Original ticket's focus:

Not always does $_SERVER['SERVER_NAME'] get passed down to PHP from the web-server environment. This causes emails to be fail to send due to invalid email address From address fields.

This affects some nginx server configurations as well as wp-cli and sometimes via Cron tasks. Leading to emails being unable to be sent when they are related to the function of the request.

The later reported vulnerability:

A pseudo-vulnerability was released related to this portion of WP core code in early 2017, where in specific and unlikely server configurations this could be used to intercept password reset tokens.

Where this bug stands is two parts:

There is an inconvenient choice for the FQDN after @ when generating the From address in emails, where in some situations it will cause emails to not be sent and the application to not function correctly.

There is an unlikely but possible vulnerability also related to this same portion of code, and the fact it accepts browser input (the value from the Host header field) for the value of the email address to send emails From.

Proposed Patches summary:

Most patches have been focused on how to get a FQDN from somewhere other that $_SERVER['SERVER_NAME']

"server_name.diff" does a replacement of all $_SERVER['SERVER_NAME'] to now use $current_site->domain

"25239.patch" creates a function get_email_domain() which uses preg_replace to extract the domain name from get_home_url( get_current_blog_id() ) -- there has been comments related to how this is an incomplete fix due to limitations of the preg_replace regex.

"pluggable.php" is similar to 25239.patch

"CVE-2017-8295.patch" uses parse_url() to extract the domain from network_home_url()

Recommendations that lack patches:

Creating a new wp_option value for the site's maintenance email address, which would be used in the From field for these sort of emails

---

There are still two open questions related to:

"Whose responsibility is it to police SERVER_NAME in a multi-domain configuration? "

"What are the arguments against having an option for setting the email address with a sensible default?"
Which have gone answered and I don't want to quash these questions with the summary.

So please, chime in if you feel a solution which patches may be a good fix ASAP (since security) and which are more complete fixes (or if you have another proposal please add it here).

---

IMHO I feel the wp_options value proposal is a more complete fix, but requires a more work and testing (and requires a fall-back in case this value of this option is NULL). While the CVE-2017-8295.patch fits the bill for a sufficient "just works" patch if someone wants to kabash that security issue and these bugs for the short term and give more time for a more complete fix to be tested, verified and implemented.

Note: See TracTickets for help on using tickets.