Fix an off-by-one in loadscpt (!728) · Merge requests · OpenMW / openmw · GitLab

Fix an off-by-one in loadscpt

here is the trace I'm trying to fix:

jvoisin@grimhilde 18:52 ~/dev/openmw/openmw/build_fuzz_asan_master ./esmtool dump -C -p -q out/default/crashes/id\:000000\,sig\:06\,src\:000002\,time\:21965\,op\:havoc\,rep\:16          master *
Using default (English) font encoding.
Loading file: out/default/crashes/id:000000,sig:06,src:000002,time:21965,op:havoc,rep:16
=================================================================
==1382604==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x607000013a1e at pc 0x00000045ab66 bp 0x7ffd6d12ea70 sp 0x7ffd6d12e230
READ of size 1 at 0x607000013a1e thread T0
    #0 0x45ab65 in strlen (/home/jvoisin/dev/openmw/openmw/build_fuzz_asan_master/esmtool+0x45ab65)
    #1 0x840ec5 in std::char_traits<char>::length(char const*) /usr/lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/char_traits.h:357:9
    #2 0x840ec5 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string<std::allocator<char> >(char const*, std::allocator<char> const&) /usr/lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/basic_string.h:527:39
    #3 0x840ec5 in ESM::Script::loadSCVR(ESM::ESMReader&) /home/jvoisin/dev/openmw/openmw/components/esm/loadscpt.cpp:60:28
    #4 0x84b05b in ESM::Script::load(ESM::ESMReader&, bool&) /home/jvoisin/dev/openmw/openmw/components/esm/loadscpt.cpp:103:21
    #5 0x5112f5 in load(Arguments&) /home/jvoisin/dev/openmw/openmw/apps/esmtool/esmtool.cpp:389:21
    #6 0x50aa40 in main /home/jvoisin/dev/openmw/openmw/apps/esmtool/esmtool.cpp:216:20
    #7 0x7ff2d0ffacb1 in __libc_start_main csu/../csu/libc-start.c:314:16
    #8 0x44834d in _start (/home/jvoisin/dev/openmw/openmw/build_fuzz_asan_master/esmtool+0x44834d)

0x607000013a1e is located 0 bytes to the right of 78-byte region [0x6070000139d0,0x607000013a1e)
allocated by thread T0 here:
    #0 0x4f205d in operator new(unsigned long) (/home/jvoisin/dev/openmw/openmw/build_fuzz_asan_master/esmtool+0x4f205d)
    #1 0x83e11c in __gnu_cxx::new_allocator<char>::allocate(unsigned long, void const*) /usr/lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/ext/new_allocator.h:115:27
    #2 0x83e11c in std::allocator_traits<std::allocator<char> >::allocate(std::allocator<char>&, unsigned long) /usr/lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/alloc_traits.h:460:20
    #3 0x83e11c in std::_Vector_base<char, std::allocator<char> >::_M_allocate(unsigned long) /usr/lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/stl_vector.h:346:20
    #4 0x83e11c in std::_Vector_base<char, std::allocator<char> >::_M_create_storage(unsigned long) /usr/lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/stl_vector.h:361:33
    #5 0x83e11c in std::_Vector_base<char, std::allocator<char> >::_Vector_base(unsigned long, std::allocator<char> const&) /usr/lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/stl_vector.h:305:9
    #6 0x83e11c in std::vector<char, std::allocator<char> >::vector(unsigned long, std::allocator<char> const&) /usr/lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/stl_vector.h:511:9
    #7 0x83e11c in ESM::Script::loadSCVR(ESM::ESMReader&) /home/jvoisin/dev/openmw/openmw/components/esm/loadscpt.cpp:17:27
    #8 0x84b05b in ESM::Script::load(ESM::ESMReader&, bool&) /home/jvoisin/dev/openmw/openmw/components/esm/loadscpt.cpp:103:21
    #9 0x5112f5 in load(Arguments&) /home/jvoisin/dev/openmw/openmw/apps/esmtool/esmtool.cpp:389:21
    #10 0x50aa40 in main /home/jvoisin/dev/openmw/openmw/apps/esmtool/esmtool.cpp:216:20
    #11 0x7ff2d0ffacb1 in __libc_start_main csu/../csu/libc-start.c:314:16

SUMMARY: AddressSanitizer: heap-buffer-overflow (/home/jvoisin/dev/openmw/openmw/build_fuzz_asan_master/esmtool+0x45ab65) in strlen
Shadow bytes around the buggy address:
  0x0c0e7fffa6f0: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fa fa
  0x0c0e7fffa700: fa fa 00 00 00 00 00 00 00 00 00 03 fa fa fa fa
  0x0c0e7fffa710: 00 00 00 00 00 00 00 00 00 03 fa fa fa fa fd fd
  0x0c0e7fffa720: fd fd fd fd fd fd fd fd fa fa fa fa 00 00 00 00
  0x0c0e7fffa730: 00 00 00 00 00 03 fa fa fa fa 00 00 00 00 00 00
=>0x0c0e7fffa740: 00 00 00[06]fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0e7fffa750: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0e7fffa760: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0e7fffa770: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0e7fffa780: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0e7fffa790: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==1382604==ABORTING

but it seems that I'm missing something, since I can still trigger crashes in this area after this MR.

Here is the esp if you want to try: id_000016_sig_06_src_000151_time_122462_op_havoc_rep_8