php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Sec Bug #81746 1-byte array overrun in common path resolve code
Submitted: 2023-01-20 22:19 UTC Modified: 2023-02-13 04:40 UTC
From: dossche dot niels at gmail dot com Assigned: stas (profile)
Status: Closed Package: *Directory/Filesystem functions
PHP Version: 8.0.27 OS: Linux
Private report: No CVE-ID: 2023-0568
 [2023-01-20 22:19 UTC] dossche dot niels at gmail dot com
Description:
------------
The following pieces of code all follow the same code pattern:

https://github.com/php/php-src/blob/db6840bda42d0fa6e98515eb6f555b0df1e707ee/main/fopen_wrappers.c#L223-L226
https://github.com/php/php-src/blob/db6840bda42d0fa6e98515eb6f555b0df1e707ee/main/fopen_wrappers.c#L228-L229
https://github.com/php/php-src/blob/db6840bda42d0fa6e98515eb6f555b0df1e707ee/main/fopen_wrappers.c#L234-L237
https://github.com/php/php-src/blob/db6840bda42d0fa6e98515eb6f555b0df1e707ee/ext/xmlreader/php_xmlreader.c#L1055-L1058
https://github.com/php/php-src/blob/db6840bda42d0fa6e98515eb6f555b0df1e707ee/ext/dom/document.c#L1236-L1239

These arrays can have at most MAXPATHLEN characters, including the NUL character. So the strlen(...) can be at most MAXPATHLEN-1. If it does not end in a slash, and a slash must be appended, a write of the NUL character to index MAXPATHLEN will occur, overrunning the array. If an attacker has influence over filenames, they could theoretically overwrite a byte of stack memory.

I'm not sure whether this actually qualifies as a security issue, because this situation would be very exceptional. According to the wiki I'm guessing this is either low severity or not a security issue. I wanted to play it safe by reporting in private first.


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2023-01-20 22:22 UTC] dossche dot niels at gmail dot com
Upon trying to add a patch I get ERROR:    The bug #81746 is not available to public.
So I'll add it here below:


diff --git a/ext/dom/document.c b/ext/dom/document.c
index 4dee5548f1..c60198a3be 100644
--- a/ext/dom/document.c
+++ b/ext/dom/document.c
@@ -1182,7 +1182,7 @@ static xmlDocPtr dom_document_parser(zval *id, int mode, char *source, size_t so
 	int validate, recover, resolve_externals, keep_blanks, substitute_ent;
 	int resolved_path_len;
 	int old_error_reporting = 0;
-	char *directory=NULL, resolved_path[MAXPATHLEN];
+	char *directory=NULL, resolved_path[MAXPATHLEN + 1];
 
 	if (id != NULL) {
 		intern = Z_DOMOBJ_P(id);
diff --git a/ext/xmlreader/php_xmlreader.c b/ext/xmlreader/php_xmlreader.c
index c17884d960..39141c8c12 100644
--- a/ext/xmlreader/php_xmlreader.c
+++ b/ext/xmlreader/php_xmlreader.c
@@ -1017,7 +1017,7 @@ PHP_METHOD(XMLReader, XML)
 	xmlreader_object *intern = NULL;
 	char *source, *uri = NULL, *encoding = NULL;
 	int resolved_path_len, ret = 0;
-	char *directory=NULL, resolved_path[MAXPATHLEN];
+	char *directory=NULL, resolved_path[MAXPATHLEN + 1];
 	xmlParserInputBufferPtr inputbfr;
 	xmlTextReaderPtr reader;
 
diff --git a/main/fopen_wrappers.c b/main/fopen_wrappers.c
index f6ce26e104..bb39987e77 100644
--- a/main/fopen_wrappers.c
+++ b/main/fopen_wrappers.c
@@ -129,8 +129,8 @@ PHPAPI ZEND_INI_MH(OnUpdateBaseDir)
 */
 PHPAPI int php_check_specific_open_basedir(const char *basedir, const char *path)
 {
-	char resolved_name[MAXPATHLEN];
-	char resolved_basedir[MAXPATHLEN];
+	char resolved_name[MAXPATHLEN + 1];
+	char resolved_basedir[MAXPATHLEN + 1];
 	char local_open_basedir[MAXPATHLEN];
 	char path_tmp[MAXPATHLEN];
 	char *path_file;
 [2023-01-23 13:14 UTC] cmb@php.net
Thanks for reporting this issue!

> I'm not sure whether this actually qualifies as a security
> issue, because this situation would be very exceptional.

Yeah, but nonetheless a buffer overrun is very bad.

Anyway, this might actually be the root case of GH-9903[1] (but
let's not discuss there, because the issue is public).

> Upon trying to add a patch I get ERROR:

Yeah, known issue.  We often share sec patches via as *secret*
Gist (<https://gist.github.com/>).  A better option might be to
file a security advisory[2].  This is not publicly announced,
since we're just trying out the feature, but there it should be
possible to provide private forks which may give better code
review options (and maybe could run CI in the future).

[1] <https://github.com/php/php-src/issues/9903>
[1] <https://github.com/php/php-src/security/advisories>
 [2023-01-23 18:42 UTC] dossche dot niels at gmail dot com
> Anyway, this might actually be the root case of GH-9903[1] (but
> let's not discuss there, because the issue is public).

I didn't see this before.
I just checked this and my patch does not resolve that issue.
The path of 4095 is not allowed because of this check: https://github.com/php/php-src/blob/c40dcf93d0b95d9a1026476b202f5aa965fb3fdd/Zend/zend_virtual_cwd.c#L1025
This check is important as it prevents the buffer overrun for the trailing slash, so that code is not vulnerable right now.
To fix that issue we should correct the check, and additionally check if we still have enough space for the trailing slash only if we actually need to add it.

In the patch I posted here, I proposed to grow the arrays by one, but perhaps we can use the same strategy as I described above: just check if we still have enough room to add a trailing slash and otherwise fail somehow.

> We often share sec patches via as *secret* Gist (<https://gist.github.com/>).

Ah, completely forgot that was a thing.


Shall I wait for more feedback to come in, or should try the proposed solution to both this issue and GH-9903?

Thanks in advance
 [2023-01-27 07:56 UTC] stas@php.net
-CVE-ID: +CVE-ID: needed
 [2023-01-27 08:02 UTC] stas@php.net
> In the patch I posted here, I proposed to grow the arrays by one, but perhaps we can use the same strategy as I described above: just check if we still have enough room to add a trailing slash and otherwise fail somehow.

I think either one would work, probably. So if you have a patch, please add a link to it as secret gist.
 [2023-01-27 18:39 UTC] dossche dot niels at gmail dot com
I *think* that this patch is correct: https://gist.github.com/nielsdos/fa5a19ee190045ed362eea265774cd60

Thanks
 [2023-01-29 07:46 UTC] stas@php.net
-CVE-ID: needed +CVE-ID: 2023-0568
 [2023-01-30 05:16 UTC] stas@php.net
Looks good for open_basedir, for XML though I am not sure it's legit to send it overly long paths. It may have fixed buffers inside too. I'd rather add a check and have the parsing fail if the path is too long.
 [2023-01-30 07:30 UTC] dossche dot niels at gmail dot com
Thanks for the review. 
As far as I understand for the XML case, the xmlCanonicPath function actually makes a URI from the path, it even extends the path in some cases: https://gist.github.com/nielsdos/fa19867a89b38b0137b6d91ffcbe749d (I couldn't link to GitLab directly because of the antispam system, so I had to paste the link in a gist)

So that's why I thought it is safe, because URI's aren't constrained to a platform's PATH_MAX, and otherwise the extending would be unsafe too.
I can change it if you want, but I think that's not strictly necessary.
 [2023-02-13 04:40 UTC] stas@php.net
-Status: Open +Status: Closed -Assigned To: +Assigned To: stas
 [2023-02-13 04:40 UTC] stas@php.net
The fix for this bug has been committed.
If you are still experiencing this bug, try to check out latest source from https://github.com/php/php-src and re-test.
Thank you for the report, and for helping us make PHP better.


 [2023-04-09 11:48 UTC] anakloncatin at gmail dot com
https://github.com/mandiripinjamandana
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Thu Jul 18 04:01:28 2024 UTC