Fix misc reading overflows by robUx4 · Pull Request #76

Fix misc reading overflows by robUx4 · Pull Request #76 · Matroska-Org/libebml

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

Conversation

some strings may read huge size into small memory buffers on 32 bits systems
some integer/float are trying to read large sizes on fixed (8 bytes) buffers

There were some asserts but in release builds that won't trigger anything and just write in out of bounds memory.

If we can't read we just pretend that we have read the data. But SetValueIsSet() is not called. Just like we do for other errors we already handle.

Fixes #74

The positions in libebml are stored in filepos_t which is in 64 bits. But when
reading in memory we are limited to 32 bits on 32 bits systems. We cannot
allocate more and read into such buffer.

When the size to read is bigger than what the system can handle, just assume
it's an allocation failure (it won't be because the 64 bits value will be
casted to 32 bits).

Ref. CVE-2021-3405

There's an assert which will be of no use on release builds. If the size is
bigger than 8, we will read more than 8 bytes into the 8 bytes binary Buffer.

All other size are not allowed and should not try to read the data. Especially
with very large sizes.

libebml doesn't handle integers with a length bigger than 8 bytes (64 bytes).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Generally the PR doesn't break any of my test cases, but I do have several things I'd like to see changed differently.

If you don't have much time I can give it a go tomorrow.

If we don't have a size of 4 for the CRC-32 we should not read it.
Otherwise we have a buffer ready to write to, no need for an extra allocation.

Historically SCOPE_NO_DATA doesn't skip the data. It's skipped in EbmlMaster::Read().

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. I still have several requests. Sorry :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good to me now, and the MKVToolNix test suite shows no regressions.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works on my end (Debian 10.7).

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.