Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add robustness checks in RAR3 formats #5654

Merged
merged 3 commits into from
Jan 28, 2025
Merged

Add robustness checks in RAR3 formats #5654

merged 3 commits into from
Jan 28, 2025

Conversation

magnumripper
Copy link
Member

Fixes #5653

@magnumripper magnumripper mentioned this pull request Jan 27, 2025
Closed
@solardiz
Copy link
Member

I synced our code with the very last plain C code that was used in ClamAV for now (very small changes). I then made the effective [retval] changes after comparing with the newer C++ code.

This could be two commits.

@magnumripper
Copy link
Member Author

This could be two commits.

And is now. BTW I need to test this a bit more before merging.

@magnumripper magnumripper force-pushed the rar-5653 branch 2 times, most recently from bdd1f78 to 3b92a40 Compare January 27, 2025 09:03
@magnumripper magnumripper marked this pull request as draft January 27, 2025 23:03
@magnumripper
Copy link
Member Author

3b92a40 is not likely a fix we really want (although it doesn't hurt much really) as I suspect the issue is false positives and/or a corrupted archive. That story continues in #5653.

This PR will instead end up with some other goodies found in the process, stay tuned.

@magnumripper magnumripper marked this pull request as ready for review January 28, 2025 01:56
@magnumripper magnumripper changed the title Fix false negatives in RAR3 formats Add robustness checks in RAR3 formats Jan 28, 2025
@magnumripper
RAR3 debug code
c4db188 
Added while investigating openwall#5653.
@magnumripper
Sync our ClamAV unrar to the very last version that existed in plain C
92a2d07 
That code is no longer maintained (except by me lol).  ClamAV is now using
the official C++ version, just like hashcat.
@magnumripper
RAR3 unrar code: Tweaks
1c170b3 
Add more sanity checks/reject logic, that also helps robustness by
avoiding out of bounds reads.

Also adds a test asserting that the inflated size meets the expections.

See openwall#5653.
@magnumripper magnumripper merged commit 126b2a4 into openwall:bleeding-jumbo Jan 28, 2025
35 of 36 checks passed
@magnumripper magnumripper deleted the rar-5653 branch January 28, 2025 22:51
@magnumripper
Copy link
Member Author

magnumripper commented Jan 28, 2025 •
edited
Loading

So #5653 wasn't a bug (except in hashcat it was) - but the extra tests added here just might add a little more early rejection 👍 and I'm sure ASan will be slightly less furious over the unrar code now.

And in the end, this made me NOT want to replace the old ClamAV code with official unrar C++ code. The latter, as seen in hashcat, doesn't return a true or false like our current ClamAV code did to start with - and now with many more false (reject) conditions added by (the royal) us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@solardiz solardiz solardiz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Problem with $RAR3$*1* hash. John can't find known password. Very strange situation.
2 participants
@magnumripper @solardiz