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 Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
If you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
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)

Pull Requests

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: Thu Nov 21 12:01:29 2024 UTC