| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
I wrote a patch for Python 3.14 to check if it's also affected: diff --git a/Include/internal/pycore_bytesobject.h b/Include/internal/pycore_bytesobject.h
index 8ea9b3ebb88..7ab96b109a5 100644
--- a/Include/internal/pycore_bytesobject.h
+++ b/Include/internal/pycore_bytesobject.h
@@ -86,6 +86,7 @@ typedef struct {
/* Stack buffer */
int use_small_buffer;
char small_buffer[512];
+ char canary_byte;
} _PyBytesWriter;
/* Initialize a bytes writer
diff --git a/Objects/bytesobject.c b/Objects/bytesobject.c
index 03245788bb1..be698236843 100644
--- a/Objects/bytesobject.c
+++ b/Objects/bytesobject.c
@@ -3456,6 +3456,7 @@ _PyBytesWriter_Init(_PyBytesWriter *writer)
memset(writer->small_buffer, PYMEM_CLEANBYTE,
sizeof(writer->small_buffer));
#endif
+ writer->canary_byte = 0xAB;
}
void
@@ -3524,6 +3525,8 @@ _PyBytesWriter_CheckConsistency(_PyBytesWriter *writer, char *str)
end = start + writer->allocated;
assert(str != NULL);
assert(start <= str && str <= end);
+
+ assert(writer->canary_byte == (char)0xAB);
return 1;
}
#endif
@@ -3665,6 +3668,10 @@ _PyBytesWriter_Finish(_PyBytesWriter *writer, void *str)
PyObject *result;
assert(_PyBytesWriter_CheckConsistency(writer, str));
+ if (writer->canary_byte != (char)0xAB) {
+ fprintf(stderr, "PyBytesWriter: buffer overflow detected! abort\n");
+ abort();
+ }
size = _PyBytesWriter_GetSize(writer, str);
if (size == 0 && !writer->use_bytearray) {I wrote a script to check for the buffer overflow in Python 3.14: writer_small_buffer = 512
ch = '\u20ac'
ch_encoded = ch.encode('latin1', 'xmlcharrefreplace')
repeat = writer_small_buffer - len(ch_encoded)
data = 'x' * repeat + ch
res = data.encode('latin1', 'xmlcharrefreplace')
print(res)
print(len(res))
Output: $ ./python x.py PyBytesWriter: buffer overflow detected! abort Abandon (core dumped)./python x So yes, Python 3.14, which uses the old internal _PyBytesWriter API, is also affected. In Python 3.14, _PyBytesWriter has its "small buffer" at the end of the structure. So the NUL byte write is actually a buffer overflow writing in the stack. It's quite bad :-( |
Sorry, something went wrong.
Write into a temporay buffer to not write the trailing NUL byte.
|
It seems like Python 3.10 to 3.16 are affected. (I didn't check older branches which no longer get security fixes.) |
Sorry, something went wrong.
|
@serhiy-storchaka: Would you mind to review this change? |
Sorry, something went wrong.
|
After looking at the code one more time, I decided that in fact, it's just a bugfix, not a security issue. In the worst case, the code writes a NUL byte on the stack after the writer (allocated on the stack). But in fact, this write cannot corrupt other stack variables nor change the return address. The write is just ignored and nothing is corrupted. |
Sorry, something went wrong.
|
On Python 3.14 and older, the write only occurs if the output length is exactly 512 bytes. On Python 3.15 and newer, the write only occurs if the output length is exactly 256 bytes. But on these Python versions, the write occurs in PyBytesObject.obj which is already a NULL pointer, so the write is harmless in practice. |
Sorry, something went wrong.
There was a problem hiding this comment.
What is the issue? We calculate the exact size of the result. Then PyBytesWriter should reserve the bytes object of that size (it is always followed by the terminating NUL). I do not see where we can make error in calculations.
BTW, using PyBytesWriter here is not needed. We know the size of the result, it is easier and faster to reserve the bytes object of that size.
Sorry, something went wrong.
len("�") is 4 bytes.
For a size less than or equal to 256 bytes, PyBytesWriter uses a small buffer of 256 bytes. There is no reserved space for a trailing NUL byte. Writing a trailing NUL byte causes a buffer overflow. See my previous comment for details. (In Python 3.14, the buffer overflow occurs with a buffer of 512 bytes.) I would prefer to not allow writing a trailing NUL byte in the PyBytesWriter API, since it would prevent implementing buffer overflow detection for example.
xmlcharrefreplace() is used by unicode_encode_ucs1() which uses PyBytesWriter. Using PyBytesWriter instead of allocating directly a bytes object should not have a signicant overhead. PyBytesWriter is convenient for error handlers which have to resize the bytes object. It's also convenient to use PyBytesWriter in all unicode_encode_ucs1() code paths to have the same API. IMO PyBytesWriter is a better API to create bytes objects. It implements additional checks in debug mode. It has some nice features like returning a singleton for an empty string or a single byte. PEP 782 soft deprecated PyBytes_FromStringAndSize(NULL, size) API. |
Sorry, something went wrong.
|
PyBytesWriter should either increase the size of that buffer to 2567, or use it only for a size less than or equal to 255 bytes. |
Sorry, something went wrong.
Reserving the last byte of the small buffer to allow writing a trailing NUL bytes prevents implementing buffer overflow detection. If we allow that, the buffer overflow detection raises an error since it detects a write outsize the allocated buffer. The C API of PyBytesObject allocates an extra byte for a trailing NUL byte. It allows overwriting the trailing NUL byte with... a NUL byte. It's an convenient feature, but it's currently undocumented. I don't think that we should allow writing an extra trailing NUL byte in the PyBytesWriter API. |
Sorry, something went wrong.
|
Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14, 3.15. |
Sorry, something went wrong.
|
GH-157232 is a backport of this pull request to the 3.15 branch. |
Sorry, something went wrong.
|
GH-157233 is a backport of this pull request to the 3.14 branch. |
Sorry, something went wrong.
|
GH-157234 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
|
I merged my change to fix the buffer overflow and unblock PR gh-156943 (which requires this fix). If needed, we can revisit the PyBytesWriter implementation later as soon as the API remains the same. |
Sorry, something went wrong.
Write into a temporary buffer to not write the trailing NUL byte into the writer. Previously, the NUL byte was written outsize the writer buffer.
Write into a temporary buffer to not write the trailing NUL byte into the writer. Previously, the NUL byte was written outsize the writer buffer.
…#157233) * gh-156939: Fix xmlcharrefreplace() buffer overflow (GH-157109) Write into a temporary buffer to not write the trailing NUL byte into the writer. Previously, the NUL byte was written outsize the writer buffer. (cherry picked from commit 9398655) Co-authored-by: Victor Stinner <vstinner@python.org> * Replace _Py_MAX_UNICODE with MAX_UNICODE --------- Co-authored-by: Victor Stinner <vstinner@python.org>
…#157234) * gh-156939: Fix xmlcharrefreplace() buffer overflow (GH-157109) Write into a temporary buffer to not write the trailing NUL byte into the writer. Previously, the NUL byte was written outsize the writer buffer. (cherry picked from commit 9398655) Co-authored-by: Victor Stinner <vstinner@python.org> * Replace _Py_MAX_UNICODE with MAX_UNICODE --------- Co-authored-by: Victor Stinner <vstinner@python.org>
| Back | FazBrowse Home | New Git URL |
Write into a temporay buffer to not write the trailing NUL byte.