Fix integer overflow in safeAddToBufferSize on MSVC#3120
Open
Ashutosh0x wants to merge 2 commits intogoogle-deepmind:mainfrom
Open
Fix integer overflow in safeAddToBufferSize on MSVC#3120Ashutosh0x wants to merge 2 commits intogoogle-deepmind:mainfrom
Ashutosh0x wants to merge 2 commits intogoogle-deepmind:mainfrom
Conversation
The MSVC fallback path in safeAddToBufferSize() performed unchecked arithmetic (type_size*nr*nc) on attacker-controlled values read from .mjb binary model files. This could cause integer overflow, leading to an undersized heap allocation followed by a heap buffer overflow when data is copied into the buffer. The fix adds manual overflow detection using SIZE_MAX/INTPTR_MAX comparisons, matching the behavior of the existing __builtin_*_overflow path used on GCC/Clang. Also adds a regression test that crafts a binary model buffer with overflow-inducing size fields and asserts safe rejection.
The MuJoCo test suite fixture translates mju_warning into ADD_FAILURE. Since mj_loadModelBuffer intentionally calls mju_warning when it safely rejects our crafted overflow buffer, this caused the test to fail. We now intercept the warning to assert it is triggered without failing the test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hey there 👋
While digging through the model loading code, I noticed that the overflow protection in
safeAddToBufferSize()only kicks in on GCC/Clang (via__builtin_*_overflow). On MSVC — which is what most Windows users build with — the#elsefallback just does raw arithmetic with no overflow checks:There's even a TODO comment about it! The values
nrandnccome straight from the.mjbbinary file header, so a maliciously crafted model file could overflow the multiplication, trick the allocator into creating a too-small buffer, and thenbufreadhappily writes past the end of it.What this PR does
src/engine/engine_io.cReplaces the unchecked fallback with proper manual overflow detection. Each multiplication and addition step is individually guarded usingSIZE_MAX/INTPTR_MAXcomparisons before proceeding. If any step would overflow, we bail out early and return 0 (same behavior as the GCC/Clang path).test/engine/engine_io_test.ccAdds a regression test (LoadModelBufferRejectsOverflowingSizes) that saves a trivial model to a binary buffer, mutates one of the size fields to something absurdly large, and checks thatmj_loadModelBuffergracefully returnsNULLinstead of crashing.How I tested it
__builtin_*_overflowpath still runs as before (no behavior change)Why this matters
Without this fix, any MSVC-built binary (including the official pip wheels on Windows) would accept a crafted
.mjbfile and hit a heap buffer overflow. At best that's a crash, at worst it's exploitable.Happy to iterate if you'd like any changes to the approach!
cc @yuvaltassa @quagla