CVE-2023-0568: 1-byte array overrun in common path resolve code

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

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-20 22:22 UTC] dossche dot niels at gmail dot com

Related To: Bug #81746

[2023-01-23 13:14 UTC] [email protected]

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 08:02 UTC] [email protected]

> 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-29 07:46 UTC] [email protected]

-CVE-ID: needed +CVE-ID: 2023-0568

[2023-01-30 05:16 UTC] [email protected]

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] [email protected]

-Status: Open +Status: Closed -Assigned To: +Assigned To: stas

[2023-02-13 04:40 UTC] [email protected]

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.