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?
| if (entry.uncompressed_filesize == UINT_MAX || entry.uncompressed_filesize == 0) { | ||
| /* Check for overflow or unreasonable size - bug 61065 */ | ||
| if (entry.uncompressed_filesize == 0 | ||
| || entry.uncompressed_filesize == UINT_MAX |
There was a problem hiding this comment.
I guess this is pointless now ?
| /* 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.
crystarm
force-pushed
the
fix/phar-longlink-oom
branch
from
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. |
4 participants
Fixes #22556.