Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:97599
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain ohgaki.net designates 180.42.98.130 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CAGa2bXbgPpO4CfHyHrDDhbXsysk7HDsc17nQoGtWNv4zdaq5kA@mail.gmail.com>
References: <CAGa2bXbgPpO4CfHyHrDDhbXsysk7HDsc17nQoGtWNv4zdaq5kA@mail.gmail.com>
Date: Mon, 9 Jan 2017 07:19:22 +0900
Message-ID: <CAGa2bXZuKmGJu2bnnoqi1DjnyvBL=9VLM=jgffXFfoWf4SJk3Q@mail.gmail.com>
To: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=f403045f24e0093a5405459ca785
Subject: Re: Improving mail() 5th parameter handling
From: yohgaki@ohgaki.net (Yasuo Ohgaki)

--f403045f24e0093a5405459ca785
Content-Type: text/plain; charset=UTF-8

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

--f403045f24e0093a5405459ca785--