FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-143379: fix UAF in struct.Struct.pack() when object modified by dunder methods by skirpichev · Pull Request #143382 · python/cpython · GitHub

Repository navigation

3 changes: 3 additions & 0 deletions Include/internal/pycore_pyatomic_ft_wrappers.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ extern "C" {
_Py_atomic_store_ulong_relaxed(&value, new_value)
#define FT_ATOMIC_STORE_SSIZE_RELAXED(value, new_value) \
_Py_atomic_store_ssize_relaxed(&value, new_value)
#define FT_ATOMIC_STORE_SSIZE(value, new_value) \
_Py_atomic_store_ssize(&value, new_value)
#define FT_ATOMIC_STORE_FLOAT_RELAXED(value, new_value) \
_Py_atomic_store_float_relaxed(&value, new_value)
#define FT_ATOMIC_LOAD_FLOAT_RELAXED(value) \
Expand Down Expand Up @@ -151,6 +153,7 @@ extern "C" {
#define FT_ATOMIC_STORE_INT8_RELAXED(value, new_value) value = new_value
#define FT_ATOMIC_STORE_INT8_RELEASE(value, new_value) value = new_value
#define FT_ATOMIC_STORE_SSIZE_RELAXED(value, new_value) value = new_value
#define FT_ATOMIC_STORE_SSIZE(value, new_value) value = new_value
#define FT_ATOMIC_STORE_SSIZE_RELEASE(value, new_value) value = new_value
#define FT_ATOMIC_STORE_UINT8_RELAXED(value, new_value) value = new_value
#define FT_ATOMIC_STORE_UINT16_RELAXED(value, new_value) value = new_value
Expand Down
15 changes: 14 additions & 1 deletion Lib/test/test_struct.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -579,7 +579,7 @@ def test_Struct_reinitialization(self):

def check_sizeof(self, format_str, number_of_codes):
# The size of 'PyStructObject'
totalsize = support.calcobjsize('2n3P')
totalsize = support.calcobjsize('2n3P1n')
# The size taken up by the 'formatcode' dynamic array
totalsize += struct.calcsize('P3n0P') * (number_of_codes + 1)
support.check_sizeof(self, struct.Struct(format_str), totalsize)
Expand Down Expand Up @@ -816,6 +816,19 @@ def test_endian_table_init_subinterpreters(self):
results = executor.map(exec, [code] * 5)
self.assertListEqual(list(results), [None] * 5)

def test_Struct_object_mutation_via_dunders(self):
S = struct.Struct('?I')
buf = array.array('b', b' '*100)

class Evil():
def __bool__(self):
# This rebuilds format codes during S.pack().
S.__init__('I')
return True

self.assertRaises(RuntimeError, S.pack, Evil(), 1)
self.assertRaises(RuntimeError, S.pack_into, buf, 0, Evil(), 1)


class UnpackIteratorTest(unittest.TestCase):
"""
Expand Down
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Fix use-after-free in :meth:`struct.Struct.pack` when the
:class:`struct.Struct` object is mutated by dunder methods (like
:meth:`object.__bool__`) during packing of arguments. Now this trigger
:exc:`RuntimeError`. Patch by Sergey B Kirpichev.
19 changes: 18 additions & 1 deletion Modules/_struct.c
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ typedef struct {
formatcode *s_codes;
PyObject *s_format;
PyObject *weakreflist; /* List of weak references */
Py_ssize_t mutex_cnt; /* to prevent mutation during packing */
} PyStructObject;

#define PyStructObject_CAST(op) ((PyStructObject *)(op))
Expand Down Expand Up @@ -1773,6 +1774,7 @@ s_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
s->s_codes = NULL;
s->s_size = -1;
s->s_len = -1;
s->mutex_cnt = 0;
}
return self;
}
Expand Down Expand Up @@ -1816,6 +1818,11 @@ Struct___init___impl(PyStructObject *self, PyObject *format)

Py_SETREF(self->s_format, format);

if (FT_ATOMIC_LOAD_SSIZE(self->mutex_cnt)) {
PyErr_SetString(PyExc_RuntimeError,
"Call Struct.__init__() in struct.pack()");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
"Call Struct.__init__() in struct.pack()");
"cannot call Struct.__init__() in struct.pack()");

return -1;
}
ret = prepare_s(self);
return ret;
}
Expand Down Expand Up @@ -2139,7 +2146,7 @@ Struct_iter_unpack_impl(PyStructObject *self, PyObject *buffer)
* argument for where to start processing the arguments for packing, and a
* character buffer for writing the packed string. The caller must insure
* that the buffer may contain the required length for packing the arguments.
* 0 is returned on success, 1 is returned if there is an error.
* 0 is returned on success, -1 is returned if there is an error.
*
*/
static int
Expand Down Expand Up @@ -2259,10 +2266,15 @@ s_pack(PyObject *self, PyObject *const *args, Py_ssize_t nargs)
char *buf = PyBytesWriter_GetData(writer);

/* Call the guts */
Py_ssize_t prev_cnt = FT_ATOMIC_LOAD_SSIZE(soself->mutex_cnt);

FT_ATOMIC_ADD_SSIZE(soself->mutex_cnt, 1);
if ( s_pack_internal(soself, args, 0, buf, state) != 0 ) {
FT_ATOMIC_STORE_SSIZE(soself->mutex_cnt, prev_cnt);
PyBytesWriter_Discard(writer);
return NULL;
}
FT_ATOMIC_STORE_SSIZE(soself->mutex_cnt, prev_cnt);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Would it work to use FT_ATOMIC_ADD_SSIZE(soself->mutex_cnt, -1); instead?

And then add maybe assert(soself->mutex_cnt >= 0);.


return PyBytesWriter_FinishWithSize(writer, soself->s_size);
}
Expand Down Expand Up @@ -2360,10 +2372,15 @@ s_pack_into(PyObject *self, PyObject *const *args, Py_ssize_t nargs)
}

/* Call the guts */
Py_ssize_t prev_cnt = FT_ATOMIC_LOAD_SSIZE(soself->mutex_cnt);

FT_ATOMIC_ADD_SSIZE(soself->mutex_cnt, 1);
if (s_pack_internal(soself, args, 2, (char*)buffer.buf + offset, state) != 0) {
FT_ATOMIC_STORE_SSIZE(soself->mutex_cnt, prev_cnt);
PyBuffer_Release(&buffer);
return NULL;
}
FT_ATOMIC_STORE_SSIZE(soself->mutex_cnt, prev_cnt);

PyBuffer_Release(&buffer);
Py_RETURN_NONE;
Expand Down
Loading

Back | FazBrowse Home | New Git URL