ext/phar: add upper-bound check on LongLink entry size to prevent OOM DoS - #22557
ext/phar: add upper-bound check on LongLink entry size to prevent OOM DoS#22557crystarm wants to merge 1 commit into
Conversation
|
@crystarm You might need to add a NEWS entry to inform the users about your change. Also given this is actually making the checker stricter, I don't sure whether we should target 8.4 or master for this fix (not sure this is a bug fix or a feature request) might need to hear from other's opinions. |
Girgias
left a comment
Girgias
left a comment
There was a problem hiding this comment.
Why are we checking against MAXPATHLEN? This doesn't seem to be related to a path at all?
| /* Check for overflow or unreasonable size - bug 61065 */ | ||
| if (entry.uncompressed_filesize == 0 | ||
| || entry.uncompressed_filesize == UINT_MAX | ||
| || entry.uncompressed_filesize > MAXPATHLEN) { |
There was a problem hiding this comment.
The MAXPATHLEN constant is for reflecting the platform's max path length. Well I agree we need to add a maximum filesize but seems like we need to introduce a new private constant on this.
Maybe UINT16_MAX will be proper? @Girgias
There was a problem hiding this comment.
MAXPATHLEN is usually 1024 or 4096, nowhere near UINT*_MAX values is what I meant.
There was a problem hiding this comment.
Anyhow we shouldn't use MAXPATHLEN here because at least we don't want the max filesize of a tar phar file associated with the platform's max path len. Perhaps we need a separate constant on this.
There was a problem hiding this comment.
Kind of agree, now that I m a bit more awake, MAXPATHLEN is inappropriate in this context anyway.
a7c03d4 to
5d640a8
Compare
|
Thanks everyone for the feedback! ( ദ്ദി °ᗜ° ) I've addressed all the suggestions and added a NEWS entry. |
|
@LamentXU123 @devnexen @Girgias |
|
8.3 is now end of life. Also, we generally merge up branches instead of backport to lower branches. So you can just rebase to 8.4 if you want to (but I don't sure if this should goes to 8.4 because this looks like a new feature to me, might need to wait for other's opinion). Also you can draft this PR if you are rebasing to prevent accidentally request a review to everyone. |
5d640a8 to
fc75b11
Compare
|
The PR looks good code wise. However, it is still controversial to decide which branch should be targeted in merging. @Girgias @iluuu1994 do you have any suggestions on this topic? |
|
Gentle ping @Girgias @iluuu1994 |
|
I would say bugfix and to rebase this onto PHP 8.4. I regard this as a (security) bug fix, based on the linked issue. It does not change much functionally, but fails more elegantly. |
fc75b11 to
0b3b9d9
Compare
|
I'm not convinced this is a bug. Reaching the memory_limit in PHP does not qualify as DoS, the request simply aborts and the SAPI continues handling next request. The allocation will use emalloc() by default, and only malloc() for That said: This type of issue can quickly turn out to have security implications. So next time please report responsibly if you're unsure. Your report even says "DoS". |
6 participants
Fixes #22556.