gh-156939: Fix xmlcharrefreplace() buffer overflow

[vstinner]

Write into a temporay buffer to not write the trailing NUL byte.

• Issue: C API: Detect buffer overflow in PyBytesWriter #156939

[vstinner]

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 :-(

[vstinner]

It seems like Python 3.10 to 3.16 are affected. (I didn't check older branches which no longer get security fixes.)

[vstinner]

@serhiy-storchaka: Would you mind to review this change?

[vstinner]

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.

[vstinner]

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.

[serhiy-storchaka]

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.

[vstinner]

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.

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.

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.

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.

[serhiy-storchaka]

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.

[vstinner]

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.

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.

[miss-islington-app]

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14, 3.15.
🐍🍒⛏🤖

[bedevere-app]

GH-157232 is a backport of this pull request to the 3.15 branch.

[bedevere-app]

GH-157233 is a backport of this pull request to the 3.14 branch.

[bedevere-app]

GH-157234 is a backport of this pull request to the 3.13 branch.

[vstinner]

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.