From 793d826d9df67c3f544505beda63ca36d8dfa1c4 Mon Sep 17 00:00:00 2001 From: Damien George Date: Fri, 1 Sep 2017 10:10:51 +1000 Subject: py/modstruct: In struct.pack, stop converting if there are no args left. This patch makes a repeat counter behave the same as repeating the typecode, when there are not enough args. For example: struct.pack('2I', 1) now behave the same as struct.pack('II', 1). --- py/modstruct.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'py/modstruct.c') diff --git a/py/modstruct.c b/py/modstruct.c index 1daa33338..0d4a45f6b 100644 --- a/py/modstruct.c +++ b/py/modstruct.c @@ -206,7 +206,8 @@ STATIC void struct_pack_into_internal(mp_obj_t fmt_in, byte *p, byte* end_p, siz memset(p + to_copy, 0, sz - to_copy); p += sz; } else { - while (sz--) { + // If we run out of args then we just finish; CPython would raise struct.error + while (sz-- && i < n_args) { mp_binary_set_val(fmt_type, *fmt, args[i++], &p); } } -- cgit v1.2.3 From 79d5acbd01fed21e1c8e46b06690072375de0bfe Mon Sep 17 00:00:00 2001 From: Damien George Date: Fri, 1 Sep 2017 10:53:29 +1000 Subject: py/modstruct: Check and prevent buffer-read overflow in struct unpacking Prior to this patch, the size of the buffer given to unpack/unpack_from was checked for being too small by using the count of the arguments, not their actual size. For example, a format spec of '4I' would only check that there was 4 bytes available, not 16; and 'I' would check for 1 byte, not 4. This bug is fixed in this patch by calculating the total size of the format spec at the start of the unpacking function. This function anyway needs to calculate the number of items at the start, so calculating the total size can be done at the same time. --- py/modstruct.c | 55 +++++++++++++++++++++++-------------------------- tests/basics/struct1.py | 6 ++++++ tests/basics/struct2.py | 6 ++++++ 3 files changed, 38 insertions(+), 29 deletions(-) (limited to 'py/modstruct.c') diff --git a/py/modstruct.c b/py/modstruct.c index 0d4a45f6b..61dd0f81b 100644 --- a/py/modstruct.c +++ b/py/modstruct.c @@ -82,26 +82,10 @@ STATIC mp_uint_t get_fmt_num(const char **p) { return val; } -STATIC uint calcsize_items(const char *fmt) { - uint cnt = 0; - while (*fmt) { - int num = 1; - if (unichar_isdigit(*fmt)) { - num = get_fmt_num(&fmt); - if (*fmt == 's') { - num = 1; - } - } - cnt += num; - fmt++; - } - return cnt; -} - -STATIC mp_obj_t struct_calcsize(mp_obj_t fmt_in) { - const char *fmt = mp_obj_str_get_str(fmt_in); +STATIC size_t calc_size_items(const char *fmt, size_t *total_sz) { char fmt_type = get_fmt_type(&fmt); - mp_uint_t size; + size_t total_cnt = 0; + size_t size; for (size = 0; *fmt; fmt++) { mp_uint_t cnt = 1; if (unichar_isdigit(*fmt)) { @@ -109,8 +93,10 @@ STATIC mp_obj_t struct_calcsize(mp_obj_t fmt_in) { } if (*fmt == 's') { + total_cnt += 1; size += cnt; } else { + total_cnt += cnt; mp_uint_t align; size_t sz = mp_binary_get_size(fmt_type, *fmt, &align); while (cnt--) { @@ -120,6 +106,14 @@ STATIC mp_obj_t struct_calcsize(mp_obj_t fmt_in) { } } } + *total_sz = size; + return total_cnt; +} + +STATIC mp_obj_t struct_calcsize(mp_obj_t fmt_in) { + const char *fmt = mp_obj_str_get_str(fmt_in); + size_t size; + calc_size_items(fmt, &size); return MP_OBJ_NEW_SMALL_INT(size); } MP_DEFINE_CONST_FUN_OBJ_1(struct_calcsize_obj, struct_calcsize); @@ -130,8 +124,9 @@ STATIC mp_obj_t struct_unpack_from(size_t n_args, const mp_obj_t *args) { // Since we implement unpack and unpack_from using the same function // we relax the "exact" requirement, and only implement "big enough". const char *fmt = mp_obj_str_get_str(args[0]); + size_t total_sz; + size_t num_items = calc_size_items(fmt, &total_sz); char fmt_type = get_fmt_type(&fmt); - uint num_items = calcsize_items(fmt); mp_obj_tuple_t *res = MP_OBJ_TO_PTR(mp_obj_new_tuple(num_items, NULL)); mp_buffer_info_t bufinfo; mp_get_buffer_raise(args[1], &bufinfo, MP_BUFFER_READ); @@ -152,21 +147,23 @@ STATIC mp_obj_t struct_unpack_from(size_t n_args, const mp_obj_t *args) { p += offset; } - for (uint i = 0; i < num_items;) { - mp_uint_t sz = 1; + // Check that the input buffer is big enough to unpack all the values + if (p + total_sz > end_p) { + mp_raise_ValueError("buffer too small"); + } + + for (size_t i = 0; i < num_items;) { + mp_uint_t cnt = 1; if (unichar_isdigit(*fmt)) { - sz = get_fmt_num(&fmt); - } - if (p + sz > end_p) { - mp_raise_ValueError("buffer too small"); + cnt = get_fmt_num(&fmt); } mp_obj_t item; if (*fmt == 's') { - item = mp_obj_new_bytes(p, sz); - p += sz; + item = mp_obj_new_bytes(p, cnt); + p += cnt; res->items[i++] = item; } else { - while (sz--) { + while (cnt--) { item = mp_binary_get_val(fmt_type, *fmt, &p); res->items[i++] = item; } diff --git a/tests/basics/struct1.py b/tests/basics/struct1.py index a442beb1e..2cf75137b 100644 --- a/tests/basics/struct1.py +++ b/tests/basics/struct1.py @@ -39,6 +39,12 @@ print(v == (10, 100, 200, 300)) # network byte order print(struct.pack('!i', 123)) +# check that we get an error if the buffer is too small +try: + struct.unpack('I', b'\x00\x00\x00') +except: + print('struct.error') + # first arg must be a string try: struct.pack(1, 2) diff --git a/tests/basics/struct2.py b/tests/basics/struct2.py index 3b9dd5c1f..e3336c0c7 100644 --- a/tests/basics/struct2.py +++ b/tests/basics/struct2.py @@ -25,6 +25,12 @@ print(struct.calcsize('0s1s0H2H')) print(struct.unpack('<0s1s0H2H', b'01234')) print(struct.pack('<0s1s0H2H', b'abc', b'abc', 258, 515)) +# check that we get an error if the buffer is too small +try: + struct.unpack('2H', b'\x00\x00') +except: + print('Exception') + # check that unknown types raise an exception try: struct.unpack('z', b'1') -- cgit v1.2.3 From 2daacc5ceef7e36ac3c88b8946088b6b3513993b Mon Sep 17 00:00:00 2001 From: Damien George Date: Fri, 1 Sep 2017 11:11:09 +1000 Subject: py/modstruct: Check and prevent buffer-write overflow in struct packing. Prior to this patch, the size of the buffer given to pack_into() was checked for being too small by using the count of the arguments, not their actual size. For example, a format spec of '4I' would only check that there was 4 bytes available, not 16; and 'I' would check for 1 byte, not 4. The pack() function is ok because its buffer is created to be exactly the correct size. The fix in this patch calculates the total size of the format spec at the start of pack_into() and verifies that the buffer is large enough. This adds some computational overhead, to iterate through the whole format spec. The alternative is to check during the packing, but that requires extra code to handle alignment, and the check is anyway not needed for pack(). So to maintain minimal code size the check is done using struct_calcsize. --- py/modstruct.c | 29 ++++++++++++++++------------- tests/basics/struct1.py | 6 ++++++ tests/basics/struct2.py | 4 ++++ 3 files changed, 26 insertions(+), 13 deletions(-) (limited to 'py/modstruct.c') diff --git a/py/modstruct.c b/py/modstruct.c index 61dd0f81b..8617a8e0d 100644 --- a/py/modstruct.c +++ b/py/modstruct.c @@ -174,37 +174,35 @@ STATIC mp_obj_t struct_unpack_from(size_t n_args, const mp_obj_t *args) { } MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(struct_unpack_from_obj, 2, 3, struct_unpack_from); -STATIC void struct_pack_into_internal(mp_obj_t fmt_in, byte *p, byte* end_p, size_t n_args, const mp_obj_t *args) { +// This function assumes there is enough room in p to store all the values +STATIC void struct_pack_into_internal(mp_obj_t fmt_in, byte *p, size_t n_args, const mp_obj_t *args) { const char *fmt = mp_obj_str_get_str(fmt_in); char fmt_type = get_fmt_type(&fmt); size_t i; for (i = 0; i < n_args;) { - mp_uint_t sz = 1; + mp_uint_t cnt = 1; if (*fmt == '\0') { // more arguments given than used by format string; CPython raises struct.error here break; } if (unichar_isdigit(*fmt)) { - sz = get_fmt_num(&fmt); - } - if (p + sz > end_p) { - mp_raise_ValueError("buffer too small"); + cnt = get_fmt_num(&fmt); } if (*fmt == 's') { mp_buffer_info_t bufinfo; mp_get_buffer_raise(args[i++], &bufinfo, MP_BUFFER_READ); - mp_uint_t to_copy = sz; + mp_uint_t to_copy = cnt; if (bufinfo.len < to_copy) { to_copy = bufinfo.len; } memcpy(p, bufinfo.buf, to_copy); - memset(p + to_copy, 0, sz - to_copy); - p += sz; + memset(p + to_copy, 0, cnt - to_copy); + p += cnt; } else { // If we run out of args then we just finish; CPython would raise struct.error - while (sz-- && i < n_args) { + while (cnt-- && i < n_args) { mp_binary_set_val(fmt_type, *fmt, args[i++], &p); } } @@ -219,8 +217,7 @@ STATIC mp_obj_t struct_pack(size_t n_args, const mp_obj_t *args) { vstr_init_len(&vstr, size); byte *p = (byte*)vstr.buf; memset(p, 0, size); - byte *end_p = &p[size]; - struct_pack_into_internal(args[0], p, end_p, n_args - 1, &args[1]); + struct_pack_into_internal(args[0], p, n_args - 1, &args[1]); return mp_obj_new_str_from_vstr(&mp_type_bytes, &vstr); } MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(struct_pack_obj, 1, MP_OBJ_FUN_ARGS_MAX, struct_pack); @@ -240,7 +237,13 @@ STATIC mp_obj_t struct_pack_into(size_t n_args, const mp_obj_t *args) { byte *end_p = &p[bufinfo.len]; p += offset; - struct_pack_into_internal(args[0], p, end_p, n_args - 3, &args[3]); + // Check that the output buffer is big enough to hold all the values + mp_int_t sz = MP_OBJ_SMALL_INT_VALUE(struct_calcsize(args[0])); + if (p + sz > end_p) { + mp_raise_ValueError("buffer too small"); + } + + struct_pack_into_internal(args[0], p, n_args - 3, &args[3]); return mp_const_none; } MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(struct_pack_into_obj, 3, MP_OBJ_FUN_ARGS_MAX, struct_pack_into); diff --git a/tests/basics/struct1.py b/tests/basics/struct1.py index 2cf75137b..db34342a1 100644 --- a/tests/basics/struct1.py +++ b/tests/basics/struct1.py @@ -69,6 +69,12 @@ print(buf) struct.pack_into('