Welcome! Log In Create A New Profile

Advanced

[PHP-DEV] Improving mail() 5th parameter handling

Posted by Yasuo Ohgaki 
Yasuo Ohgaki
[PHP-DEV] Improving mail() 5th parameter handling
January 07, 2017 11:10PM
Hi all,

All of us knew details of PHPMailer and Swift Mailer issues with mail()'s
5th (additional_parameters) parameter by now, I suppose. Current behavior
(applying php_escape_shell_cmd to addtional_parameters) is not nice and
similar issue may raise with addtional_parameters in the future.

The issue could be mitigated by allowing array addtional_parameter. It's
basically the same as 4th (addtional_header) parameter change which is
committed by me.

- Allow array additional_parameter and soft deprecate (document
deprecation) string one.
- Use key as "option name" and validate chars
- Use value as "option value" and validate some control chars then apply
escapeshellarg()

Since we cannot assume which shell to be used with sendmail command/how
sendmail command is invoked, this is not complete solution. (This includes
php.ini option setting, i.e. sendmail_path and mail.force_extra_parameters)
This is a mitigation, but it seems we are better to have this to protect
PHP systems.

Any comment for this change?
Or better, is anyone working on this?

Removing 5th option may be good idea also. The most severe BC impact would
be SMTP authentication. If users need SMTP authentication (or any other
options) with sendmail command, mail.force_extra_parameters/sendmail_path
ini setting may be used.

We cannot remove parameter suddenly. We may document deprecation now, raise
warning with 7.2, remove it by 7.3 or 8.0.

Are there comments for removing 5th option?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net
Yasuo Ohgaki
[PHP-DEV] Re: Improving mail() 5th parameter handling
January 08, 2017 11:30PM
Hi all,

On Sun, Jan 8, 2017 at 6:57 AM, Yasuo Ohgaki <[email protected]> wrote:

> All of us knew details of PHPMailer and Swift Mailer issues with mail()'s
> 5th (additional_parameters) parameter by now, I suppose. Current behavior
> (applying php_escape_shell_cmd to addtional_parameters) is not nice and
> similar issue may raise with addtional_parameters in the future.
>
> The issue could be mitigated by allowing array addtional_parameter. It's
> basically the same as 4th (addtional_header) parameter change which is
> committed by me.
>
> - Allow array additional_parameter and soft deprecate (document
> deprecation) string one.
> - Use key as "option name" and validate chars
> - Use value as "option value" and validate some control chars then apply
> escapeshellarg()
>
> Since we cannot assume which shell to be used with sendmail command/how
> sendmail command is invoked, this is not complete solution. (This includes
> php.ini option setting, i.e. sendmail_path and mail.force_extra_parameters)
> This is a mitigation, but it seems we are better to have this to protect
> PHP systems.
>
> Any comment for this change?
> Or better, is anyone working on this?
>
> Removing 5th option may be good idea also. The most severe BC impact would
> be SMTP authentication. If users need SMTP authentication (or any other
> options) with sendmail command, mail.force_extra_parameters/sendmail_path
> ini setting may be used.
>
> We cannot remove parameter suddenly. We may document deprecation now,
> raise warning with 7.2, remove it by 7.3 or 8.0.
>
> Are there comments for removing 5th option?
>

If there isn't any preference, I would like to write RFC for removing
'addtional_parameters' option from mail()/mb_send_mail(). Command
injections are still possible with INI settings. Users will notice risks by
additional comments in php.ini-{production,development} and the manual when
we remove 'addtional_parameters' option, hopefully.

If anyone would like to keep mail()'s 'addtional_parameters' (5th) option,
please let me know now.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net
Nikita Popov
Re: [PHP-DEV] Re: Improving mail() 5th parameter handling
January 08, 2017 11:40PM
On Sun, Jan 8, 2017 at 11:19 PM, Yasuo Ohgaki <[email protected]> wrote:

> Hi all,
>
> On Sun, Jan 8, 2017 at 6:57 AM, Yasuo Ohgaki <[email protected]> wrote:
>
> > All of us knew details of PHPMailer and Swift Mailer issues with mail()'s
> > 5th (additional_parameters) parameter by now, I suppose. Current behavior
> > (applying php_escape_shell_cmd to addtional_parameters) is not nice and
> > similar issue may raise with addtional_parameters in the future.
> >
> > The issue could be mitigated by allowing array addtional_parameter. It's
> > basically the same as 4th (addtional_header) parameter change which is
> > committed by me.
> >
> > - Allow array additional_parameter and soft deprecate (document
> > deprecation) string one.
> > - Use key as "option name" and validate chars
> > - Use value as "option value" and validate some control chars then apply
> > escapeshellarg()
> >
> > Since we cannot assume which shell to be used with sendmail command/how
> > sendmail command is invoked, this is not complete solution. (This
> includes
> > php.ini option setting, i.e. sendmail_path and
> mail.force_extra_parameters)
> > This is a mitigation, but it seems we are better to have this to protect
> > PHP systems.
> >
> > Any comment for this change?
> > Or better, is anyone working on this?
> >
> > Removing 5th option may be good idea also. The most severe BC impact
> would
> > be SMTP authentication. If users need SMTP authentication (or any other
> > options) with sendmail command, mail.force_extra_parameters/
> sendmail_path
> > ini setting may be used.
> >
> > We cannot remove parameter suddenly. We may document deprecation now,
> > raise warning with 7.2, remove it by 7.3 or 8.0.
> >
> > Are there comments for removing 5th option?
> >
>
> If there isn't any preference, I would like to write RFC for removing
> 'addtional_parameters' option from mail()/mb_send_mail(). Command
> injections are still possible with INI settings. Users will notice risks by
> additional comments in php.ini-{production,development} and the manual
> when
> we remove 'addtional_parameters' option, hopefully.
>
> If anyone would like to keep mail()'s 'addtional_parameters' (5th) option,
> please let me know now.
>

Without this option, how do you specify the envelope sender? That seems to
be the primary use-case.

Nikita
Yasuo Ohgaki
Re: [PHP-DEV] Re: Improving mail() 5th parameter handling
January 09, 2017 12:00AM
Hi Nikita and all,

On Mon, Jan 9, 2017 at 7:31 AM, Nikita Popov <[email protected]> wrote:

> Without this option, how do you specify the envelope sender? That seems to
> be the primary use-case.


Indeed, it seems it is.
It could be set by mail.force_extra_parameters. I agree this isn't a great
way to do, but the obstacle may help users to notice risks.

Parameters must be validated still, but it will help in most cases and I
don't mind writing patch for arrayed 'addtional_parameter'. In this case,
I'll just fix this as normal bug fix and post proposed patch before commit.
Any comments on this?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net
Yasuo Ohgaki
Re: [PHP-DEV] Re: Improving mail() 5th parameter handling
January 09, 2017 12:30AM
On Mon, Jan 9, 2017 at 7:56 AM, Yasuo Ohgaki <[email protected]> wrote:

> it will help in most cases


I meant "arrayed 'addtional_parameters'" will help.

--
Yasuo Ohgaki
yohgaki@ohgaki.net
Stanislav Malyshev
Re: [PHP-DEV] Improving mail() 5th parameter handling
January 16, 2017 10:10PM
Hi!

> - Allow array additional_parameter and soft deprecate (document
> deprecation) string one.
> - Use key as "option name" and validate chars
> - Use value as "option value" and validate some control chars then apply
> escapeshellarg()

Making array sounds good, but I'm not sure what is meant by "option
name" here - CLI arguments don't have names?
--
Stas Malyshev
smalyshev@gmail.com

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Yasuo Ohgaki
Re: [PHP-DEV] Improving mail() 5th parameter handling
January 17, 2017 10:30AM
Hi Stas,

On Tue, Jan 17, 2017 at 6:02 AM, Stanislav Malyshev <[email protected]>
wrote:

> > - Allow array additional_parameter and soft deprecate (document
> > deprecation) string one.
> > - Use key as "option name" and validate chars
> > - Use value as "option value" and validate some control chars then apply
> > escapeshellarg()
>
> Making array sounds good, but I'm not sure what is meant by "option
> name" here - CLI arguments don't have names?


I'm planning to use extra_parameters array like

$opts = [
'-f' => [email protected]', // Envelope sender
'-4' => null, // Force IPv4
'-au' => [email protected]', // SMTP auth user
'-ap' => 'secret', // SMTP auth password
'-am' => 'CRAM-MD5', // SMTP auth method
];

To all,

I cannot reseach all kinds of sendmail binaries. If there are exotic
sendmail binaries,
I would like to know the reference for them. Thank you.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net
Yasuo Ohgaki
Re: [PHP-DEV] Improving mail() 5th parameter handling
January 17, 2017 11:30AM
On Tue, Jan 17, 2017 at 6:20 PM, Yasuo Ohgaki <[email protected]> wrote:

> I'm planning to use extra_parameters array like
>
> $opts = [
> '-f' => [email protected]', // Envelope sender
> '-4' => null, // Force IPv4
> '-au' => [email protected]', // SMTP auth user
> '-ap' => 'secret', // SMTP auth password
> '-am' => 'CRAM-MD5', // SMTP auth method
> ];
>
> To all,
>
> I cannot reseach all kinds of sendmail binaries. If there are exotic
> sendmail binaries,
> I would like to know the reference for them. Thank you.
>

The command line would look something like

$ sendmail -F 'Some User' -f [email protected]' -4 -au [email protected]'
-ap 'secret' -am 'CRAM-MD5' to@example.com < msg.txt
// I added -F (Fullname) option to illustrate we need ' '(space) for option
values.

Option values are escaped by php_escape_shell_arg().
Thank you for feedback.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net
Stanislav Malyshev
Re: [PHP-DEV] Improving mail() 5th parameter handling
January 17, 2017 05:30PM
Hi!

> I'm planning to use extra_parameters array like
>
> $opts = [
> '-f' => [email protected] <mailto:[email protected]>', // Envelope sender
> '-4' => null, // Force IPv4
> '-au' => [email protected] <mailto:[email protected]>', // SMTP auth user
> '-ap' => 'secret', // SMTP auth password
> '-am' => 'CRAM-MD5', // SMTP auth method
> ];

I don't think it is a good idea. Starting options with "-" is indeed a
tradition, but only a tradition, and nothing prevents a tool from having
different way of accepting options, including long format (--option) or
Windows format (/option) or any other way. Hard-coupling it with
specific sendmail options does not seem a good idea to me. It's better
to tread each option as a full string than trying to parse it and
introduce hard dependencies on specific binary's internals.

> I cannot reseach all kinds of sendmail binaries. If there are exotic
> sendmail binaries,
> I would like to know the reference for them. Thank you.

I don't think it is a good idea to specialize for specific binaries.

--
Stas Malyshev
smalyshev@gmail.com

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Yasuo Ohgaki
Re: [PHP-DEV] Improving mail() 5th parameter handling
January 18, 2017 01:30AM
Hi Stas,

On Wed, Jan 18, 2017 at 1:26 AM, Stanislav Malyshev <[email protected]>
wrote:

>
> > I cannot reseach all kinds of sendmail binaries. If there are exotic
> > sendmail binaries,
> > I would like to know the reference for them. Thank you.
>
> I don't think it is a good idea to specialize for specific binaries.


This is what I thought, too.

"sendmail" binary should be compatible with "sendmail", but there may be
binaries aren't compatible sendmail style options. Stricter validation
provides
better security while there is compatibility risk.

We cannot specify sendmail binary nor shell, i.e. cannot make sure how it
works
and there is chance for security and compatibility risk. I prefer stricter
validation
for better security.

However, it could be somewhere between.

- Allow only alpha numeric + '-' + '_' + '/' (Only under Windows) for
option names

What do you think?

If anyone know more chars should be allowed, please comment.
e.g. XYZ sendmail requires "sendmail -f='sender'" style.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net
Sorry, only registered users may post in this forum.

Click here to login