-
-
Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-143988: tation crashes in socket sendmsg/recvmsg_into via __buffer__
#143987
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
base: main
Are you sure you want to change the base?
gh-143988: tation crashes in socket sendmsg/recvmsg_into via __buffer__
#143987
Conversation
__buffer____buffer__
333ee07 to
78c18ed
Compare
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be poked with soft cushions! |
78c18ed to
42ad743
Compare
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @picnixz: please review the changes made to this pull request. |
Lib/test/test_socket.py
Outdated
| try: | ||
| left.sendmsg(seq) | ||
| except (TypeError, OSError): | ||
| pass # Expected - the important thing is no crash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case use assertRaises.
Lib/test/test_socket.py
Outdated
| left.sendmsg(seq) | ||
| except (TypeError, OSError): | ||
| pass # Expected - the important thing is no crash | ||
| finally: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of finally, use addCleanup.
Lib/test/test_socket.py
Outdated
| seq[:] = [ | ||
| MutBuffer(b'Hello'), | ||
| b'World', | ||
| b'Test', | ||
| ] | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it on one line..It is short enough. And no need for a slice replacement. You can just write seq = ... (or would there be some NameError?)
|
|
||
| @unittest.skipUnless(hasattr(socket.socket, "sendmsg"), | ||
| "sendmsg not supported") | ||
| def test_sendmsg_reentrant_data_mutation(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a refwrence to the gh issue as well. Using plain comments and
"See: URL TO THE ISSUE."
And not just gh-XXXXXX. URLs can be opened directly from IDEs, but not gh-* objects.
Lib/test/test_socket.py
Outdated
|
|
||
| class MutBuffer: | ||
| def __init__(self, data): | ||
| self._data = bytes(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think you need an attribute. You can just return a memoryview on the fly.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
Please read the devguide:
|
720a086 to
4b592ef
Compare
…cvmsg_into via __buffer__ Fix crashes in socket.sendmsg() and socket.recvmsg_into() that could occur if buffer sequences are mutated re-entrantly during argument parsing via __buffer__ protocol callbacks. The vulnerability occurs because: 1. PySequence_Fast() returns the original list object when the input is already a list (not a copy) 2. During iteration, PyObject_GetBuffer() triggers __buffer__ callbacks which may clear the list 3. Subsequent iterations access invalid memory (heap OOB read) The fix replaces PySequence_Fast() with PySequence_Tuple() which always creates a new tuple, ensuring the sequence cannot be mutated during iteration. This addresses two vulnerabilities related to pythongh-143637: - sendmsg() argument 1 (data buffers) - via __buffer__ - recvmsg_into() argument 1 (buffers) - via __buffer__
4b592ef to
2c8aad6
Compare
Sorry about the force pushes - I'll use regular commits going forward. Thanks for the reminder about the devguide. |
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @picnixz: please review the changes made to this pull request. |
Lib/test/test_socket.py
Outdated
| # Should not crash. With the fix, the call succeeds; | ||
| # without the fix, it would crash (SIGSEGV). | ||
| try: | ||
| left.sendmsg(seq) | ||
| except (TypeError, OSError): | ||
| pass # Also acceptable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the useless comments. It is obvious that we do not want a crash. However, I want to know why we silence the exceptions. Ideally we want an exact exception catching (whatever it is) with assertRaises or there should be no exception at all.
Lib/test/test_socket.py
Outdated
|
|
||
| class MutBuffer: | ||
| def __init__(self, data): | ||
| self._data = bytearray(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well you do not need the attribute I guess.
| @@ -0,0 +1,3 @@ | |||
| Fixed crashes in :meth:`socket.socket.sendmsg` and :meth:`socket.socket.recvmsg_into` | |||
| that could occur if buffer sequences are mutated re-entrantly during argument parsing | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that could occur if buffer sequences are concurrently mutated
- Remove argument parsing mention. It is an implementation detail.
- We do not really use the term re-entrency. I do not share the definition of re-entrency and prefer using concurrently here (you do two things at the same time)
| } | ||
| for (; nbufs < nitems; nbufs++) { | ||
| if (!PyArg_Parse(PySequence_Fast_GET_ITEM(fast, nbufs), | ||
| if (!PyArg_Parse(PyTuple_GET_ITEM(fast, nbufs), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit confused with this one. There is nothing that checks for the fact that this really a buffer object or did I forget about PyArg_Parse?
Summary
This PR fixes two heap out-of-bounds read vulnerabilities in
socket.sendmsg()andsocket.recvmsg_into()that are related to gh-143637.While gh-143637 addresses the
__index__re-entrancy issue insendmsg()ancillary data parsing (argument 2), there are two additional vulnerable code paths that use the__buffer__protocol:socket.sendmsg()argument 1 (data buffers) - insock_sendmsg_iovec()socket.recvmsg_into()argument 1 (buffers) - insock_recvmsg_into()Root Cause
The vulnerability occurs because:
PySequence_Fast()returns the original list object when the input is already a list (not a copy)PyObject_GetBuffer()triggers__buffer__protocol callbacks which may clear the listProof of Concept
sendmsg()data buffers crash:recvmsg_into()buffers crash:Fix
Replace
PySequence_Fast()withPySequence_Tuple()which always creates a new tuple, ensuring the sequence cannot be mutated during iteration.Testing
Added
Lib/test/test_socket_reentrant.pywith regression tests for both vulnerable code paths.Note: This is related to but separate from PR #143892 which fixes the
__index__issue.