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

Ebml[Unicode]String::ReadData heap overflow bug on 32bit builds #74

Closed
kryc opened this issue Feb 7, 2021 · 4 comments · Fixed by #76
Closed

Ebml[Unicode]String::ReadData heap overflow bug on 32bit builds #74

kryc opened this issue Feb 7, 2021 · 4 comments · Fixed by #76
Assignees
Labels

Comments

@kryc
Copy link

kryc commented Feb 7, 2021

--[ 1. Summary

An extremely exploitable heap overflow bug exists in the implementation
of EbmlString::ReadData and EbmlUnicodeString::ReadData in libebml. This
bug is reachable from the current stable release of vlc.

--[ 2. Discussion

The issue exists in the calculation of the required buffer size to store
the string. The following calculation is performed:

310        auto Buffer = new (std::nothrow) char[GetSize()+1];
...
315            input.readFully(Buffer, GetSize());

The value returned from GetSize is guaranteed to be an unisigned 64 bug
number, and due to the way in which intetgers are stored and parsed in
Ebml will only use the lowest 48 bits. This guarantees that the integer
cannot overflow on 64but builds.

However, on 32bit builds, the value is implicitly cast to a size_t in
the call to new, meaning that the truncated length can be significantly
shorter than the amount of data to be copied. For example, if the length
of the string element is claimed to be 0xffffffff, the resultant
allocation will be 0x100000000. The cast to a 32bit size_t drops the top
1 bit, meaning an array of size zero is allocated.

In the event that the string element is placed maliciously at the end of
the file to be parsed, an extremely exploitable controlled heap overflow
can occur.

--[ 3. Resolution

The fix for this bug is relatively straightforward, a check must be
added to ensure that the value of GetData() + 1 is less than SIZE_MAX to
ensure that it will not be truncated in the call to new.

--[ 4. Proof of Concept

The following proof of concept file shows the behaviour in the latest
(at time of writing) version of vlc on Ubuntu 10.10.

$ sudo apt install vlc:i386 gdb
$ wget https://kryc.uk/libebml-poc.mkv
$ gdb -q vlc
$$ r libebml-poc.mkv
@mbunkus
Copy link
Contributor

mbunkus commented Feb 7, 2021

Thank you very much! I'll likely prepare a new release tomorrow.

@mbunkus mbunkus self-assigned this Feb 7, 2021
@carnil
Copy link

carnil commented Feb 10, 2021

This issue was assigned CVE-2021-3405

@mbunkus
Copy link
Contributor

mbunkus commented Feb 18, 2021

As written in #76:

I've just spent a couple of hours building current libebml with this PR, current libmatroska & VLC on Debian 10 (as there are no 32-bit installation images for Ubuntu anymore). Debian's VLC with Debian's libebml/libmatroska crashes as expected; the re-built ones don't — only a couple of error messages from VLC's Matroska plugin. Meaning the fix seems to work as far as I can tell.

I'll package a new release of libebml later today.

@mbunkus
Copy link
Contributor

mbunkus commented Feb 18, 2021

libEBML v1.4.2 & libMatroska v1.6.3 are out:
https://www.matroska.org/downloads/libraries.html

The former is solely the bug fix for this issue; the latter solely adding new classes for track header flags.

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

Successfully merging a pull request may close this issue.

3 participants