Skip to content

ext/zip: don't use gl_pathc after call to globfree (#79424) #5311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

ext/zip: don't use gl_pathc after call to globfree (#79424) #5311

wants to merge 1 commit into from

Conversation

maxcrees
Copy link
Contributor

This was introduced by b734bf4 ("- sync with PECL HEAD"), so 7.2, 7.3, 7.4, and master are affected. I based it against 7.3 per the CONTRIBUTING recommendations.

However, there is still a minor problem here - there is a discrepancy between the types of globbuf.gl_pathc (size_t) and the return value of php_zip_glob (int). I wasn't sure how best to correct this so I leave that to the maintainers.

@nikic
Copy link
Member

nikic commented Mar 27, 2020

Am I reading right that this is not a use-after-free in the classical sense (i.e. it would not show up in valgrind / asan), and as such we also don't need to treat this as a security issue?

@maxcrees
Copy link
Contributor Author

I'm not a security researcher so I'm not sure, I just wanted to fix this so that ZipArchive::addGlob is no longer broken on musl.

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the context. For reference, the musl implementation: https://github.com/bminor/musl/blob/d0b547dfb5f7678cab6bc39dd736ed6454357ca4/src/regex/glob.c#L300

@cmb69
Copy link
Member

cmb69 commented Mar 27, 2020

cc @remicollet

@remicollet
Copy link
Member

@sroracle could you please open a bug, so it can be linked in changelog ?

@remicollet remicollet self-assigned this Mar 28, 2020
@maxcrees maxcrees changed the title ext/zip: avoid use-after-free in php_zip_glob return value ext/zip: don't use gl_pathc after call to globfree (#79424) Mar 28, 2020
@maxcrees
Copy link
Contributor Author

Opened as #79424. Added an entry to NEWS and adjusted the commit message as appropriate.

This breaks on Linux with the musl libc, since it zeroes out gl_pathc during
globfree.
@remicollet
Copy link
Member

Thanks a lot,

Amended and applied in 7.3+

@remicollet remicollet closed this Mar 29, 2020
@maxcrees maxcrees deleted the zip-glob-uaf branch March 29, 2020 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants