fix panic on 256 bits RC4 keys #230
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Found this bug while fuzzing the crate. The spec says that in the ecryption dictonary, /Length is the length of the key, and should be between 40 to 128 bits. However, if we specify 256 the parser still accepts it but panics in Decoder::decrypt() while performing the b) step. The b) step is implemented by truncating the key length to a maximum of 128 bits (16 bytes) before concatenating the object number and generation of the string being decrypted. However, while computing the size of n, Decoder::decode() does not truncate the key size to 16, and this leads to an out of bound panic. Note that I can't find the reason for this trucation to 16 bytes in the b) step. The spec does require truncating the key to 16 bytes, but that's in the d) step which is where we truncate the resulting hash. Maybe a better fix would be to reject a /Length of 256 bits in the parser instead.
|
Highly relates to #231 |
|
I think not limiting the length and then doing the d) step correct might be the better solution? |
|
Are there any valid pdfs with 256bit keys? |
s3bk
approved these changes
Aug 2, 2024
|
This fixes a panic. If you have a better solution that can be merged later. |
No idea 🤷♀ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Found this bug while fuzzing the crate.
The spec says that in the ecryption dictonary, /Length is the length of the key, and should be between 40 to 128 bits. However, if we specify 256 the parser still accepts it but panics in Decoder::decrypt() while performing the b) step.
The b) step is implemented by truncating the key length to a maximum of 128 bits (16 bytes) before concatenating the object number and generation of the string being decrypted.
However, while computing the size of n, Decoder::decode() does not truncate the key size to 16, and this leads to an out of bound panic.
Note that I can't find the reason for this trucation to 16 bytes in the b) step. The spec does require truncating the key to 16 bytes, but that's in the d) step which is where we truncate the resulting hash.
Maybe a better fix would be to reject a /Length of 256 bits in the parser instead.