[Nasm-bugs] [Bug 3392609] Stack buffer overflow found when specifying addresses and offset

[noreply-nasm at gorcunov.org]

https://bugzilla.nasm.us/show_bug.cgi?id=3392609

T Turek <tureqsec at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tureqsec at gmail.com

--- Comment #1 from T Turek <tureqsec at gmail.com> ---
Hi again,

I managed to analyze this issue a bit to try and help out with a fix. I was not
able to reproduce the issue when debugging with gdb and I'm still not sure why.
I removed the -O2 flag to prevent the code doing weird things and managed to
get a different output (there is different behaviour observed depending on this
flag):

```
$ ./ndisasm -u -o148h -s23eh -k 0,149h test.bin
4141414141414141  skipping 0x149 bytes
Segmentation fault (core dumped)
```

I verified in code that I have overwritten the offset variable. Digging further
I can see the following:

```   
...
p = q = buffer;
// nextsync is set to 0 here, offset is 328 and synclen 329
nextsync = next_sync(offset, &synclen);
do {
    // to_read is set to 64 here (buffer - p is 0 and sizeof(buffer) is 64)
    uint32_t to_read = buffer + sizeof(buffer) - p;
if ((nextsync || synclen) &&
    to_read > nextsync - offset - (p - q))
        // here to_read is recalculated to -328 and since
        // to_read is uint32_t this becomes a huge number
        to_read = nextsync - offset - (p - q);
    if (to_read) {
        // since to_read is huge, whole file is read, overwriting memory
        lenread = fread(p, 1, to_read, fp);
        if (lenread == 0)
            eof = true;     /* help along systems with bad feof */
    } else
        lenread = 0;
    p += lenread;
    ...
```

I'm not 100% sure what the cleanest fix for this would be, the quickest fix
I've made is to put a check for `nextsync - offset - (p - q))` before fread is
executed. Something like:

```
...
p = q = buffer;
nextsync = next_sync(offset, &synclen);
do {
    uint32_t to_read = buffer + sizeof(buffer) - p;
    // added code starts here
    int32_t check = nextsync - offset - (p - q);
    if (check < 0) {
        break;
    }
    // added code ends here
if ((nextsync || synclen) &&
    to_read > check)
        to_read = check;
    if (to_read) {
        lenread = fread(p, 1, to_read, fp);
    ...
```

Quick test shows that this approach would fix the issue but I'm not completely
sure if this breaks something else.

-- 
You are receiving this mail because:
You are watching all bug changes.
You are on the CC list for the bug.