Add Check if Block Size Exceeds Maximum #1685
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
lib/decompress/zstd_decompress.c
Outdated
| { blockProperties_t bp; | ||
| size_t const cBlockSize = ZSTD_getcBlockSize(src, ZSTD_blockHeaderSize, &bp); | ||
| if (ZSTD_isError(cBlockSize)) return cBlockSize; | ||
| RETURN_ERROR_IF(cBlockSize > ZSTD_BLOCKSIZE_MAX, corruption_detected, "Block Size Exceeds Maximum"); |
There was a problem hiding this comment.
Yes, this is a first good place for this check.
It controls the compressed size, and no block shall have its compressed size larger than maximum block size.
However :
- No block shall have its decompressed size larger than maximum block size. This is more complex, and must be controlled elsewhere.
ZSTD_BLOCKSIZE_MAXis the "maximum maximum block size". But the specifications make it clear that the maximum block size can be smaller than that, depending on frame header instructions. It actually depends on the window size. This value should be computed once, at header decoding stage, stored intodctx, and then used for later checks.
You may consider the 2nd point as an extension.
Comparing with a constant ZSTD_BLOCKSIZE_MAX is a useful first step, which is already valuable.
There was a problem hiding this comment.
Thanks so much, this was really helpful! I think the maximum block size (min of window size and block size) seemed to already be stored in dctx->fParams.blockSizeMax, so I switched to using that. I've also added a check for the decompressed block size by checking rSize returned after decompressing a block.
|
Thanks @vivekmig ! Now, it seems it generates errors in the It might be because |
|
Updated decodecorpus test to not create an RLE block if frame content size is 0, this eliminates the possibility that an RLE block exceeds the max block size. |
|
Great work @vivekmig ! We are in the middle of a new release, so we'll wait a little bit for the actual merge. |
|
Release completed, we can merge |
Adding check if block size exceeds maximum.
Tested by creating a frame with a fake frame with raw block of size approximately 500 kb, and verifying the output:
