8000 py/modstruct: Check and prevent buffer-read overflow in struct unpacking · MicioMax/micropython@79d5acb · GitHub
[go: up one dir, main page]
Skip to content

Commit 79d5acb

Browse files
dpgeorgedpgeorge
committed
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.
1 parent 793d826 commit 79d5acb

File tree

3 files changed

+38
-29
lines changed

3 files changed

+38
-29
lines changed

‎py/modstruct.c

Lines changed: 26 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -82,35 +82,21 @@ STATIC mp_uint_t get_fmt_num(const char **p) {
8282
return val;
8383
}
8484

85-
STATIC uint calcsize_items(const char *fmt) {
86-
uint cnt = 0;
87-
while (*fmt) {
88-
int num = 1;
89-
if (unichar_isdigit(*fmt)) {
90-
num = get_fmt_num(&fmt);
91-
if (*fmt == 's') {
92-
num = 1;
93-
}
94-
}
95-
cnt += num;
96-
fmt++;
97-
}
98-
return cnt;
99-
}
100-
101-
STATIC mp_obj_t struct_calcsize(mp_obj_t fmt_in) {
102-
const char *fmt = mp_obj_str_get_str(fmt_in);
85+
STATIC size_t calc_size_items(const char *fmt, size_t *total_sz) {
10386
char fmt_type = get_fmt_type(&fmt);
104-
mp_uint_t size;
87+
size_t total_cnt = 0;
88+
size_t size;
10589
for (size = 0; *fmt; fmt++) {
10690
mp_uint_t cnt = 1;
10791
if (unichar_isdigit(*fmt)) {
10892
cnt = get_fmt_num(&fmt);
10993
}
11094

11195
if (*fmt == 's') {
96+
total_cnt += 1;
11297
size += cnt;
11398
} else {
99+
total_cnt += cnt;
114100
mp_uint_t align;
115101
size_t sz = mp_binary_get_size(fmt_type, *fmt, &align);
116102
while (cnt--) {
@@ -120,6 +106,14 @@ STATIC mp_obj_t struct_calcsize(mp_obj_t fmt_in) {
120106
}
121107
}
122108
}
109+
*total_sz = size;
110+
return total_cnt;
111+
}
112+
113+
STATIC mp_obj_t struct_calcsize(mp_obj_t fmt_in) {
114+
const char *fmt = mp_obj_str_get_str(fmt_in);
115+
size_t size;
116+
calc_size_items(fmt, &size);
123117
return MP_OBJ_NEW_SMALL_INT(size);
124118
}
125119
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) {
130124
// Since we implement unpack and unpack_from using the same function
131125
// we relax the "exact" requirement, and only implement "big enough".
132126
const char *fmt = mp_obj_str_get_str(args[0]);
127+
size_t total_sz;
128+
size_t num_items = calc_size_items(fmt, &total_sz);
133129
char fmt_type = get_fmt_type(&fmt);
134-
uint num_items = calcsize_items(fmt);
135130
mp_obj_tuple_t *res = MP_OBJ_TO_PTR(mp_obj_new_tuple(num_items, NULL));
136131
mp_buffer_info_t bufinfo;
137132
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) {
152147
p += offset;
153148
}
154149

155-
for (uint i = 0; i < num_items;) {
156-
mp_uint_t sz = 1;
150+
// Check that the input buffer is big enough to unpack all the values
151+
if (p + total_sz > end_p) {
152+
mp_raise_ValueError("buffer too small");
153+
}
154+
155+
for (size_t i = 0; i < num_items;) {
156+
mp_uint_t cnt = 1;
157157
if (unichar_isdigit(*fmt)) {
158-
sz = get_fmt_num(&fmt);
159-
}
160-
if (p + sz > end_p) {
161-
mp_raise_ValueError("buffer too small");
158+
cnt = get_fmt_num(&fmt);
162159
}
163160
mp_obj_t item;
164161
if (*fmt == 's') {
165-
item = mp_obj_new_bytes(p, sz);
166-
p += sz;
162+
item = mp_obj_new_bytes(p, cnt);
163+
p += cnt;
167164
res->items[i++] = item;
168165
} else {
169-
while (sz--) {
166+
while (cnt--) {
170167
item = mp_binary_get_val(fmt_type, *fmt, &p);
171168
res->items[i++] = item;
172169
}

‎tests/basics/struct1.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,12 @@
3939
# network byte order
4040
print(struct.pack('!i', 123))
4141

42+
# check that we get an error if the buffer is too small
43+
try:
44+
struct.unpack('I', b'\x00\x00\x00')
45+
except:
46+
print('struct.error')
47+
4248
# first arg must be a string
4349
try:
4450
struct.pack(1, 2)

‎tests/basics/struct2.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,12 @@
2525
print(struct.unpack('<0s1s0H2H', b'01234'))
2626
print(struct.pack('<0s1s0H2H', b'abc', b'abc', 258, 515))
2727

28+
# check that we get an error if the buffer is too small
29+
try:
30+
struct.unpack('2H', b'\x00\x00')
31+
except:
32+
print('Exception')
33+
2834
# check that unknown types raise an exception
2935
try:
3036
struct.unpack('z', b'1')

0 commit comments

Comments
 (0)
0