Everything

php

Home » Project » Newsgroups » php.internals » Improving mail() 5th parameter handling »

Improving mail() 5th parameter handling

php.internals

Yasuo Ohgaki

9 years ago

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

9 years ago

Hi all,

On Sun, Jan 8, 2017 at 6:57 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> 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

9 years ago

On Sun, Jan 8, 2017 at 11:19 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> Hi all,
>
> On Sun, Jan 8, 2017 at 6:57 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> 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

9 years ago

Hi Nikita and all,

On Mon, Jan 9, 2017 at 7:31 AM, Nikita Popov <nikita.ppv@gmail.com> 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

9 years ago

On Mon, Jan 9, 2017 at 7:56 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> it will help in most cases

I meant "arrayed 'addtional_parameters'" will help.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

OISHI Kazuo

9 years ago

Hi,

Yasuo Ohgaki <yohgaki@ohgaki.net> writes:

>> 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.

It is very important for mail sending API to be able to change envelope
sender PER mail. For example, this is used to set Return-Path for error
handling like;
Return-Path: <internals-return-97601-kazuo=o-ishi.jp@lists.php.net>

mail.force_extra_parameters in php.ini is not enough.

--
Kazuo Oishi

Nikita Popov

9 years ago

On Sun, Jan 8, 2017 at 11:56 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> Hi Nikita and all,
>
> On Mon, Jan 9, 2017 at 7:31 AM, Nikita Popov <nikita.ppv@gmail.com> 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?
>

Allowing an array for additional_parameter sounds reasonable. Before
committing a patch, please lets make sure that people from phpmailer and
other people familiar with the recent exploits verify it.

Nikita

Christoph Becker

9 years ago

On 09.01.2017 at 17:23, Nikita Popov wrote:

> On Sun, Jan 8, 2017 at 11:56 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
>
>> Hi Nikita and all,
>>
>> On Mon, Jan 9, 2017 at 7:31 AM, Nikita Popov <nikita.ppv@gmail.com> 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?
>
> Allowing an array for additional_parameter sounds reasonable. Before
> committing a patch, please lets make sure that people from phpmailer and
> other people familiar with the recent exploits verify it.

See bug #73842.

--
Christoph M. Becker

Mike Willbanks

9 years ago

On Sun, Jan 8, 2017 at 4:56 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> Hi Nikita and all,
>
> On Mon, Jan 9, 2017 at 7:31 AM, Nikita Popov <nikita.ppv@gmail.com> 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?
>

Removing the 5th parameter would be a serious mistake and limitation IMO.
Moving the additional_headers and additional_parameters to be an array of
parameters would resolve most of the main issues I see here but forcing an
INI setting here would be a mistake, huge BC break and overall not
something you could fix as a "bug fix" in this case in particular.

Stas Malyshev

9 years ago

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

Yasuo Ohgaki

9 years ago

Hi Stas,

On Tue, Jan 17, 2017 at 6:02 AM, Stanislav Malyshev <smalyshev@gmail.com>
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' => 'some@example.com',  // Envelope sender
'-4' => null, // Force IPv4
'-au' => 'user@example.com', // 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

9 years ago

On Tue, Jan 17, 2017 at 6:20 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> I'm planning to use extra_parameters array like
>
> $opts = [
>   '-f' => 'some@example.com',  // Envelope sender
>   '-4' => null, // Force IPv4
>   '-au' => 'user@example.com', // 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 'some@example.com' -4 -au 'user@example.com'
-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

Stas Malyshev

9 years ago

Hi!

> I'm planning to use extra_parameters array like
>
> $opts = [
>   '-f' => 'some@example.com <mailto:some@example.com>',  // Envelope sender
>   '-4' => null, // Force IPv4
>   '-au' => 'user@example.com <mailto:user@example.com>', // 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

Yasuo Ohgaki

9 years ago

Hi Stas,

On Wed, Jan 18, 2017 at 1:26 AM, Stanislav Malyshev <smalyshev@gmail.com>
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

Andrey Andreev

9 years ago

Hello,

On Wed, Jan 18, 2017 at 2:20 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

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

Here's a gist containing a lot of information about the issue:
https://gist.github.com/Zenexer/40d02da5e07f151adeaeeaa11af9ab36

Cheers,
Andrey.