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 MemIOCallback buffer overflows #148

Merged
merged 2 commits into from
Nov 4, 2023

Conversation

robUx4
Copy link
Contributor

@robUx4 robUx4 commented Nov 1, 2023

Fixes #147

If the addition of 2 positive values is smaller than one of the values then we
have an overflowing addition.

In this case that means we are trying to read more data that is actually in
our buffer. So we can use the same mechanism as reading too much data.
If the addition of 2 positive values is smaller than one of the values then we
have an overflowing addition.

In this case we will not be able to allocate that much, just return a size
written as 0.
@robUx4 robUx4 added the bug label Nov 1, 2023
@robUx4 robUx4 requested a review from mbunkus November 4, 2023 08:09
@robUx4
Copy link
Contributor Author

robUx4 commented Nov 4, 2023

Poke @mbunkus

@mbunkus
Copy link
Contributor

mbunkus commented Nov 4, 2023

Given that all variables in that addition are unsigned, testing for overflow like this is OK. Looks good to me.

I've also tested it by cherry-picking both commits on top of the v1.x branch which I'm still bundling in MKVToolNix & running my test suite, which completes just fine. Additionally I've verified that the provided sample program doesn't segfault anymore with a patched v1.x lib.

You can merge this. Please also cherry-pick & merge both commits onto the v1.x branch. We should then do another v1.x release soon afterwards.

@robUx4 robUx4 merged commit 2d5c11c into Matroska-Org:master Nov 4, 2023
20 checks passed
@robUx4 robUx4 deleted the memio_overflow branch November 4, 2023 09:53
@00xc
Copy link

00xc commented Dec 28, 2023

Hi, do you plan to request CVEs for these bugs? They look security-relevant

@mbunkus
Copy link
Contributor

mbunkus commented Dec 28, 2023

Yes, they're security relevant. Personally I'm not interested in dealing with the bureaucracy of CVE. If Steve wants to handle it, he can, of course; or if anyone else does, go ahead.

@00xc
Copy link

00xc commented Dec 28, 2023

Steve let me know if you'll go on with the process, otherwise I can take care of it.

@robUx4
Copy link
Contributor Author

robUx4 commented Dec 29, 2023

Same here. I'd rather not deal with the CVE process.

@00xc
Copy link

00xc commented Dec 29, 2023

I just requested a CVE. I'll post it here for documentation purposes once it's assigned.

@rathann
Copy link

rathann commented Feb 2, 2024

FWIW, this is CVE-2023-52339 .

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 this pull request may close these issues.

MemIOCallback::read has a integer overflow bug
4 participants