gltfio: heap OOB write in cgltf_accessor_unpack_floats via meshopt-bypassed sparse accessor index
- Dominant language
- C++
- Stars
- 20.5k
- Forks
- 2.3k
- Avg merge
- 2d 14h
- Merged PRs (30d)
- 83
Description
### Summary
`cgltf_accessor_unpack_floats` writes decoded sparse accessor values at `out + writer_index * floats_per_element` with no bounds check on `writer_index`. When a sparse accessor's `indices.bufferView` uses `EXT_meshopt_compression`, `cgltf_validate` evaluates the compressed fallback bytes as index values (not the decoded stream), allowing decoded sparse index values >= `accessor->count` to bypass validation and produce a heap OOB write.
### Affected code
**`third_party/cgltf/cgltf.h` - sparse pass in `cgltf_accessor_unpack_floats` (line ~2436):**
```c
size_t writer_index = cgltf_component_read_index(index_data, sparse->indices_component_type);
float* writer_head = out + writer_index * floats_per_element; // NO bounds check
cgltf_element_read_float(reader_head, ..., writer_head, floats_per_element);
// OOB write if writer_index >= element_count
```
**`third_party/cgltf/cgltf.h` - sparse index bounds check in `cgltf_validate` (line ~1625):**
```c
if (sparse->indices_buffer_view->buffer->data) // reads fallback/compressed bytes
{
cgltf_size index_bound = cgltf_calc_index_bound(sparse->indices_buffer_view, ...);
CGLTF_ASSERT_IF(index_bound >= accessor->count, cgltf_result_data_too_short);
}
```
`cgltf_calc_index_bound` reads `buffer_view->buffer->data` (the compressed fallback stream). For a meshopt-compressed `indices_buffer_view`, the actual decoded indices live in `buffer_view->data` - set only after `decodeMeshoptCompression` runs, which is **after** `cgltf_validate` in `ResourceLoader::loadResources`:
```
utility::loadCgltfBuffers(...) // cgltf_load_buffers + cgltf_validate (reads fallback bytes)
utility::decodeMeshoptCompression(...) // sets buffer_view->data = decoded indices (NO re-validation)
pImpl->computeTangents(asset) // TangentsJob -> cgltf_accessor_unpack_floats -> OOB write
```
**`libs/gltfio/src/TangentsJob.cpp` - write targets (lines 87–155):**
```cpp
unpackedNormals.reset(new float3[vertexCount]);
cgltf_accessor_unpack_floats(baseNormalsInfo, &unpackedNormals[0].x, vertexCount * 3);
// if baseNormalsInfo->is_sparse and sparse index bypass fires → OOB write past unpackedNormals
unpackedPositions.reset(new float3[vertexCount]);
cgltf_accessor_unpack_floats(basePosInfo, &unpackedPositions[0].x, vertexCount * 3);
unpackedTangents.reset(new float4[vertexCount]);
cgltf_accessor_unpack_floats(baseTangentsInfo, &unpackedTangents[0].x, vertexCount * 4);
unpackedTexCoords.reset(new float2[vertexCount]);
cgltf_accessor_unpack_floats(uvInfo, &unpackedTexCoords[0].x, vertexCount * 2);
```
The extended path (`libs/gltfio/src/extended/TangentsJobExtended.cpp` line 264) also calls `cgltf_accessor_unpack_floats` via `requiresConversion` (which returns `true` for sparse accessors), so it is equally affected.
### Trigger conditions
Craft a `.glb` file where:
1. A mesh primitive has a vertex attribute accessor (NORMAL, POSITION, TANGENT, or TEXCOORD_0) with `accessor.sparse` set
2. The sparse accessor's `indices.bufferView` has `EXT_meshopt_compression` applied (mode = INDICES)
3. The compressed bytes, when read raw by `cgltf_calc_index_bound`, produce values < `accessor.count` - easy to achieve since meshopt format headers start with 0xe1 and the variable-length encoding naturally produces small byte values when misinterpreted as integers
4. The decoded sparse index values are >= `accessor.count` (= `vertexCount`)
5. `accessor.sparse.count >= 1`
### Impact
Heap OOB write with both offset and content under attacker control:
- **Write offset**: `(writer_index - vertexCount) * floats_per_element * sizeof(float)` bytes past the allocation - attacker controls `writer_index` via the meshopt-compressed sparse index stream
- **Write content**: attacker-controlled floats from `sparse.values.bufferView`
Any application loading untrusted `.glb`/`.gltf` files through Filament's gltfio layer is affected. The write-what-where primitive is sufficient to corrupt adjacent heap allocations or metadata.
### Suggested fix
Add a bounds check on `writer_index` in `cgltf_accessor_unpack_floats` before the write:
```diff
--- a/third_party/cgltf/cgltf.h
+++ b/third_party/cgltf/cgltf.h
size_t writer_index = cgltf_component_read_index(index_data,
sparse->indices_component_type);
+ if (writer_index >= element_count) {
+ return 0;
+ }
float* writer_head = out + writer_index * floats_per_element;
```
Alternatively, fix the root cause by running `decodeMeshoptCompression` before `cgltf_validate` in `ResourceLoader.cpp`, so the bounds check in `cgltf_validate` operates on decoded values.
*Related: #10193 (meshopt bypass on triangle index accessor), #10194 (MAT4 type-width OOB in sparse pass). This is a third distinct variant - same decode-after-validate root cause, different accessor type (sparse vertex attribute indices) and different write target (`cgltf_accessor_unpack_floats` sparse loop).*
Contributor guide
Research direction
Start with the sparse pass in third_party/cgltf/cgltf.h, then trace utility::loadCgltfBuffers and utility::decodeMeshoptCompression in ResourceLoader::loadResources. Inspect the affected calls in libs/gltfio/src/TangentsJob.cpp and the extended path. Done means a crafted meshopt-compressed sparse accessor no longer causes an out-of-bounds write during validation or float unpacking.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- computer-graphics, security
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 48/100