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
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?
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
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?
If there isn't any preference, I would like to write RFC for removing
'addtional_parameters' option frommail()/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
Hi Nikita and all,
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
it will help in most cases
I meant "arrayed 'addtional_parameters'" will help.
--
Yasuo Ohgaki
yohgaki@ohgaki.net
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
Hi Nikita and all,
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
Hi Nikita and all,
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
Hi Nikita and all,
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.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
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
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
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
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
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
Hello,
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.