fix: add bounds checking in normalizeSkinningWeights to prevent heap OOB write#9878
Open
YLChen-007 wants to merge 1 commit intogoogle:mainfrom
Open
fix: add bounds checking in normalizeSkinningWeights to prevent heap OOB write#9878YLChen-007 wants to merge 1 commit intogoogle:mainfrom
YLChen-007 wants to merge 1 commit intogoogle:mainfrom
Conversation
…OOB write The normalize lambda in normalizeSkinningWeights trusts the accessor's count field without validating it against the actual buffer size. A maliciously crafted glTF file can set an arbitrarily large count with a small buffer, causing out-of-bounds heap writes during weight normalization. This patch adds the following safety checks: - Null pointer validation for buffer_view, buffer, and data pointers - Offset validation to ensure accessor offset is within buffer bounds - Safe count computation based on available buffer bytes and stride - Clamping of accessor count to prevent out-of-bounds access The fix gracefully handles malformed inputs by clamping the iteration count and logging a warning, rather than crashing or corrupting memory.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
Summary
Fix a heap buffer overflow vulnerability in
normalizeSkinningWeights(resolves #9877).Problem
The
normalizelambda inlibs/gltfio/src/ResourceLoader.cpptrusts the glTF accessor'scountfield without validating it against the actual buffer size. A maliciously crafted.gltffile can specify an arbitrarily largecountwith a small buffer, causing out-of-bounds heap writes during skinning weight normalization.This leads to:
Any application using Filament's
gltfiowithnormalizeSkinningWeights = true(the default in gltf_viewer, Android ModelViewer, Web viewer, etc.) is vulnerable when loading untrusted glTF files.Fix
This patch adds bounds checking to the
normalizelambda:buffer_view,buffer, anddatapointers before dereferencingtotalOffset(viewOffset + accessorOffset) does not exceed buffer sizedata->countto the safe maximum with aLOG(WARNING)messageThe fix gracefully handles malformed inputs by clamping the iteration count rather than crashing.
Testing
Verified with a standalone harness mirroring the
normalizeSkinningWeightslogic:poc.gltf)control.gltf)Changes
libs/gltfio/src/ResourceLoader.cpp: Added bounds checking innormalizeSkinningWeightsnormalize lambda (+38 lines, -2 lines)