search

joncampbell123 / dosbox-x

DOSBox-X fork of the DOSBox project
GNU General Public License v2.0
2.53k stars 369 forks source link

SFT table can potentially be written out of bounds by DOS_MultiplexFunctions #5043

Closed weirddan455 closed 2 weeks ago

weirddan455 commented 2 weeks ago

Describe the bug

I was looking through your recent SFT changes and spotted a potential problem. You're allocating enough space for 5 entries here:

https://github.com/joncampbell123/dosbox-x/blob/6b42bac5b1cad48364cee8ec9b3e22bd82fad817/src/dos/dos_classes.cpp#L146-L150

But DOS_MultiplexFunctions can write up to 16 entires. It can also step on the "magic" CON strings needed by Windows 3.1.

https://github.com/joncampbell123/dosbox-x/blob/6b42bac5b1cad48364cee8ec9b3e22bd82fad817/src/dos/dos_misc.cpp#L148-L154

This is the problem I was trying to solve with Dunkle Schatten 2. I see you've moved the CON driver out of the way, which is what it was originally stepping on but there might still be memory corruption that can happen.

I decided to do a clean revert of my previous SFT commit and just allocate dedicated memory to DOS_MultiplexFunctions since it's kind of a hack anyway. That way it won't step on the "real" SFT table that Windows 3.1 depends on. Pending review, this is what Staging will go with: https://github.com/dosbox-staging/dosbox-staging/pull/3744

Steps to reproduce the behaviour

Don't have a clear repro case. This was just what I saw from reading DOSBox-X's code. Just wanted to share in case this becomes problematic for you.

Expected behavior

No response

What operating system(s) this bug have occurred on?

Linux

What version(s) of DOSBox-X have this bug?

tip of main

Used configuration

No response

Output log

No response

Additional information

No response

Have you checked that no similar bug report(s) exist?

Code of Conduct & Contributing Guidelines

joncampbell123 commented 2 weeks ago

I'll look at the code. It's probably assuming the initial SFT table is long enough (16 or more) not to care about the second SFT table, so the fix if I'm right is to modify that code to allow writing the first 16 with the SFT linked list and table sizes in mind.

joncampbell123 commented 2 weeks ago

Do you know of any program that uses that API?

weirddan455 commented 2 weeks ago

I know Dunkle Schatten 2 uses it. You could probably trigger the overrun if it exists by opening 10 or so file handles and then starting the game.

weirddan455 commented 2 weeks ago

Another interesting test would be to start Dunkle Schatten 2 on a clean start, close it, and then try to start Windows 3.1 to see if it overwrites the CON strings.

joncampbell123 commented 2 weeks ago

Looks like the code works as intended, when driven by DS2.