Skip to content

Fix heap-buffer-overflow read in binary PLY list property parsing - #1213

Open
ganiganesh25 wants to merge 1 commit into
google:mainfrom
ganiganesh25:fix/ply-reader-oob-read
Open

Fix heap-buffer-overflow read in binary PLY list property parsing#1213
ganiganesh25 wants to merge 1 commit into
google:mainfrom
ganiganesh25:fix/ply-reader-oob-read

Conversation

@ganiganesh25

Copy link
Copy Markdown

Summary

  • PlyReader::ParseElementData() (in src/draco/io/ply_reader.cc) reads an attacker-controlled list count from a property list field in a binary PLY file, and uses it to compute the number of bytes to copy out of the input DecoderBuffer, without checking that the buffer actually has that many bytes remaining.
  • The copy goes through buffer->data_head() directly (raw pointer + std::vector::insert) instead of a bounds-checked Decode()/Peek() call, and the return value of the one bounds-checked Decode() call in that path was also being discarded.
  • A crafted binary binary_little_endian PLY file with a large list count followed by truncated data therefore causes an out-of-bounds heap read.
  • This is reachable from the public draco::ReadPointCloudFromFile / ReadMeshFromFile API when loading untrusted .ply files.

Fix

  • Check the return value of the list-count Decode() call.
  • Validate num_bytes_to_read against buffer->remaining_size() before copying list property data.
  • Add the same bounds check for non-list property data.
  • Add a regression test (PlyReaderTest.TestReaderTruncatedListData) with a truncated list property that previously triggered the overflow.

Verification

Reproduced locally with a small standalone driver linking libdraco built with -fsanitize=address, feeding it a crafted PLY:

==11878==ERROR: AddressSanitizer: container-overflow on address 0x6110000000f8 ...
READ of size 1 at 0x6110000000f8 thread T0
    ...
    #5 ... draco::PlyReader::ParseElementData(draco::DecoderBuffer*, int) ply_reader.cc:206
    #6 ... draco::PlyReader::ParsePropertiesData(draco::DecoderBuffer*) ply_reader.cc:177
    #7 ... draco::PlyReader::Read(draco::DecoderBuffer*) ply_reader.cc:71
    #8 ... draco::PlyDecoder::DecodeInternal() ply_decoder.cc:71

With the fix applied, the same input is rejected cleanly (Couldn't parse properties) instead of crashing.

Test plan

  • New regression test PlyReaderTest.TestReaderTruncatedListData passes.
  • Full draco_tests suite (186 tests) passes under AddressSanitizer, no regressions.
  • Manually verified existing testdata/*.ply fixtures (ASCII and binary) still decode correctly with the patched reader.

@google-cla

google-cla Bot commented Jul 30, 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.

PlyReader::ParseElementData() read an attacker-controlled list count
from a "property list" field and used it to compute the number of
bytes to copy out of the input DecoderBuffer, without checking that
the buffer had that many bytes remaining. The copy went through
buffer->data_head() directly rather than a bounds-checked
Decode()/Peek() call, so a crafted binary PLY with a large list count
followed by truncated data causes an out-of-bounds heap read.

Reachable via the public draco::ReadPointCloudFromFile /
ReadMeshFromFile API on untrusted binary_little_endian PLY input.

Add bounds checks before the list and non-list property copies, and
check the return value of the list-count Decode() call. Add a
regression test with a truncated list property.
@ganiganesh25
ganiganesh25 force-pushed the fix/ply-reader-oob-read branch from 8607acc to 1edbde0 Compare August 2, 2026 18:17
@ganiganesh25

Copy link
Copy Markdown
Author

@googlebot I signed it!

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.

1 participant