-
Notifications
You must be signed in to change notification settings - Fork 63
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
Integer overflow in stack walker when processing minidump. #422
Comments
...huh. It's surprising you're seeing overflow there given I have an explicit guard on the previous line for preventing an overflow in this code. ... forehead slap
|
fixing it... |
Fixes rust-minidump#422 This issue is kinda nonsense, but in a 'hey good job fuzzer for finding this wild input' kinda way? Basically rust-minidump likes doing things in 64-bit to reuse code and keep things uniform, but this creates a fun situation where you can define *impossible* memory ranges for a 32-bit system and have calculated pointers successfully access that memory because the operations are happening in 64-bit. In-and-of-itself this is not a problem, but it does mean that things can go Bad when you have some of the operations happening in the platform's native width, causing some operations to succeed and others to overflow. In particular, this would allow you to load some memory that is beyond u32::MAX (which normally would be a strong proof that the entire memory range is a valid pointer) and then update the 32-bit frame pointer to that address, causing an overflow. The fix is to simply do the overflow check that *already existed for exactly this code* in 32-bit instead of 64-bit. I'm not sure if an actual minidump would have been able to trigger this bug, since it's possible some earlier code would have freaked out at the memory range definitions. But hey, the guards were wrong either way.
Fixes #422 This issue is kinda nonsense, but in a 'hey good job fuzzer for finding this wild input' kinda way? Basically rust-minidump likes doing things in 64-bit to reuse code and keep things uniform, but this creates a fun situation where you can define *impossible* memory ranges for a 32-bit system and have calculated pointers successfully access that memory because the operations are happening in 64-bit. In-and-of-itself this is not a problem, but it does mean that things can go Bad when you have some of the operations happening in the platform's native width, causing some operations to succeed and others to overflow. In particular, this would allow you to load some memory that is beyond u32::MAX (which normally would be a strong proof that the entire memory range is a valid pointer) and then update the 32-bit frame pointer to that address, causing an overflow. The fix is to simply do the overflow check that *already existed for exactly this code* in 32-bit instead of 64-bit. I'm not sure if an actual minidump would have been able to trigger this bug, since it's possible some earlier code would have freaked out at the memory range definitions. But hey, the guards were wrong either way.
Would it make things better to fiddle the APIs to make the shared unwinder machinery generic over the CPU word size? I'd have to go read through the code again to see if that's feasible. |
In my memory there's a certain amount of |
Apologies for the very poorly minimised test case, just making a note of this now so I don't forget.
The bug is in the stack walker, but I found this when fuzzing
minidump_processor
.The text was updated successfully, but these errors were encountered: