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.

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

Memory overflow in core option system #5924

Closed
barbudreadmon opened this issue Dec 17, 2017 · 12 comments
Closed

Memory overflow in core option system #5924

barbudreadmon opened this issue Dec 17, 2017 · 12 comments

Comments

@barbudreadmon
Copy link
Contributor

barbudreadmon commented Dec 17, 2017

Description

Following libretro/fbalpha#162 , i think this is a retroarch issue, while doing my tests i ended up having the string /home/myusername/.config/retroarch/retroarch-core-options.cfg in my dkong.hi file, this path doesn't exist anywhere in the core code, so how could this be a memory overflow in the core code ?
Also, i tried building the core with address sanitizer (which i think is supposed to detect this kind of issue) and it didn't detect anything.

Expected behavior

Not having random string belonging to the core options system in the hiscore files, so they won't corrupt the game when loading.

Actual behavior

Random string appear in the hiscore files, they prevent the game to run properly.

Steps to reproduce the bug

  1. Copy any format of hiscore.dat to system_dir/fba
  2. Load dkong in fbalpha-libretro, enable hiscores, quit the game
  3. Do things that would populate your retroarch-core-options.cfg file (starting other games in fbalpha is a good way)
  4. Remove the dkong.hi file from system_dir/fba
  5. Start dkong again, quit it
  6. Open the generated dkong.hi file with an hex editor, you'll see random string obviously belonging to the core option system in it (sometime random content from it, ended up with the path once, which made me believe it's not a core issue), if it doesn't (it failed once in my tests) go back to 3 and keep going.

Bisect Results

I don't know, i tried an older release of retroarch (1.5.0) and it was the same

Version/Commit

Retroarch 1.6.9 release with current commit of fbalpha (also happen with older commit of fbalpha)

Environment information

  • OS: Linux x86_64
  • Compiler: gcc 6.4

Sorry in advance if it's not an issue in retroarch itself, but from my understanding :

  • hiscore code in fba is the same as upstream, and this issue doesn't happen upstream, memset is also properly executed in hiscore.cpp code.
  • if it was the other way around (hiscore buffer overflowing over the core option system), the retroarch-core-options.cfg would get corrupt with content from dkong.hi file and the dkong.hi file would be "clean" ?
@andres-asm
Copy link
Contributor

I think it's more likely that FBA is overflowing. Did the entries contain the original text followed by garbage?
make sure all your strings are null terminated.

@barbudreadmon
Copy link
Contributor Author

barbudreadmon commented Dec 17, 2017

The vector provided to RETRO_ENVIRONMENT_SET_VARIABLES is properly terminated by {NULL, NULL} .

Do you mean the string which define the name/values of the core options ? Hmmm some of them are strcopy'ed at some point, but all of them are handled by snprintf before going to the vector, which i thought was supposed to do this fine ?

Also, the fact is that most of the time, the string present in the dkong.hi file is related to core options, but unrelated to the current game itself (i end up seeing strings like "fba-macro-karnovr-Button_..." while karnovr is another game and i don't generate that specific string for the current game, but the string itself is present in my retroarch-core-options.cfg file, or as mentioned earlier, in one extreme case i saw the path of the file itself in dkong.hi), the only component which could be aware of those strings at that time is retroarch itself.

Edit : just to make things clear, the core options in fbalpha are generated at runtime on per-game basis for dipswitches and macros.

@andres-asm
Copy link
Contributor

No I was thinking the hi-score strings whenever they are populated.

@barbudreadmon
Copy link
Contributor Author

The hiscore buffer is directly handled by vanilla code from upstream, the only thing i do is defining the "szAppHiscorePath" variable (the path to the folder where hiscore.dat is stored, and where the game.hi files are generated). This code was already checked, it was someone from upstream who told me there was garbage in the game.hi files and it was some buffer overflow somewhere in the non-upstream code.

Also, i checked commits prior to the inclusion of the libretro-common file_stream overrides in fbalpha (i thought it could be messing up the upstream code which handle storing the hiscore to the file), same issue.

@andres-asm
Copy link
Contributor

Ahh... ok, well I'm kinda out of ideas hopefully someone else has some.

@barbudreadmon
Copy link
Contributor Author

Hmm i'm not really a C/C++ developper, upstream is not particularily aware of the specifics of libretro cores/frontend, perhaps it would be a good thing if someone knowledgable take a look at https://github.com/libretro/fbalpha/blob/master/src/burn/hiscore.cpp just to make sure we are not overlooking something.

@inactive123
Copy link
Contributor

@Alcaro can i ask for your expertise in this matter?

@ghost
Copy link

ghost commented Dec 18, 2017

Just build with ASAN?

@barbudreadmon
Copy link
Contributor Author

I'll try building retroarch with asan when i'm back home, that's actually pretty dumb of me to not try this since i did try asan on the core itself.

@Alcaro
Copy link
Contributor

Alcaro commented Dec 18, 2017

The only thing I can offer here is 'use asan or valgrind until you've got a stack trace'.

@barbudreadmon
Copy link
Contributor Author

I was probably drunk last time i ran dkong with asan. I tried again and found the following issue :

==28902==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000042391 at pc 0x7f34f3caef8c bp 0x7ffe42f96390 sp 0x7ffe42f96380
READ of size 1 at 0x602000042391 thread T0
    #0 0x7f34f3caef8b in HiscoreInit src/burn/hiscore.cpp:539
    #1 0x7f34f3ca4cef in BurnDrvInit src/burn/burn.cpp:622
    #2 0x7f34f5520fde in fba_init src/burner/libretro/libretro.cpp:1589
    #3 0x7f34f5521bdb in retro_load_game src/burner/libretro/libretro.cpp:1734
    #4 0x41aa10  (/usr/bin/retroarch+0x41aa10)
    #5 0x42e235  (/usr/bin/retroarch+0x42e235)
    #6 0x430987  (/usr/bin/retroarch+0x430987)
    #7 0x422a87  (/usr/bin/retroarch+0x422a87)
    #8 0x41d5fc  (/usr/bin/retroarch+0x41d5fc)
    #9 0x42d2f4  (/usr/bin/retroarch+0x42d2f4)
    #10 0x42fe8b  (/usr/bin/retroarch+0x42fe8b)
    #11 0x419b98  (/usr/bin/retroarch+0x419b98)
    #12 0x7f350bfdf4ef in __libc_start_main (/lib64/libc.so.6+0x204ef)
    #13 0x416fc9  (/usr/bin/retroarch+0x416fc9)
0x602000042391 is located 0 bytes to the right of 1-byte region [0x602000042390,0x602000042391)
allocated by thread T0 here:
    #0 0x7f35140a2600 in malloc (/usr/lib/gcc/x86_64-pc-linux-gnu/6.4.0/libasan.so+0xc7600)
    #1 0x7f34f3ca9161 in BurnMalloc(int) src/burn/burn_memory.cpp:29
    #2 0x7f34f3caedde in HiscoreInit src/burn/hiscore.cpp:532
    #3 0x7f34f3ca4cef in BurnDrvInit src/burn/burn.cpp:622
    #4 0x7f34f5520fde in fba_init src/burner/libretro/libretro.cpp:1589
    #5 0x7f34f5521bdb in retro_load_game src/burner/libretro/libretro.cpp:1734
    #6 0x41aa10  (/usr/bin/retroarch+0x41aa10)
SUMMARY: AddressSanitizer: heap-buffer-overflow src/burn/hiscore.cpp:539 in HiscoreInit
Shadow bytes around the buggy address:
  0x0c0480000420: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0480000430: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0480000440: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0480000450: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0480000460: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c0480000470: fa fa[01]fa fa fa 03 fa fa fa 01 fa fa fa 01 fa
  0x0c0480000480: fa fa 01 fa fa fa 01 fa fa fa 01 fa fa fa 01 fa
  0x0c0480000490: fa fa 03 fa fa fa fd fa fa fa 00 01 fa fa 00 fa
  0x0c04800004a0: fa fa 00 fa fa fa 00 fa fa fa fd fd fa fa 06 fa
  0x0c04800004b0: fa fa 06 fa fa fa 06 fa fa fa 05 fa fa fa 00 07
  0x0c04800004c0: fa fa 00 03 fa fa fd fd fa fa 02 fa fa fa 02 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
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  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
==28902==ABORTING

Funny thing (or not), if i disable everything related to libretro-common's file_stream_transforms, it fixes the issue. I'll link @albertofustinoni since i think he is responsible for this code.

@barbudreadmon
Copy link
Contributor Author

Had to hotfix libretro-common's file_stream_transform by myself since the previous code was buggy and the current one makes dkong crashes. The current commit of libretro-fbalpha probably don't work properly with some windows devices, but that's still better than having file corruption and settings not being saved properly for everyone.
Closing this issue and waiting for a proper fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants