Skip to content

Add blacklisted command line flags for sendline via mail() #12783

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Add blacklisted command line flags for sendline via mail() #12783

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1d26b7a
Added mail.allow_additional_sendmail_flags php.ini-development
TCGeek Nov 25, 2023
b4a57b9
Added mail.allow_additional_sendmail_flags php.ini-production
TCGeek Nov 25, 2023
440b2ca
Added blacklist functionality mail.c
TCGeek Nov 25, 2023
fae7507
Declared php_mail_disable_flags php_mail.h
TCGeek Nov 25, 2023
ec0cd1c
Added bool for mail.allow_additional_sendmail_flags main.c
TCGeek Nov 25, 2023
7d7eff5
Added globals for allow_additional_sendmail_flags php_globals.h
TCGeek Nov 25, 2023
ef0b0ef
Create allowed test mail_blacklist_allowed.phpt
TCGeek Nov 25, 2023
10b4625
Create disallow test mail_blacklist_disallowed.phpt
TCGeek Nov 25, 2023
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions ext/standard/mail.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,28 @@ PHPAPI zend_string *php_mail_build_headers(HashTable *headers)
return s.s;
}

/* {{{ php_mail_disable_flags
Remove all dangerous command line flags
from the sendmail shell command
*/
PHPAPI zend_string *php_mail_disable_flags(const char *str)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please pass in the str_length as an argument, we know the size and this prevents a useless computation.

Also, this will just truncate the command if it contains a null byte.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, why is this exposed as a PHPAPI? can't it be static
{
const char* blacklist[4] = {"-O", "-C", "-X", "-be"};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be better to make this configurable as a list of potentially insecure flags changeable using INI. You could keep those as default.
size_t str_length = strlen(str);
char *return_str = emalloc(str_length + 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this allocation really needed?

You can just allocate the zend_string directly, and iterate on the underlying char pointer.
strcpy(return_str, str);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use memcpy.
for (int i = 0; i < 4; ++i) {
size_t blacklist_length = strlen(blacklist[i]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really not a fan of this, those strings are constant and computing the lengths at runtime really feels off
char* position = return_str;
while ((position = strstr(position, blacklist[i])) != NULL) {
memset(position, ' ', blacklist_length);
position += blacklist_length;
}
}
zend_string *cmd = zend_string_init(return_str, str_length, 0);
efree(return_str);
return cmd;
}

/* {{{ Send an email message */
PHP_FUNCTION(mail)
Expand Down Expand Up @@ -278,6 +300,9 @@ PHP_FUNCTION(mail)
if (force_extra_parameters) {
extra_cmd = php_escape_shell_cmd(force_extra_parameters);
} else if (extra_cmd) {
if (!PG(allow_additional_sendmail_flags)) {
extra_cmd = php_mail_disable_flags(ZSTR_VAL(extra_cmd));
}
extra_cmd = php_escape_shell_cmd(ZSTR_VAL(extra_cmd));
}

Expand Down
1 change: 1 addition & 0 deletions ext/standard/php_mail.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
PHP_MINFO_FUNCTION(mail);

PHPAPI zend_string *php_mail_build_headers(HashTable *headers);
PHPAPI zend_string *php_mail_disable_flags(const char *str);
PHPAPI extern int php_mail(const char *to, const char *subject, const char *message, const char *headers, const char *extra_cmd);

#define PHP_MAIL_BUILD_HEADER_CHECK(target, s, key, val) \
Expand Down
13 changes: 13 additions & 0 deletions ext/standard/tests/mail/mail_blacklist_allowed.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
--TEST--
Enable additional command line flags from being passed to sendmail through mail()
--INI--
sendmail_path=tee mail_blacklist.eml >/dev/null
mail.allow_additional_sendmail_flags = On
--FILE--
<?php
var_dump(mail('user@example.com', 'Test Subject', 'A Message', 'KHeaders', '-X new'));
?>
--EXPECT--
tee: invalid option -- 'X'
Try 'tee --help' for more information.
bool(false)
11 changes: 11 additions & 0 deletions ext/standard/tests/mail/mail_blacklist_disallowed.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
--TEST--
Disable additional command line flags from being passed to sendmail through mail()
--INI--
sendmail_path=tee mail_blacklist.eml >/dev/null
mail.allow_additional_sendmail_flags = Off
--FILE--
<?php
var_dump(mail('user@example.com', 'Test Subject', 'A Message', 'KHeaders', '-X new'));
?>
--EXPECT--
bool(true)
1 change: 1 addition & 0 deletions main/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -749,6 +749,7 @@ PHP_INI_BEGIN()
PHP_INI_ENTRY("smtp_port", "25", PHP_INI_ALL, NULL)
STD_PHP_INI_BOOLEAN("mail.add_x_header", "0", PHP_INI_SYSTEM|PHP_INI_PERDIR, OnUpdateBool, mail_x_header, php_core_globals, core_globals)
STD_PHP_INI_BOOLEAN("mail.mixed_lf_and_crlf", "0", PHP_INI_SYSTEM|PHP_INI_PERDIR, OnUpdateBool, mail_mixed_lf_and_crlf, php_core_globals, core_globals)
STD_PHP_INI_BOOLEAN("mail.allow_additional_sendmail_flags", "0", PHP_INI_SYSTEM|PHP_INI_PERDIR, OnUpdateBool, allow_additional_sendmail_flags, php_core_globals, core_globals)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to have a mode for this which would either allow, deny or ignore the list of insecure flags. The default should be deny so users get warning if such flag is present. That's better than just silently ignore things IMHO.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could potentially consider warn mode that would would ignore it but also produce warning so users can catch those changes. That could be potentially default.
STD_PHP_INI_ENTRY("mail.log", NULL, PHP_INI_SYSTEM|PHP_INI_PERDIR, OnUpdateMailLog, mail_log, php_core_globals, core_globals)
PHP_INI_ENTRY("browscap", NULL, PHP_INI_SYSTEM, OnChangeBrowscap)
PHP_INI_ENTRY("memory_limit", "128M", PHP_INI_ALL, OnChangeMemoryLimit)
Expand Down
1 change: 1 addition & 0 deletions main/php_globals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ struct _php_core_globals {
char *mail_log;
bool mail_x_header;
bool mail_mixed_lf_and_crlf;
bool allow_additional_sendmail_flags;

bool in_error_log;

Expand Down
4 changes: 4 additions & 0 deletions php.ini-development
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1107,6 +1107,10 @@ smtp_port = 25
; the 5th parameter to mail().
;mail.force_extra_parameters =

; Allow the blacklisted sendmail flags -O, -X, -C and -be to be passed into the
; the 5th parameter of the mail() function by setting to On.
mail.allow_additional_sendmail_flags = Off

; Add X-PHP-Originating-Script: that will include uid of the script followed by the filename
mail.add_x_header = Off

Expand Down
4 changes: 4 additions & 0 deletions php.ini-production
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1109,6 +1109,10 @@ smtp_port = 25
; the 5th parameter to mail().
;mail.force_extra_parameters =

; Allow the blacklisted sendmail flags -O, -X, -C and -be to be passed into the
; the 5th parameter of the mail() function by setting to On.
mail.allow_additional_sendmail_flags = Off

; Add X-PHP-Originating-Script: that will include uid of the script followed by the filename
mail.add_x_header = Off

Expand Down