php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login

go to bug id or search bugs for

Sec Bug #79330 shell_exec() silently truncates after a null byte
Submitted: 2020-03-01 19:19 UTC Modified: 2020-04-14 04:10 UTC
From: 64796c6e69 at gmail dot com Assigned: stas (profile)
Status: Closed Package: Program Execution
PHP Version: Irrelevant OS: any
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If this is not your bug, you can add a comment by following this link.
If this is your bug, but you forgot your password, you can retrieve your password here.
Password:
Status:
Package:
Bug Type:
Summary:
From: 64796c6e69 at gmail dot com
New email:
PHP Version: OS:

 

 [2020-03-01 19:19 UTC] 64796c6e69 at gmail dot com 
Description:
------------
shell_exec() silently truncates anything after a null byte in the command it uses.

This was tested on PHP 7.3, but all supported PHP versions have this bug.

There's no issue if escapeshellarg() is used, but I thought it would be best to make this a security bug regardless. There may be some use case I haven't considered that creates a vulnerability. Checking for a null byte would also be consistent with exec() and other process functions.

The backtick operator has the same problem, but there is less risk of that causing a vulnerability.

Test script:
---------------
<?php
var_dump(shell_exec("echo before\0after"));

Expected result:
----------------
Warning: shell_exec(): NULL byte detected. Possible attack in shell_exec.php on line 2
bool(false)

Actual result:
--------------
string(7) "before
"

Patches

shell_exec.patch (last revision 2020-03-01 19:19 UTC by 64796c6e69 at gmail dot com)

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2020-03-21 17:02 UTC] cmb@php.net
-Status: Open +Status: Verified -Assigned To: +Assigned To: stas
 [2020-03-21 17:02 UTC] cmb@php.net 
Instead of using the attached patch, I wonder whether we should
simply check for a valid path (like popen() does):

 ext/standard/exec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ext/standard/exec.c b/ext/standard/exec.c
index b748e173ce..2d7961d908 100644
--- a/ext/standard/exec.c
+++ b/ext/standard/exec.c
@@ -528,7 +528,7 @@ PHP_FUNCTION(shell_exec)
 	php_stream *stream;
 
 	ZEND_PARSE_PARAMETERS_START(1, 1)
-		Z_PARAM_STRING(command, command_len)
+		Z_PARAM_PATH(command, command_len)
 	ZEND_PARSE_PARAMETERS_END();
 
 #ifdef PHP_WIN32
 [2020-03-30 20:13 UTC] 64796c6e69 at gmail dot com 
I think either would be fine. My patch causes shell_exec() to act the same as exec() for null bytes. I thought that made more sense only to make the error message more logical. "expects parameter 1 to be a valid path, string given" is a bit strange to see when the first argument is a command line. However, I'm also not opposed to that being the error. It would be more consistent with other functions.

Also, when I was seeing how different shell functions handle this, I found that proc_open() has the same problem. So, whichever implementation is chosen should be applied to that function as well.
 [2020-04-13 17:08 UTC] cmb@php.net 
Hmm, just noticed that this issue had already been reported as bug
#76035.
 [2020-04-14 03:54 UTC] stas@php.net 
I like the customized patch more, exec() argument is not really a path...
 [2020-04-14 04:10 UTC] stas@php.net 
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=14fcc813948254b84f382ff537247d8a7e5e0e62
Log: Fix bug #79330 - make all execution modes consistent in rejecting \0
 [2020-04-14 04:10 UTC] stas@php.net
-Status: Verified +Status: Closed
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved. 		Last updated: Wed Apr 24 02:01:30 2024 UTC