Skip to content

Fix heap-buffer-overflow in glTF decoder accessor reads - #1165

Closed
Sebasteuo wants to merge 1 commit into
google:mainfrom
Sebasteuo:fix-gltf-oob-read
Closed

Sebasteuo wants to merge 1 commit into
google:mainfrom
Sebasteuo:fix-gltf-oob-read

Conversation

@Sebasteuo

Copy link
Copy Markdown

Add bounds validation to all glTF accessor data copy functions to prevent out-of-bounds heap reads when processing crafted .glb/.gltf files with malicious byteOffset, byteLength, or byteStride values.

The glTF decoder trusted accessor/bufferView fields from the input file without validating them against the actual buffer size. A crafted .glb file could cause memcpy to read past the end of the allocated buffer, leading to heap-buffer-overflow (confirmed via AddressSanitizer).

Affected functions:

  • TinyGltfUtils::CopyDataAsFloatImpl() in tiny_gltf_utils.h
  • CopyDataAsUint32() in gltf_decoder.cc
  • CopyDataAs() (both specializations) in gltf_decoder.cc
  • CopyDataFromBufferView() in gltf_decoder.cc

Add bounds validation to all glTF accessor data copy functions to
prevent out-of-bounds heap reads when processing crafted .glb/.gltf
files with malicious byteOffset, byteLength, or byteStride values.

The glTF decoder trusted accessor/bufferView fields from the input
file without validating them against the actual buffer size. A crafted
.glb file could cause memcpy to read past the end of the allocated
buffer, leading to heap-buffer-overflow (confirmed via AddressSanitizer).

Affected functions:
- TinyGltfUtils::CopyDataAsFloatImpl() in tiny_gltf_utils.h
- CopyDataAsUint32() in gltf_decoder.cc
- CopyDataAs<T>() (both specializations) in gltf_decoder.cc
- CopyDataFromBufferView() in gltf_decoder.cc
@google-cla

google-cla Bot commented Apr 1, 2026

Copy link
Copy Markdown

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.

@Sebasteuo

Sebasteuo commented May 20, 2026 •

Copy link
Copy Markdown
Author

@googlebot I fixed this

@Sebasteuo

Copy link
Copy Markdown
Author

Thanks for confirming the impact and advocating for the fix to land. Quick clarification on authorship**: I am the author of PR #1165 (handle @Sebasteuo)**. The CLA is now signed, so the maintainers can proceed with review. Happy to coordinate if you have additional test cases or related findings.

@Sebasteuo Sebasteuo closed this May 20, 2026
@Sebasteuo Sebasteuo reopened this May 20, 2026
@Sebasteuo Sebasteuo closed this May 20, 2026
@Sebasteuo Sebasteuo reopened this May 20, 2026
@igorvytyaz

Copy link
Copy Markdown
Collaborator

Closing this PR as the issue is fixed in merged PR #1214.

@igorvytyaz igorvytyaz closed this Sep 24, 2026
@Sebasteuo

Copy link
Copy Markdown
Author

@igorvytyaz thanks for looking at this. One technical point before
this stays closed:

#1214 only patches CopyDataAsFloatImpl() in tiny_gltf_utils.h. This
PR covered four additional call sites that still read accessor data
without bounds validation:

  • CopyDataAsUint32() in gltf_decoder.cc
  • CopyDataAs(), both specializations, in gltf_decoder.cc
  • CopyDataFromBufferView() in gltf_decoder.cc

Those take the same untrusted byteOffset, byteLength and byteStride
from the input file and pass them to memcpy without checking them
against the buffer size, which is the same pattern #1214 just fixed
in one place.

Happy to rebase this PR down to just those four functions so it
doesn't conflict with what already landed.

One other thing worth noting for the record... this PR was opened on
April 1 with the analysis and the fix for all five call sites, four
months before #1214. I'm not disputing that finding, independent
discovery happens, but since the merged fix is narrower than what
was already proposed here it seemed worth flagging.

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