Fix heap out-of-bounds reads on short iCLASS dump files - #3433
Open
munzzyy wants to merge 1 commit into
Open
Conversation
hf iclass eload -v, hf iclass decrypt and hf iclass eview -v all cast a loaded dump buffer to picopass_hdr_t* and print it without checking the buffer is that big. pm3_load_dump() sizes the allocation to the real file content and only clamps when it's larger than the max, never pads a short file up, so a truncated dump gives a short heap block and the print code reads past it. Same root cause as RfidResearchGroup#3412, which only covered view. decrypt is the worst of the three: no -v needed, and it probes fixed offsets through block 9 - app_issuer_area, the aa1_encryption flag and block 7 inside iclass_decode_credentials(), then its own block 9 PACS/PIN check - regardless of what applimit or decryptedlen/8 claim the real block count is. Check the loaded length against what's about to be read. eload and eview skip only the unsafe verbose print so a legitimately short custom dump still uploads; decrypt refuses below 10 blocks because the credential decode always reaches block 9. Signed-off-by: Cole Munz <colemunz@gmail.com>
|
You are welcome to add an entry to the CHANGELOG.md as well |
Contributor
Author
|
The failing
Every other check on this PR is green, including |
This file contains hidden or 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
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.
hf iclass eload -v,hf iclass decryptandhf iclass eview -vall cast a buffer loaded from a dump file straight topicopass_hdr_t*and print it, without checking the buffer is actually that big. Same root cause as #3412.pm3_load_dump()sizes the allocation to the real file content, and only clamps when the file is larger than the max; it never pads a short file up. So a truncated dump gives you a short heap block and the print code walks off the end. #3412 only coveredview.decryptis the worst of the three. It needs no-v, and it probes fixed offsets through block 9 (app_issuer_area, theaa1_encryptionflag and block 7 insideiclass_decode_credentials(), then its own block 9 PACS/PIN check) no matter whatapplimitordecryptedlen / 8say the real block count is. On a release build that prints garbage heap bytes labelled as CSN, config, debit key and PACS.Confirmed with
make client SANITIZE=1on an unmodified tree, no device attached:eload -f tiny.bin -vcrashes the same way insideprint_picopass_headerviasprint_hex/hex_to_buffer, the identical signature to the #3412 report.The fix checks the loaded length against what the code is about to read. For
eloadandeviewI only skip the unsafe verbose print, since the upload itself should still work on a legitimately short custom dump.decryptrefuses below 10 blocks, because the credential decode always reaches block 9.I first tried a 7-block minimum for
decryptand it still crashed one function deeper, in the block 9 check insideiclass_decode_credentials(). That's why the guard is 10 blocks and not 7.Boundary cases re-checked after the fix, same ASan build:
One honest gap: I couldn't get a live crash for
eview. It needs a connected device, and offlineGetFromDevicetimes out before reaching the vulnerable print. So that third guard is a same-pattern fix from reading the code, not a reproduced crash like the other two.