Repository navigation
gh-148178: Validate async debug offsets read from remote process - #148179
gpshead wants to merge 2 commits into
Conversation
Sorry, something went wrong.
|
I need to think a bit more about how to do this properly. We need to validate every field of every section, not just the async debug. I also need to think carefully how to do this without having to write all by hand and how to ensure we fail at build time if we forget. |
Sorry, something went wrong.
|
@gpshead I was working on a fix while you opened this. If you want we can iterate over this draft but since there is a lot more to do here and is quite tricky because I suspect we are going to need a lot of macros I am not sure if that's going to be the best route. |
Sorry, something went wrong.
The _remote_debugging module reads async_debug_offsets from the target process's memory but did not validate them, unlike debug_offsets which go through validate_debug_offsets(). The asyncio_task_object.size field is used as the read length into fixed-size 4096-byte stack buffers (SIZEOF_TASK_OBJ); a malicious or compromised target process could supply a larger size and overflow the debugger's stack. Add validate_async_debug_offsets() and call it from read_async_debug() (the single chokepoint for loading these offsets) to bound the task object size and the member offsets that index into the local buffer.
|
Totally fine if this PR gets closed. I mostly i wanted to surface it as food for thought (I suppose I could've just linked to the branch for that without a draft PR). |
Sorry, something went wrong.
Thanks a lot! I really appreciate the help and I want to say this explicitly :) It's just that unfortunately this fix will be tricky to balance in all fronts :( |
Sorry, something went wrong.
|
Closing in favour of #148187 Thanks for the draft Greg! |
Sorry, something went wrong.
The _remote_debugging module reads async_debug_offsets from the target process's memory but did not validate them, unlike debug_offsets which go through validate_debug_offsets(). The asyncio_task_object.size field is used as the read length into fixed-size 4096-byte stack buffers (SIZEOF_TASK_OBJ); a malicious or compromised target process could supply a larger size and overflow the debugger's stack.
Add validate_async_debug_offsets() and call it from read_async_debug() (the single chokepoint for loading these offsets) to bound the task object size and the member offsets that index into the local buffer.
--
Opening as a draft to start with as this isn't my area of the code and there are possibly more of these to be fixed. Claude authored the fix.