Skip to content

Fix heap OOB read in PLY reader from unchecked list entry count#1205

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

Fix heap OOB read in PLY reader from unchecked list entry count#1205
gn00295120 wants to merge 1 commit into
google:mainfrom
gn00295120:fix/ply-oob-read

Conversation

@gn00295120

Copy link
Copy Markdown

Summary

PlyReader::ParseElementData reads a list entry count from the PLY file and uses it to compute how many bytes to copy from the buffer, without checking that the buffer actually contains that much data. A PLY file with a large list count and truncated data triggers a heap-buffer-overflow read. The Decode() return value for the count itself was also silently ignored.

The same issue exists for non-list properties: data_type_num_bytes() bytes are copied from the buffer without checking remaining_size().

Changes

  • Check the Decode() return value when reading the list entry count
  • Reject negative entry counts
  • Validate that num_bytes_to_read <= remaining_size() before the copy (also prevents integer overflow in the multiplication)
  • Add a remaining_size() check for non-list property reads

Reproduction

# Build with ASAN
cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ \
  -DCMAKE_CXX_FLAGS="-fsanitize=address -fno-omit-frame-pointer" \
  -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=address" .
cmake --build build

# Binary PLY with large list count, truncated data
printf 'ply\nformat binary_little_endian 1.0\nelement v 1\nproperty list uchar int i\nend_header\n\xff\x41\x41\x41' > crash.ply
./build/draco_encoder -i crash.ply

ASAN trace (before fix):

==PID==ERROR: AddressSanitizer: heap-buffer-overflow on address ...
READ of size 16 at ... thread T0
    #0 in draco::PlyReader::ParseElementData ply_reader.cc:206
    #1 in draco::PlyReader::ParsePropertiesData ply_reader.cc:177
    #2 in draco::PlyReader::Read ply_reader.cc:71

PlyReader::ParseElementData uses a file-supplied list entry count to
compute a copy size without checking the buffer has that many bytes.
A truncated binary PLY with a large list count causes a heap OOB read.

- Check Decode() return value for the list count
- Reject negative entry counts
- Validate num_bytes_to_read against remaining_size()
- Add remaining_size() check for non-list property reads
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