Buffer overflow in kwajd_read_headers()
jwilk opened this issue · 3 comments
Apparently buffer overflow is possible in the kwajd_read_headers function:
$ printf 'KWAJ\210\360\47\3210000\377%07d\0\0%013d' > overflow.kwaj
$ valgrind -q msexpand overflow.kwaj /dev/null
==26911== Invalid write of size 1
==26911== at 0x4840E2A: kwajd_read_headers (kwajd.c:221)
==26911== by 0x4840FD6: kwajd_open (kwajd.c:108)
==26911== by 0x4842264: kwajd_decompress (kwajd.c:334)
==26911== by 0x1088A7: main (msexpand.c:34)
==26911== Address 0x4a1b555 is 0 bytes after a block of size 13 alloc'd
==26911== at 0x482E2BC: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==26911== by 0x483BF46: msp_alloc (system.c:208)
==26911== by 0x4840D5C: kwajd_read_headers (kwajd.c:202)
==26911== by 0x4840FD6: kwajd_open (kwajd.c:108)
==26911== by 0x4842264: kwajd_decompress (kwajd.c:334)
==26911== by 0x1088A7: main (msexpand.c:34)
==26911==
==26911== Invalid write of size 1
==26911== at 0x4840E67: kwajd_read_headers (kwajd.c:226)
==26911== by 0x4840FD6: kwajd_open (kwajd.c:108)
==26911== by 0x4842264: kwajd_decompress (kwajd.c:334)
==26911== by 0x1088A7: main (msexpand.c:34)
==26911== Address 0x4a1b556 is 1 bytes after a block of size 13 alloc'd
==26911== at 0x482E2BC: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==26911== by 0x483BF46: msp_alloc (system.c:208)
==26911== by 0x4840D5C: kwajd_read_headers (kwajd.c:202)
==26911== by 0x4840FD6: kwajd_open (kwajd.c:108)
==26911== by 0x4842264: kwajd_decompress (kwajd.c:334)
==26911== by 0x1088A7: main (msexpand.c:34)
==26911==
Tested against git master (6037317).
Found using American Fuzzy Lop:
http://lcamtuf.coredump.cx/afl/
Thanks for the report!
This is in the logic for reading an 8.3 filename.ext, just looking over it I don't like how it's written and it could be better. The off-by-one could be avoided by allocating one or two more bytes for the filename, but I'd rather re-examine this code and check its logic is sound at the same time.
I'll check some test files that are intentionally bad (no null terminating byte in the filename, no null terminating byte in the extension), but another sample file would help. Could you supply the sample file that triggered this bug?