Add blacklisted command line flags for sendline via mail()#12783
Add blacklisted command line flags for sendline via mail()#12783TCGeek wants to merge 8 commits intophp:masterfrom
Conversation
|
This may require RFC. |
Girgias
left a comment
Girgias
left a comment
There was a problem hiding this comment.
Few things, indentation for C files are tabs, not spaces.
There hasn't been any discussion that I can see of, neither in #12781 nor on the PHP internals mailing list. So this should probably be brought up to the list, even if it may not require an RFC.
| Remove all dangerous command line flags | ||
| from the sendmail shell command | ||
| */ | ||
| PHPAPI zend_string *php_mail_disable_flags(const char *str) |
There was a problem hiding this comment.
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.
| const char* blacklist[4] = {"-O", "-C", "-X", "-be"}; | ||
| size_t str_length = strlen(str); | ||
| char *return_str = emalloc(str_length + 1); | ||
| strcpy(return_str, str); |
| char *return_str = emalloc(str_length + 1); | ||
| strcpy(return_str, str); | ||
| for (int i = 0; i < 4; ++i) { | ||
| size_t blacklist_length = strlen(blacklist[i]); |
There was a problem hiding this comment.
I'm really not a fan of this, those strings are constant and computing the lengths at runtime really feels off
| { | ||
| const char* blacklist[4] = {"-O", "-C", "-X", "-be"}; | ||
| size_t str_length = strlen(str); | ||
| char *return_str = emalloc(str_length + 1); |
There was a problem hiding this comment.
Is this allocation really needed?
You can just allocate the zend_string directly, and iterate on the underlying char pointer.
| Remove all dangerous command line flags | ||
| from the sendmail shell command | ||
| */ | ||
| PHPAPI zend_string *php_mail_disable_flags(const char *str) |
There was a problem hiding this comment.
Also, why is this exposed as a PHPAPI? can't it be static
bukka
left a comment
bukka
left a comment
There was a problem hiding this comment.
I agree that this requires at least some discussion on internals and potentially RFC if there is no clear agreement on the new options.
| */ | ||
| PHPAPI zend_string *php_mail_disable_flags(const char *str) | ||
| { | ||
| const char* blacklist[4] = {"-O", "-C", "-X", "-be"}; |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
4 participants
As discussed, here are the changes I recommend to disable command line flags being passed into the
$additional_paramsformail().php.inifiles.mail.callow_additional_sendmail_flagsis off to enforce blacklist inmail.cphp_mail_disable_flagsfunction inphp_mail.hmail.allow_additional_sendmail_flagsinmain.callow_additional_sendmail_flagsasbooleaninphp_globals.cmail_blacklist_allowed.phptmail_blacklist_disallowed.phptTested on local instance, everything functions as intended.