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
scan_eol() Buffer Over-read #69177
Comments
Python 3.5 suffers from a vulnerability caused by the behavior of the scan_eol() function. When called, the function gets a line from the buffer of a BytesIO object by searching for a newline character starting at the position in the buffer. However, if the position is set to a value that is larger than the buffer, this logic will result in a call to memchr that reads off the end of the buffer:
if (len) {
n = memchr(start, '\n', len); In some applications, it may be possible to exploit this behavior to disclose the contents of adjacent memory. The buffer over-read can be observed by running the following script: import tempfile
a = tempfile.SpooledTemporaryFile()
a.seek(0b1)
a.readlines() Which, depending on the arrangement of memory, may produce an exception such as this: 0:000> g To fix this issue, it is recommended that scan_eol() be updated to check that the position is not greater than or equal to the size of the buffer. A proposed patch is attached. |
Simpler test case, which might find a place somewhere like /Lib/test/test_memoryio.py: >>> from io import BytesIO
>>> b = BytesIO()
>>> b.seek(1)
1
>>> b.readlines() # Should return an empty list
Segmentation fault (core dumped)
[Exit 139] The patch looks like the right approach. Just watch out for the indentation (tabs vs spaces). However I don’t see why we need the GET_BYTES_SIZE() macro or the (size_t) casting. Would it be okay comparing PyBytes_GET_SIZE() directly to self->pos? They are both meant to be Py_ssize_t. The bug looks like it was introduced by Serhiy’s changes in bpo-15381. |
We based our fix on the check in write_bytes: if (endpos > (size_t)PyBytes_GET_SIZE(self->buf)) {
if (resize_buffer(self, endpos) < 0)
return -1;
} I see now that our casting was extraneous. As for the macro, it was suspected that similar issues may be present and we wanted to write reusable code, but this also seems unnecessary now that it's known the cast is unneeded. Early tomorrow I'll take some time to create a revised patch. |
It should be self->string_size, not PyBytes_GET_SIZE(self->buf). Thank you for your report and your patch John. Here is revised patch with tests based on Martin's test. Larry, perhaps this bug is grave enough to be fixed in RC3. |
scan_eol_Buffer_Over-read_2.patch looks good to me.
Since it looks like a regression in Python 3.5, yes, it's a severe bug. Please send a pull request to Larry for it. |
Yes, please create a pull request for this patch. Thanks! And just to confirm: I just applied patch 2 to CPython, then undid the change to bytesio.c. The new test fails, and sometimes Python will segmentation fault. If I then apply the patch to bytesio.c it passes. Nice work! |
New changeset a5858c30db7c by Serhiy Storchaka in branch '3.5': New changeset 215800fb955d by Serhiy Storchaka in branch 'default': |
Pull request accepted. Please forward-merge. Thanks! |
New changeset 2b6ce7e9595c by Serhiy Storchaka in branch '3.5': |
New changeset 07e04c34bab5 by Larry Hastings in branch '3.5': |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: