gltfio: heap out-of-bounds write in `AssetLoaderExtended::createPrimitive` (slotIndices sized by filtered morph count, indexed by raw index)
- Dominant language
- C++
- Stars
- 20.5k
- Forks
- 2.3k
- Avg merge
- 2d 16h
- Merged PRs (30d)
- 74
Description
### Summary
Loading an untrusted `.glb`/`.gltf` through the extended asset loader can cause a heap out-of-bounds write in `AssetLoaderExtended::createPrimitive`. `slotIndices` is sized by the *filtered* morph-target count but written at the *raw* morph-target index, so when the morph-target filter drops an earlier target, a later target's raw index exceeds the array bounds.
### Affected code (branch `main`)
`libs/gltfio/src/extended/AssetLoaderExtended.cpp`:
```cpp
// createPrimitive
out->slotIndices.resize(morphTargets.size()); // sized by the FILTERED count
...
if (slot.offset == 0xdeadbeef) {
out->slotIndices[slot.slot] = outSlots.size() + i; // indexed by the RAW morph index
}
```
`morphTargets` (built earlier in `createPrimitive`) keeps a morph target only if it has a TANGENT attribute, or (no tangent) if the primitive has a non-unlit material. A POSITION-only morph target on a material-less primitive is dropped. But `slot.slot` is the raw morph-target index (`computeGeometries` sets `.slot = target.morphTarget`), so when target 0 is dropped and target 1 (raw index 1) is kept, `slotIndices` is `resize(1)` and the code writes `slotIndices[1]` — one element past the allocation.
### Reproduction (AddressSanitizer)
`.glb`: one primitive, **no material**, two morph targets — target[0] POSITION-only (dropped by the filter), target[1] with a TANGENT (kept, raw index 1). Loaded through the extended loader (`AssetConfiguration::ext` set; `AssetConfigurationExtended::isSupported()` true on desktop).
ASan trace (full-engine `test_gltfio` build, `-DFILAMENT_ENABLE_ASAN_UBSAN=ON`):
```
==ERROR: AddressSanitizer: heap-buffer-overflow ... WRITE of size 4 ...
#0 AssetLoaderExtended::createPrimitive(...) AssetLoaderExtended.cpp:558
#1 FAssetLoader::createPrimitives(...) AssetLoader.cpp:695
...
#6 AssetLoader::createAsset(...) AssetLoader.cpp:1821
0x... is located 0 bytes after 4-byte region [ resize(1) == one int ]
allocated by:
std::vector::resize(...) <- AssetLoaderExtended.cpp:543 // the resize(morphTargets.size())
SUMMARY: AddressSanitizer: heap-buffer-overflow AssetLoaderExtended.cpp:558 in ...::createPrimitive(...)
```
I can attach the crafted `.glb` and a builder script, or a `test_gltfio` test case, whichever you prefer.
### Root cause / suggested direction
This is more than a size mismatch — the extended loader's morph-target *filter* (which decides which targets need a tangent-space job) is being used to size and populate the morph-slot bookkeeping, but the consumer (`FAssetLoader::createRenderable`) indexes `slotIndices` by the raw target index and asserts `slot.slot == tindex` for every POSITION-bearing target:
```cpp
// createRenderable
for (int tindex = 0; tindex < numMorphTargets && (size_t) tindex < numSlots; ++tindex) {
...
BufferSlot& slot = slots[outputPrim->slotIndices[tindex]];
assert_invariant(slot.slot == tindex);
}
```
So a target that the extended filter drops has no slot, yet the consumer still expects one (and, separately, that target's morph POSITION data is not uploaded). The non-extended loader (`createPrimitive` in `AssetLoader.cpp`) sizes `slotIndices` to the full `targetsCount` and writes an entry for every target, keeping this invariant.
Simply changing the extended `resize(morphTargets.size())` to `resize(targetsCount)` stops the out-of-bounds write but leaves the dropped targets pointing at the wrong slot (it trips the `slot.slot == tindex` assertion in debug builds). The correct fix likely needs the extended path to create a morph slot for every target (decoupling the tangent-computation filter from the morph-slot/POSITION bookkeeping), matching the non-extended loader — which is why I'm filing this as an issue rather than a one-line PR, since it touches the extended loader's slot model.
### Impact
Heap out-of-bounds write from an untrusted, `cgltf_validate`-clean `.glb`, during asset loading, on the desktop/tools extended loader path (`isSupported()` is false on Android/iOS/Emscripten). Controlled write offset; the written value is a small non-negative integer.
Happy to help with a repro or a patch once you confirm the intended direction for the slot model.
Contributor guide
Research direction
Start in libs/gltfio/src/extended/AssetLoaderExtended.cpp at createPrimitive, then compare its slot handling with AssetLoader.cpp and the createRenderable consumer. Reproduce with the described two-target .glb using the ASan-enabled test_gltfio build. Done means the extended loader handles every POSITION-bearing morph target without an out-of-bounds write and preserves the slot invariant.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- computer-graphics, security
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100