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;

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>

> 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

> 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.

I *think* that this patch is correct: https://gist.github.com/nielsdos/fa5a19ee190045ed362eea265774cd60

Thanks

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.

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.

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.

https://github.com/mandiripinjamandana