Buffer overrun when asking Sulik what weapons he can use

[grahamgower]

Using the Linux x64 v1.3 release binary, I get a crash after: paying Sulik $350, asking him to join me, then asking him what weapons he can use.

*** buffer overflow detected ***: terminated
Aborted (core dumped)

It looks like the 8 char string "slk58alt" is being copied into char field_50[8]; using strcpy, so the string's 0-terminator overruns the buffer. The release is built with -D_FORTIFY_SOURCE=2, so this terminates the program.

(gdb) bt
#0  0x0000770ec05bf32c in ?? () from /usr/lib/libc.so.6
#1  0x0000770ec056e6c8 in raise () from /usr/lib/libc.so.6
#2  0x0000770ec05564b8 in abort () from /usr/lib/libc.so.6
#3  0x0000770ec0557395 in ?? () from /usr/lib/libc.so.6
#4  0x0000770ec064675b in __fortify_fail () from /usr/lib/libc.so.6
#5  0x0000770ec0646106 in __chk_fail () from /usr/lib/libc.so.6
#6  0x0000770ec0647b97 in __strcpy_chk () from /usr/lib/libc.so.6
#7  0x00005ee178bc95d1 in strcpy (__src=0x7ffc8bf71940 "slk58alt", __dest=0x5ee178c9072c <fallout::gLipsData+108> "SLK10A")
    at /usr/include/x86_64-linux-gnu/bits/string_fortified.h:90
#8  fallout::lipsLoad (audioFileName=audioFileName@entry=0x5ee17a93e080 "slk58alt", headFileName=headFileName@entry=0x7ffc8bf71bb0 "sulik")
    at /home/runner/work/fallout2-ce/fallout2-ce/src/lips.cc:264
#9  0x00005ee178b8f4bf in fallout::gameDialogStartLips (audioFileName=0x5ee17a93e080 "slk58alt") at /home/runner/work/fallout2-ce/fallout2-ce/src/game_dialog.cc:838
#10 0x00005ee178bf8775 in fallout::_scr_get_msg_str_speech (messageListId=383, messageId=820, a3=1) at /home/runner/work/fallout2-ce/fallout2-ce/src/scripts.cc:2764
#11 0x00005ee178bfc2fd in fallout::_scr_get_msg_str_speech (messageListId=<optimized out>, messageId=<optimized out>, a3=a3@entry=1)
    at /home/runner/work/fallout2-ce/fallout2-ce/src/scripts.cc:2773
#12 0x00005ee178b8d682 in fallout::_gdProcessUpdate () at /home/runner/work/fallout2-ce/fallout2-ce/src/game_dialog.cc:2236
#13 0x00005ee178b90a1a in fallout::_gdProcessChoice (a1=0) at /home/runner/work/fallout2-ce/fallout2-ce/src/game_dialog.cc:2088
#14 0x00005ee178b91c2f in fallout::_gdProcess () at /home/runner/work/fallout2-ce/fallout2-ce/src/game_dialog.cc:1989
#15 fallout::_gdialogGo () at /home/runner/work/fallout2-ce/fallout2-ce/src/game_dialog.cc:1181
#16 0x00005ee178ba4fb4 in fallout::_op_gsay_end (program=0x5ee17aa63990) at /home/runner/work/fallout2-ce/fallout2-ce/src/interpreter_extra.cc:3772
#17 0x00005ee178bb0abd in fallout::_interpret (program=program@entry=0x5ee17aa63990, a2=-482, a2@entry=-1)
    at /home/runner/work/fallout2-ce/fallout2-ce/src/interpreter.cc:2691
#18 0x00005ee178bb5865 in fallout::_executeProcedure (program=program@entry=0x5ee17aa63990, procedureIndex=procedureIndex@entry=8)
    at /home/runner/work/fallout2-ce/fallout2-ce/src/interpreter.cc:2890
#19 0x00005ee178bfa82c in fallout::scriptExecProc (sid=sid@entry=67126961, proc=proc@entry=11) at /home/runner/work/fallout2-ce/fallout2-ce/src/scripts.cc:1338
#20 0x00005ee178b8fead in fallout::gameDialogEnter (speaker=0x5ee17a81ee50, a2=a2@entry=0) at /home/runner/work/fallout2-ce/fallout2-ce/src/game_dialog.cc:740
#21 0x00005ee178bf910f in fallout::scriptsHandleRequests () at /home/runner/work/fallout2-ce/fallout2-ce/src/scripts.cc:1009
#22 0x00005ee178bd0329 in fallout::mainLoop () at /home/runner/work/fallout2-ce/fallout2-ce/src/main.cc:370
#23 0x00005ee178bd0f35 in fallout::falloutMain (argc=<optimized out>, argv=<optimized out>) at /home/runner/work/fallout2-ce/fallout2-ce/src/main.cc:190
#24 0x0000770ec0557cd0 in ?? () from /usr/lib/libc.so.6
#25 0x0000770ec0557d8a in __libc_start_main () from /usr/lib/libc.so.6
#26 0x00005ee178b4b84e in _start ()

[phobos2077]

This looks like a vanilla issue. kcsulik.msg has this line:

{820}{slk58alt}{We be good with fists, spear, big hammer or sub-machine gun. We be a handful.}

Probably never caused issues in original because there were no buffer overrun checks :)

The only place this field is used is when calling lips_fix_string which also accepts length parameter, that's why this 1-char overrun never broke the game. Not really sure what to do about it.

In original code, there's no strcpy but what appears to be an inlined custom string copy function:

0047B0A3 loc_47B0A3:                                  ; lips_load_file_+F2�j
0047B0A3            lea     esi, [esp+12Ch+var_28]
0047B0AA            mov     edi, offset aTest         ; "TEST"
0047B0AF            mov     edx, 8
0047B0B4            push    edi
0047B0B5
0047B0B5 loc_47B0B5:                                  ; lips_load_file_+11F�j
0047B0B5            mov     al, [esi]
0047B0B7            mov     [edi], al
0047B0B9            cmp     al, 0
0047B0BB            jz      short loc_47B0CD
0047B0BD            mov     al, [esi+1]
0047B0C0            add     esi, 2
0047B0C3            mov     [edi+1], al
0047B0C6            add     edi, 2
0047B0C9            cmp     al, 0
0047B0CB            jnz     short loc_47B0B5

Worth examining. it seems to be just a loop that copies 2 characters per iteration, nothing special.

[grahamgower]

Maybe just copy 8 chars? Is there anything that relies on the null terminator?

[grahamgower]

This is a duplicate of #342.

[grahamgower]

Another dupe: #316