Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Stack buffer over-read in ReadRawLanguageStrings #7600

Closed
JGRennison opened this issue May 22, 2019 · 0 comments
Closed

Stack buffer over-read in ReadRawLanguageStrings #7600

JGRennison opened this issue May 22, 2019 · 0 comments

Comments

@JGRennison
Copy link
Contributor

Version of OpenTTD

Since e804173

Expected result

No stack buffer overread

Actual result

ret->lines.emplace_back(buffer, buffer + to_read - 1); in ReadRawLanguageStrings()
reads and initialises the string with data beyond the bounds of buffer in the case where the translation file size > 2049 bytes.
to_read is the nominal remaining file size, not the buffer content size.
Suggested change: ret->lines.emplace_back(buffer, buffer + i)

Steps to reproduce

Load a GameScript with AddressSanitizer enabled

AddressSanitizer log;

READ of size 3648 at 0x7ffc2b474370 thread T0
    #0 0x7f95c4d81732  (/usr/lib/x86_64-linux-gnu/libasan.so.4+0x79732)
    #1 0x55b91002ab7f in std::char_traits<char>::copy(char*, char const*, unsigned long) /usr/include/c++/7/bits/char_traits.h:350
    #2 0x55b91002ab7f in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_S_copy(char*, char const*, unsigned long) /usr/include/c++/7/bits/basic_string.h:340
    #3 0x55b91002ab7f in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_S_copy_chars(char*, char const*, char const*) /usr/include/c++/7/bits/basic_string.h:387
    #4 0x55b91002ab7f in void std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>(char const*, char const*, std::forward_iterator_tag) /usr/include/c++/7/bits/basic_string.tcc:225
    #5 0x55b910040895 in void std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct_aux<char*>(char*, char*, std::__false_type) /usr/include/c++/7/bits/basic_string.h:236
    #6 0x55b910040895 in void std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char*>(char*, char*) /usr/include/c++/7/bits/basic_string.h:255
    #7 0x55b910040895 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string<char*, void>(char*, char*, std::allocator<char> const&) /usr/include/c++/7/bits/basic_string.h:607
    #8 0x55b910040895 in void __gnu_cxx::new_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >::construct<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, char (&) [2048], char*>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, char (&) [2048], char*&&) /usr/include/c++/7/ext/new_allocator.h:136
    #9 0x55b910040895 in void std::allocator_traits<std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::construct<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, char (&) [2048], char*>(std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, char (&) [2048], char*&&) /usr/include/c++/7/bits/alloc_traits.h:475
    #10 0x55b910040895 in void std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::_M_realloc_insert<char (&) [2048], char*>(__gnu_cxx::__normal_iterator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > >, char (&) [2048], char*&&) /usr/include/c++/7/bits/vector.tcc:415
    #11 0x55b91002fcd5 in void std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::emplace_back<char (&) [2048], char*>(char (&) [2048], char*&&) /usr/include/c++/7/bits/vector.tcc:105
    #12 0x55b91002fcd5 in ReadRawLanguageStrings(char const*) /home/jgr/openttd/trunk4/src/game/game_text.cpp:114
    #13 0x55b910033eb1 in LoadTranslations() /home/jgr/openttd/trunk4/src/game/game_text.cpp:263
    #14 0x55b910039830 in RegisterGameTranslation(Squirrel*) /home/jgr/openttd/trunk4/src/game/game_text.cpp:350
    #15 0x55b90fff2ed5 in GameInstance::RegisterAPI() /home/jgr/openttd/trunk4/src/game/game_instance.cpp:220
    #16 0x55b911541957 in ScriptInstance::Initialize(char const*, char const*, Owner) /home/jgr/openttd/trunk4/src/script/script_instance.cpp:78
    #17 0x55b90ffa5e32 in Game::StartNew() /home/jgr/openttd/trunk4/src/game/game_core.cpp:92
    #18 0x55b911234b39 in Load_GSDT /home/jgr/openttd/trunk4/src/saveload/game_sl.cpp:104
    #19 0x55b911290d55 in SlLoadChunk /home/jgr/openttd/trunk4/src/saveload/saveload.cpp:1856
    #20 0x55b911290d55 in SlLoadChunks /home/jgr/openttd/trunk4/src/saveload/saveload.cpp:2051
    #21 0x55b911290d55 in DoLoad /home/jgr/openttd/trunk4/src/saveload/saveload.cpp:2944
    #22 0x55b91129bff8 in SaveOrLoad(char const*, SaveLoadOperation, DetailedFileType, Subdirectory, bool) /home/jgr/openttd/trunk4/src/saveload/saveload.cpp:3070
    #23 0x55b910cc697a in SafeLoad(char const*, SaveLoadOperation, DetailedFileType, GameMode, Subdirectory, LoadFilter*) /home/jgr/openttd/trunk4/src/openttd.cpp:1102
    #24 0x55b910cc6e27 in SwitchToMode(SwitchMode) /home/jgr/openttd/trunk4/src/openttd.cpp:1186
    #25 0x55b910cde4c8 in GameLoop() /home/jgr/openttd/trunk4/src/openttd.cpp:1659
    #26 0x55b9120f2940 in VideoDriver_SDL::MainLoop() /home/jgr/openttd/trunk4/src/video/sdl_v.cpp:754
    #27 0x55b910ccd3c8 in openttd_main(int, char**) /home/jgr/openttd/trunk4/src/openttd.cpp:924
    #28 0x7f95c1dc9b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
    #29 0x55b90f3d58d9 in _start (/home/jgr/openttd/trunk4/bin/openttd+0x483b8d9)

Address 0x7ffc2b474370 is located in stack of thread T0 at offset 2656 in frame
    #0 0x55b91002f2cf in ReadRawLanguageStrings(char const*) /home/jgr/openttd/trunk4/src/game/game_text.cpp:85

  This frame has 10 object(s):
    [32, 33) '<unknown>'
    [96, 97) '<unknown>'
    [160, 161) '<unknown>'
    [224, 232) 'to_read'
    [288, 296) 'fhClose'
    [352, 360) 'ret'
    [416, 424) '<unknown>'
    [480, 488) '__p'
    [544, 552) '<unknown>'
    [608, 2656) 'buffer' <== Memory access at offset 2656 overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow (/usr/lib/x86_64-linux-gnu/libasan.so.4+0x79732) 
Shadow bytes around the buggy address:
  0x100005686810: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100005686820: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100005686830: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100005686840: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100005686850: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x100005686860: 00 00 00 00 00 00 00 00 00 00 00 00 00 00[f3]f3
  0x100005686870: f3 f3 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100005686880: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100005686890: 00 00 00 00 00 00 f1 f1 f1 f1 01 f2 f2 f2 f2 f2
  0x1000056868a0: f2 f2 01 f2 f2 f2 f2 f2 f2 f2 01 f2 f2 f2 f2 f2
  0x1000056868b0: f2 f2 01 f2 f2 f2 f2 f2 f2 f2 01 f2 f2 f2 f2 f2
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
==24548==ABORTING
michicc added a commit to michicc/OpenTTD that referenced this issue May 22, 2019
@michicc
Fix OpenTTD#7600: Don't read uninitialised memory when parsing GS lan…
88271ff 
…guage files.
douiwby pushed a commit to douiwby/OpenTTD that referenced this issue Apr 16, 2020
@michicc @douiwby
Fix OpenTTD#7600: Don't read uninitialised memory when parsing GS lan…
55f6ddf 
…guage files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@JGRennison