Skip to content

ext/bz2: bzcompress/bzdecompress check inputs lengths beforehand. #17887

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

Merged
merged 1 commit into from
Feb 23, 2025

Conversation

devnexen
Copy link
Member

also removing useless casts for filter ZendMM parts.

@devnexen devnexen marked this pull request as ready for review February 22, 2025 19:12
ext/bz2/bz2.c Outdated
@@ -450,6 +450,13 @@ PHP_FUNCTION(bzcompress)
RETURN_THROWS();
}

const unsigned int source_len_max = (unsigned int)((UINT_MAX - 600) / 1.01);

if (source_len > source_len_max) {
Copy link
Member

Choose a reason for hiding this comment

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

Something's off, I removed this check and it compressed just fine.
Just because the max output length is expressed in terms of the input length (i.e. (source_len + (0.01 * source_len) + 600)) doesn't mean that's a limit.
I didn't check decompression but I suppose the same mistake happened there.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, was trying to cater to the type (unsigned int).

Copy link
Member

@nielsdos nielsdos Feb 23, 2025

Choose a reason for hiding this comment

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

Hmmm, surprisingly indeed the theoretical bound would be what you checked on then. However, this is the worst-case and we certainly can make inputs that compress in less bytes. So a lot of inputs would be unrightfully blocked if you did this check.
What probably needs to happen is use of the lower level interfaces to not do compression/decompression in a one-shot manner.
https://sourceware.org/pub/bzip2/docs/v101/manual_3.html states:

Compression in this manner is a one-shot event, done with a single call to this function. The resulting compressed data is a complete bzip2 format data stream. There is no mechanism for making additional calls to provide extra input data. If you want that kind of mechanism, use the low-level interface.
This would allow going beyond unsigned int.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh I see, bzCompressInit -> bzCompress/BZ_RUN until we fill all or error -> bzCompressEnd. I ll see about this later :)

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Dropping the casts is fine, but if you merge this then make sure to use the commit title instead of the PR title

@devnexen devnexen merged commit 67a349d into php:master Feb 23, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants