10000 extmod/vfs_rom: Add bounds checking for all filesystem accesses. · sparkfun/micropython@14ba32b · GitHub
[go: up one dir, main page]
Skip to content

Commit 14ba32b

Browse files
dpgeorgedpgeorge
committed
extmod/vfs_rom: Add bounds checking for all filesystem accesses.
Testing with ROMFS shows that it is relatively easy to end up with a corrupt filesystem on the device -- eg due to the ROMFS deploy process stopping half way through -- which could lead to hard crashes. Notably, there can be boot loops trying to mount a corrupt filesystem, crashes when importing modules like `os` that first scan the filesystem for `os.py`, and crashing when deploying a new ROMFS in certain cases because the old one is removed while still mounted. The main problem is that `mp_decode_uint()` has an loop that keeps going as long as it reads 0xff byte values, which can happen in the case of erased and unwritten flash. This commit adds full bounds checking in the new `mp_decode_uint_checked()` function, and that makes all ROMFS filesystem accesses robust. Signed-off-by: Damien George <damien@micropython.org>
1 parent e3101ce commit 14ba32b

File tree

2 files changed

+163
-24
lines changed

2 files changed

+163
-24
lines changed

‎extmod/vfs_rom.c

Lines changed: 90 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,7 @@
9191
#define ROMFS_RECORD_KIND_DATA_POINTER (3)
9292
#define ROMFS_RECORD_KIND_DIRECTORY (4)
9393
#define ROMFS_RECORD_KIND_FILE (5)
94+
#define ROMFS_RECORD_KIND_FILESYSTEM (0x14a6b1)
9495

9596
typedef mp_uint_t record_kind_t;
9697

@@ -101,34 +102,72 @@ struct _mp_obj_vfs_rom_t {
101102
const uint8_t *filesystem_end;
102103
};
103104

104-
static record_kind_t extract_record(const uint8_t **fs, const uint8_t **fs_next) {
105-
record_kind_t record_kind = mp_decode_uint(fs);
106-
mp_uint_t record_len = mp_decode_uint(fs);
105+
// Returns 0 for success, -1 for failure.
106+
static int mp_decode_uint_checked(const uint8_t **ptr, const uint8_t *ptr_max, mp_uint_t *value_out) {
107+
mp_uint_t unum = 0;
108+
byte val;
109+
const uint8_t *p = *ptr;
110+
do {
111+
if (p >= ptr_max) {
112+
return -1;
113+
}
114+
val = *p++;
115+
unum = (unum << 7) | (val & 0x7f);
116+
} while ((val & 0x80) != 0);
117+
*ptr = p;
118+
*value_out = unum;
119+
return 0;
120+
}
121+
122+
static record_kind_t extract_record(const uint8_t **fs, const uint8_t **fs_next, const uint8_t *fs_max) {
123+
mp_uint_t record_kind;
124+
if (mp_decode_uint_checked(fs, fs_max, &record_kind) != 0) {
125+
return ROMFS_RECORD_KIND_UNUSED;
126+
}
127+
mp_uint_t record_len;
128+
if (mp_decode_uint_checked(fs, fs_max, &record_len) != 0) {
129+
return ROMFS_RECORD_KIND_UNUSED;
130+
}
107131
*fs_next = *fs + record_len;
108132
return record_kind;
109133
}
110134

111-
static void extract_data(mp_obj_vfs_rom_t *self, const uint8_t *fs, const uint8_t *fs_top, size_t *size_out, const uint8_t **data_out) {
112-
*size_out = 0;
113-
*data_out = NULL;
135+
// Returns 0 for success, a negative integer for failure.
136+
static int extract_data(mp_obj_vfs_rom_t *self, const uint8_t *fs, const uint8_t *fs_top, size_t *size_out, const uint8_t **data_out) {
114137
while (fs < fs_top) {
115138
const uint8_t *fs_next;
116-
record_kind_t record_kind = extract_record(&fs, &fs_next);
117-
if (record_kind == ROMFS_RECORD_KIND_DATA_VERBATIM) {
118-
// Verbatim data.
119-
*size_out = fs_next - fs;
120-
*data_out = fs;
139+
record_kind_t record_kind = extract_record(&fs, &fs_next, fs_top);
140+
if (record_kind == ROMFS_RECORD_KIND_UNUSED) {
141+
// Corrupt filesystem.
121142
break;
143+
} else if (record_kind == ROMFS_RECORD_KIND_DATA_VERBATIM) {
144+
// Verbatim data.
145+
if (size_out != NULL) {
146+
*size_out = fs_next - fs;
147+
*data_out = fs;
148+
}
149+
return 0;
122150
} else if (record_kind == ROMFS_RECORD_KIND_DATA_POINTER) {
123151
// Pointer to data.
124-
*size_out = mp_decode_uint(&fs);
125-
*data_out = self->filesystem + mp_decode_uint(&fs);
126-
break;
152+
mp_uint_t size;
153+
if (mp_decode_uint_checked(&fs, fs_next, &size) != 0) {
154+
break;
155+
}
156+
mp_uint_t offset;
157+
if (mp_decode_uint_checked(&fs, fs_next, &offset) != 0) {
158+
break;
159+
}
160+
if (size_out != NULL) {
161+
*size_out = size;
162+
*data_out = self->filesystem + offset;
163+
}
164+
return 0;
127165
} else {
128166
// Skip this record.
129167
fs = fs_next;
130168
}
131169
}
170+
return -MP_EIO;
132171
}
133172

134173
// Searches for `path` in the filesystem.
@@ -144,10 +183,17 @@ mp_import_stat_t mp_vfs_rom_search_filesystem(mp_obj_vfs_rom_t *self, const char
144183
}
145184
while (path_len > 0 && fs < fs_top) {
146185
const uint8_t *fs_next;
147-
record_kind_t record_kind = extract_record(&fs, &fs_next);
148-
if (record_kind == ROMFS_RECORD_KIND_DIRECTORY || record_kind == ROMFS_RECORD_KIND_FILE) {
186+
record_kind_t record_kind = extract_record(&fs, &fs_next, fs_top);
187+
if (record_kind == ROMFS_RECORD_KIND_UNUSED) {
188+
// Corrupt filesystem.
189+
return MP_IMPORT_STAT_NO_EXIST;
190+
} else if (record_kind == ROMFS_RECORD_KIND_DIRECTORY || record_kind == ROMFS_RECORD_KIND_FILE) {
149191
// A directory or file record.
150-
mp_uint_t name_len = mp_decode_uint(&fs);
192+
mp_uint_t name_len;
193+
if (mp_decode_uint_checked(&fs, fs_next, &name_len) != 0) {
194+
// Corrupt filesystem.
195+
return MP_IMPORT_STAT_NO_EXIST;
196+
}
151197
if ((name_len == path_len
152198
|| (name_len < path_len && path[name_len] == '/'))
153199
&& memcmp(path, fs, name_len) == 0) {
@@ -167,8 +213,9 @@ mp_import_stat_t mp_vfs_rom_search_filesystem(mp_obj_vfs_rom_t *self, const char
167213
if (path_len != 0) {
168214
return MP_IMPORT_STAT_NO_EXIST;
169215
}
170-
if (size_out != NULL) {
171-
extract_data(self, fs, fs_top, size_out, data_out);
216+
if (extract_data(self, fs, fs_top, size_out, data_out) != 0) {
217+
// Corrupt filesystem.
218+
return MP_IMPORT_STAT_NO_EXIST;
172219
}
173220
return MP_IMPORT_STAT_FILE;
174221
}
@@ -214,7 +261,15 @@ static mp_obj_t vfs_rom_make_new(const mp_obj_type_t *type, size_t n_args, size_
214261
}
215262

216263
// The ROMFS is a record itself, so enter into it and compute its limit.
217-
extract_record(&self->filesystem, &self->filesystem_end);
264+
record_kind_t record_kind = extract_record(&self->filesystem, &self->filesystem_end, self->filesystem + bufinfo.len);
265+
if (record_kind != ROMFS_RECORD_KIND_FILESYSTEM) {
266+
mp_raise_OSError(MP_ENODEV);
267+
}
268+
269+
// Check the filesystem is within the limits of the input buffer.
270+
if (self->filesystem_end > (const uint8_t *)bufinfo.buf + bufinfo.len) {
271+
mp_raise_OSError(MP_ENODEV);
272+
}
218273

219274
return MP_OBJ_FROM_PTR(self);
220275
}
@@ -259,13 +314,21 @@ static mp_obj_t vfs_rom_ilistdir_it_iternext(mp_obj_t self_in) {
259314

260315
while (self->index < self->index_top) {
261316
const uint8_t *index_next;
262-
record_kind_t record_kind = extract_record(&self->index, &index_next);
317+
record_kind_t record_kind = extract_record(&self->index, &index_next, self->index_top);
263318
uint32_t type;
264319
mp_uint_t name_len;
265320
size_t data_len;
266-
if (record_kind == ROMFS_RECORD_KIND_DIRECTORY || record_kind == ROMFS_RECORD_KIND_FILE) {
321+
if (record_kind == ROMFS_RECORD_KIND_UNUSED) {
322+
// Corrupt filesystem.
323+
self->index = self->index_top;
324+
break;
325+
} else if (record_kind == ROMFS_RECORD_KIND_DIRECTORY || record_kind == ROMFS_RECORD_KIND_FILE) {
267326
// A directory or file record.
268-
name_len = mp_decode_uint(&self->index);
327+
if (mp_decode_uint_checked(&self->index, index_next, &name_len) != 0) {
328+
// Corrupt filesystem.
329+
self->index = self->index_top;
330+
break;
331+
}
269332
if (record_kind == ROMFS_RECORD_KIND_DIRECTORY) {
270333
// A directory.
271334
type = MP_S_IFDIR;
@@ -274,7 +337,10 @@ static mp_obj_t vfs_rom_ilistdir_it_iternext(mp_obj_t self_in) {
274337
// A file.
275338
type = MP_S_IFREG;
276339
const uint8_t *data_value;
277-
extract_data(self->vfs_rom, self->index + name_len, index_next, &data_len, &data_value);
340+
if (extract_data(self->vfs_rom, self->index + name_len, index_next, &data_len, &data_value) != 0) {
341+
// Corrupt filesystem.
342+
break;
343+
}
278344
}
279345
} else {
280346
// Skip this record.

‎tests/extmod/vfs_rom.py

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,79 @@ def test_unknown_record(self):
223223
self.assertEqual(f.read(), b"contents")
224224

225225

226+
class TestCorrupt(unittest.TestCase):
227+
def test_corrupt_filesystem(self):
228+
# Make the filesystem length bigger than the buffer.
229+
romfs = bytearray(make_romfs(()))
230+
romfs[3] = 0x01
231+
with self.assertRaises(OSError):
232+
vfs.VfsRom(romfs)
233+
234+
# Corrupt the filesystem length.
235+
romfs = bytearray(make_romfs(()))
236+
romfs[3] = 0xFF
237+
with self.assertRaises(OSError):
238+
vfs.VfsRom(romfs)
239+
240+
# Corrupt the contents of the filesystem.
241+
romfs = bytearray(make_romfs(()))
242+
romfs[3] = 0x01
243+
romfs.extend(b"\xff\xff")
244+
fs = vfs.VfsRom(romfs)
245+
with self.assertRaises(OSError):
246+
fs.stat("a")
247+
self.assertEqual(list(fs.ilistdir("")), [])
248+
249+
def test_corrupt_file_entry(self):
250+
romfs = make_romfs((("file", b"data"),))
251+
252+
# Corrupt the length of filename.
253+
romfs_corrupt = bytearray(romfs)
254+
romfs_corrupt[7:] = b"\xff" * (len(romfs) - 7)
255+
fs = vfs.VfsRom(romfs_corrupt)
256+
with self.assertRaises(OSError):
257+
fs.stat("file")
258+
self.assertEqual(list(fs.ilistdir("")), [])
259+
260+
# Erase the data record (change it to a padding record).
261+
romfs_corrupt = bytearray(romfs)
262+
romfs_corrupt[12] = VfsRomWriter.ROMFS_RECORD_KIND_PADDING
263+
fs = vfs.VfsRom(romfs_corrupt)
264+
with self.assertRaises(OSError):
265+
fs.stat("file")
266+
self.assertEqual(list(fs.ilistdir("")), [])
267+
268+
# Corrupt the header of the data record.
269+
romfs_corrupt = bytearray(romfs)
270+
romfs_corrupt[12:] = b"\xff" * (len(romfs) - 12)
271+
fs = vfs.VfsRom(romfs_corrupt)
272+
with self.assertRaises(OSError):
273+
fs.stat("file")
274+
275+
# Corrupt the interior of the data record.
276+
romfs_corrupt = bytearray(romfs)
277+
romfs_corrupt[13:] = b"\xff" * (len(romfs) - 13)
278+
fs = vfs.VfsRom(romfs_corrupt)
279+
with self.assertRaises(OSError):
280+
fs.stat("file")
281+
282+
# Change the data record to an indirect pointer and corrupt the length.
283+
romfs_corrupt = bytearray(romfs)
284+
romfs_corrupt[12] = VfsRomWriter.ROMFS_RECORD_KIND_DATA_POINTER
285+
romfs_corrupt[14:18] = b"\xff\xff\xff\xff"
286+
fs = vfs.VfsRom(romfs_corrupt)
287+
with self.assertRaises(OSError):
288+
fs.stat("file")
289+
290+
# Change the data record to an indirect pointer and corrupt the offset.
291+
romfs_corrupt = bytearray(romfs)
292+
romfs_corrupt[12] = VfsRomWriter.ROMFS_RECORD_KIND_DATA_POINTER
293+
romfs_corrupt[14:18] = b"\x00\xff\xff\xff"
294+
fs = vfs.VfsRom(romfs_corrupt)
295+
with self.assertRaises(OSError):
296+
fs.stat("file")
297+
298+
226299
class TestStandalone(TestBase):
227300
def test_constructor(self):
228301
self.assertIsInstance(vfs.VfsRom(self.romfs), vfs.VfsRom)

0 commit comments

Comments
 (0)
0