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

[MU4 Issue] Stack buffer overflow vulnerability while parse MIDI file #16346

Closed
kunshim opened this issue Feb 13, 2023 · 8 comments · Fixed by #16348
Closed

[MU4 Issue] Stack buffer overflow vulnerability while parse MIDI file #16346

kunshim opened this issue Feb 13, 2023 · 8 comments · Fixed by #16348

Comments

@kunshim
Copy link

kunshim commented Feb 13, 2023

Describe the bug
It only affects the Windows version.
In src/importexport/midi/internal/midishared/midifile.cpp

void MidiFile::skip(qint64 len)
{
    // Note: if MS is updated to use Qt 5.10, this can be implemented with QIODevice::skip(), which should be more efficient
    //       as bytes do not need to be moved around.
    if (len <= 0) {
        return;
    }
#if (!defined (_MSCVER) && !defined (_MSC_VER))
    char tmp[len];
    read(tmp, len);
#else
    const int tmp_size = 256;    // Size of fixed-length temporary buffer. MSVC does not support VLA.
    char tmp[tmp_size];
    while (len > tmp_size) {
        read(tmp, len); //vulnerability 
        len -= tmp_size;
    }
    // Now len is <= tmp_size, last read fits in the buffer.
    read(tmp, tmp_size);
#endif
}

It is copying the length of len, not tmp_size. If len is bigger than 256, it will overwrite stack.
This leads to a buffer overflow and overwrite the stack cookies. If an attacker knows the stack cookie value, it can lead to code execution. Check PoC.midi in PoC.zip.
This vulnerability works from version 3.0 to 4.0.1(latest)

image
PoC.zip

If you have any problems, contact me via kunshim@naver.com

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Feb 13, 2023

How about this then:

#if (!defined (_MSCVER) && !defined (_MSC_VER))
      char tmp[len];
      read(tmp, len);
#else
      // MSVC does not support VLA. Replace with std::vector. If profiling determines that the
      //    heap allocation is slow, an optimization might be used.
      std::vector<char> tmp(len);
      read(&tmp, len);
#endif

Or

#if (!defined (_MSCVER) && !defined (_MSC_VER))
      char tmp[len];
#else
      // MSVC does not support VLA. Replace with std::vector. If profiling determines that the
      //    heap allocation is slow, an optimization might be used.
      std::vector<char> buffer(len);
      char *tmp = buffer.data();
#endif
      read(tmp, len);
      }

(I like that one better, it is done that way in various different places, PR in the works)

@kunshim
Copy link
Author

kunshim commented Feb 13, 2023

Pretty good!
I can't built and test with the source code right away, but the bug is expected to be patched.

@Jojo-Schmitz
Copy link
Contributor

Another possible fix would be:

#if (!defined (_MSCVER) && !defined (_MSC_VER))
    char tmp[len];
    read(tmp, len);
#else
    const int tmp_size = 256;    // Size of fixed-length temporary buffer. MSVC does not support VLA.
    char tmp[tmp_size];
    while (len > tmp_size) {
        read(tmp, tmp_size);
        len -= tmp_size;
    }
    // Now len is <= tmp_size, last read fits in the buffer.
    read(tmp, len);
#endif
}

I.e. in the loop read tmp_size bytes and after the loop read (the remaining) len bytes rather than the other way round. I guess that was the original intention, back then when we moved from MinGW to MSVC.
@anatoly-os?

@cbjeukendrup
Copy link
Contributor

And what about that comment at the top of the function?

    // Note: if MS is updated to use Qt 5.10, this can be implemented with QIODevice::skip(), which should be more efficient
    //       as bytes do not need to be moved around.

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Feb 13, 2023

Good question...
So something like this:

void MidiFile::skip(qint64 len)
{
    if (len <= 0) {
        return;
    }
    fp->skip(len);
}

Updated the PR

@Jojo-Schmitz
Copy link
Contributor

Further simplified it, eliminating that function entirely

@kunshim
Copy link
Author

kunshim commented Mar 29, 2023

CVE-2023-26923 assigned

@Jojo-Schmitz
Copy link
Contributor

You may want to mention that is fixed in 4.0.2 and later and also in my 3.7 from https://github.com/Jojo-Schmitz/MuseScore/tree/3.x

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

Successfully merging a pull request may close this issue.

3 participants