Error with sendmail: "Potential code injection in From header"

[topicode]

With the latest Version 1.22.0 of zf1-future I get the following error when using Sendmail:

Uncaught Zend_Mail_Transport_Exception: Potential code injection in From header in /var/www/project/vendor/shardj/zf1-future/library/Zend/Mail/Transport/Sendmail.php:125
Stack trace:
#0 /var/www/project/vendor/shardj/zf1-future/library/Zend/Mail/Transport/Abstract.php(348): Zend_Mail_Transport_Sendmail->_sendMail()
#1 /var/www/project/vendor/shardj/zf1-future/library/Zend/Mail.php(1201): Zend_Mail_Transport_Abstract->send()
#2 /var/www/project/library/Wlwp/Mail.php(580): Zend_Mail->send()
#3 /var/www/project/cronjobs/processScheduler.php(419): Wlwp_Mail->send()
#4 /var/www/project/cronjobs/processScheduler.php(66): TaskScheduler->_sendCronjobErrorMail()
#5 /var/www/project/cronjobs/processScheduler.php(8): TaskScheduler->process()
#6 {main}
  thrown in /var/www/project/vendor/shardj/zf1-future/library/Zend/Mail/Transport/Sendmail.php on line 125

The problem is the added validation of $this->parameters:

zf1-future/library/Zend/Mail/Transport/Sendmail.php

Lines 122 to 135 in 6545737

	$fromEmailHeader = str_replace(' ', '', $this->parameters);
	// Sanitize the From header
	if (!Zend_Validate::is($fromEmailHeader, 'EmailAddress')) {
	throw new Zend_Mail_Transport_Exception('Potential code injection in From header');
	} else {
	set_error_handler([$this, '_handleMailErrors']);
	$result = mail(
	$this->recipients,
	$this->_mail->getSubject(),
	$this->body,
	$this->header,
	$fromEmailHeader);
	restore_error_handler();
	}

According to the documentation of the mail()-function, the fifth parameter is for additional parameters to the sendmail program. So, while it is entirely possible that this string contains an email address, it is definitely not the only valid value, and I propose to remove this validation (and the replacement of spaces).

[hungtrinh]

This change come from here #295 (comment)

Mr @sreichel could you please review this issue?

[sreichel]

Found some info why it has been added ... https://scandiweb.com/blog/surprise-emails-that-can-execute-mailing-solution-vulnerability/

@topicode can you add some details about the params you pass to mail()?

[topicode]

We configure our application via an application.ini file, so the relevant settings are:

resources.mail.transport.type = Zend_Mail_Transport_Sendmail

That's it. This creates the sendmail transport from the config, by passing an (empty) options array to the transport:

zf1-future/library/Zend/Application/Resource/Mail.php

Line 145 in c098449

$transport = new $transportName($options);

This leads to an empty string in $this->parameters, which in turn does (of course) not qualify as a valid email address.

One possible general solution/workaround I see here would be the usage of escapeshellarg() - but that might be a problem when $parameters is passed as string instead of as array/Zend_Config, and might produce other problems. Additionally I am not sure about Windows compatibility here (if that is a thing).

In any case, a check for empty $this->parameters should be added (or the === null could be changed to empty()), and that would suffice for our use case here, but other use cases with more parameters (if that is ever applicable) would still break.

[develart-projects]

I just faced the problem myself after ZF1 Future migration, so going to find some solution.

[develart-projects]

@topicode parameters are always string beyond the constructor. Now looking into that and the whole email validation seems quite BS to me, as it's by default expected, that parameters = from email. And that's not the case.

[develart-projects]

Addressed by #366
Still up to discussion if we are going to somehow extend the validation.

[yhabteab]

Hi, we also encountered the same problem when trying to set the envelope sender using the additional parameters here. The already merged quick fix from @develart-projects, won't work for our case either, since we don't pass an empty additional params.

Thanks

[develart-projects]

@yhabteab that's actually what I was afraid about. Question is how to address that. I'll release the quickfix, so will partially work. But will keep this one open for further development.

[develart-projects]

Adding some more resources, trying to figure out what to do here.

https://framework.zend.com/security/advisory/ZF2016-04
https://scandiweb.com/blog/surprise-emails-that-can-execute-mailing-solution-vulnerability/
https://nvd.nist.gov/vuln/detail/CVE-2016-10034

[develart-projects]

@topicode seems, like 5th param of PHP mail() is already sanitized that way. From the PHP manual:

`
additional_params (optional)

The additional_params parameter can be used to pass additional flags as command line options to the program configured to be used when sending mail, as defined by the sendmail_path configuration setting. For example, this can be used to set the envelope sender address when using sendmail with the -f sendmail option.

This parameter is escaped by [escapeshellcmd()](https://www.php.net/manual/en/function.escapeshellcmd.php) internally to prevent command execution. [escapeshellcmd()](https://www.php.net/manual/en/function.escapeshellcmd.php) prevents command execution, but allows to add additional parameters. For security reasons, it is recommended for the user to sanitize this parameter to avoid adding unwanted parameters to the shell command.

Since [escapeshellcmd()](https://www.php.net/manual/en/function.escapeshellcmd.php) is applied automatically, some characters that are allowed as email addresses by internet RFCs cannot be used. mail() can not allow such characters, so in programs where the use of such characters is required, alternative means of sending emails (such as using a framework or a library) is recommended.

The user that the webserver runs as should be added as a trusted user to the sendmail configuration to prevent a 'X-Warning' header from being added to the message when the envelope sender (-f) is set using this method. For sendmail users, this file is /etc/mail/trusted-users.

`

[develart-projects]

Option 1: ZF2 solution - double quotes validation

if (preg_match('/\\\"/', $address->getEmail())) {
            throw new Exception\RuntimeException("Potential code injection in From header");
        }

[develart-projects]

Option 2: If I got all correctly, we are looking for 3 double-quotes in the string, so this can be solution as well. As we are aiming to cover -f possibility, we can go for combined validation:

1. Do not validate, if empty
2. validate by Zend Mail validator, if option -f is not present
3. validate with more-than-2 double-quotes, if -f is used (or just for quotes, as ZF2, optionally)

By this, we are still covering most cases as they are now, but allowing to run it properly for the "-f" users as well.

Opening discussion before implementing anything. But not the long one, as I would like to close this one.

[develart-projects]

No one has any clues on this? If not, I'll implement option 2.

[develart-projects]

#371

[develart-projects]

@yhabteab released in 1.23.3, pls reopen if you still have problem with that

[yhabteab]

Hii @develart-projects thanks for your effort, but it still doesn't work for us.

zf1-future/library/Zend/Mail/Transport/Sendmail.php

Lines 146 to 150 in 622a60b

	if( substr( $fromEmailHeader, 0, 2 ) === '-f' && substr_count($fromEmailHeader, '"') >2 ) { // we are considering just usage of double-quotes
	throw new Zend_Mail_Transport_Exception('Potential code injection in From header');
	} elseif( Zend_Validate::is($fromEmailHeader, 'EmailAddress') === FALSE ) { // full email validation

The last elseif branch still tries to Zend mail validate the additional parameters while the -f option is set.

PS: I am also unable to reopen this issue :)

[develart-projects]

@yhabteab what is the exact string you are passing, pls? Or maybe you can adjust first if to match your string (?)

[develart-projects]

@yhabteab ok, I see where the problem is. I think I'll stop to do fixes in a rush :) Anyway, please send me the exact string you are using, just for sure.

[yhabteab]

@yhabteab what is the exact string you are passing, pls?

We're passing the params to Sendmail constructor like this:

new Zend_Mail_Transport_Sendmail('-f ' . escapeshellarg($this->getFrom()));

and the resulting string looks like this: "-f 'reporting@icinga'"

[develart-projects]

This will fix it: #374

[develart-projects]

@yhabteab pls validate 1.23.5, before I close it

[yhabteab]

It Works! Thanks!