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

Fix out-of-memory errors with mdat boxes and remove size-based checks #152

Merged
merged 1 commit into from
Oct 1, 2023

Conversation

aler9
Copy link
Contributor

@aler9 aler9 commented Sep 30, 2023

@sunfish-shogi

While patch #150 is able to prevent RAM exhaustion with the majority of small, specially-crafted strings, it isn'effective against strings that contain mdat boxes. A short string is able to cause RAM exhaustion by setting the mdat box size to a big number.

This PR fixes the issue by replacing size-based checks with checks on the effective size of the underlying buffer, that are performed by using Seek(). In this way, an attacker may cause DoS errors if and only if he is able to upload an amount of data equal to the size of the RAM of the machine, and if there are no size checks before passing the buffer to ReadBoxStructure() or Unmarshal().

Size-based checks are performed only in case of non-uint8 slices, since it's not possible to know in advance the overall size of a generic slice.

marshaller.go Outdated Show resolved Hide resolved
@sunfish-shogi
Copy link
Contributor

sunfish-shogi commented Oct 1, 2023

Thank you for PR!
I missed reflect.MakeSlice in #150 .

I have only one question about this change.
Could you check my comment about maxSliceCapacity?

@aler9 aler9 force-pushed the patch/oom branch 2 times, most recently from 22a1d29 to 2c7a537 Compare October 1, 2023 10:56
@aler9
Copy link
Contributor Author

aler9 commented Oct 1, 2023

i edited the patch and merged the comment, thanks

@sunfish-shogi
Copy link
Contributor

sunfish-shogi commented Oct 1, 2023

@aler9

i increased the maximum capacity to 128k

Sorry, I didn't make it clear enough.
Some boxes have entries for each frames.
For example, in 60fps video, it will be 216k entries per hour.
So I think maxSliceCapacity should be larger than 2M.

@aler9
Copy link
Contributor Author

aler9 commented Oct 1, 2023

If that's the case, i would discard the idea of imposing a fixed cap on the array size, since it limits the use cases of this library without solving the issue, that is preventing specially-crafted strings to crash applications.

It's also impossible to know in advance the overall bit size of a field and use it to check whether there are enough bits, since some fields have a dynamic length that depends on some other fields, and it would require decoding the struct.

In my opinion the only way to solve the issue consists in replacing the static preallocation of the slice with a dynamic allocation, replacing the make() call with multiple append() calls. I know that this decreases performance, but i don't see any other way.

@aler9
Copy link
Contributor Author

aler9 commented Oct 1, 2023

I've edited the patch in order to remove the slice preallocation. Adding appends() was not necessary since they are already there.

While patch abema#150 is able to prevent RAM exhaustion with the majority of
small, specially-crafted strings, it isn'effective against strings that
contain mdat boxes. A short string is able to cause RAM exhaustion by
setting the mdat box size to a big number.

This PR fixes the issue by replacing size-based checks with checks on
the effective size of the underlying buffer, that are performed by
using Seek(). In this way, an attacker may cause DoS errors if and only
if he is able to upload an amount of data equal to the size of the RAM
of the machine, and if there are no size checks before passing the
buffer to ReadBoxStructure() or Unmarshal().

Size-based checks are performed only in case of non-uint8 slices, since
it's not possible to know in advance the overall size of a generic
slice.
@sunfish-shogi sunfish-shogi merged commit b0aad6c into abema:master Oct 1, 2023
6 checks passed
@aler9 aler9 deleted the patch/oom branch October 7, 2023 14:47
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 this pull request may close these issues.

2 participants