MeshAssimp: glTF texture URI is joined without a containment check, allowing path traversal outside the model directory
- Dominant language
- C++
- Stars
- 20.5k
- Forks
- 2.3k
- Avg merge
- 2d 14h
- Merged PRs (30d)
- 83
Description
## Summary
In `MeshAssimp`, a texture URI taken from a model file is concatenated onto the model's directory with no containment check, so a URI containing `../` resolves outside that directory. Loading a malicious glTF can make the process open a file elsewhere on the filesystem.
## Where
`libs/filamentapp/src/MeshAssimp.cpp:476`:
```cpp
loadTexture(engine, textureDirectory + textureFile.C_Str(), &textureMap, isSRGB, hasAlpha);
```
`textureDirectory` is the model's own directory (`MeshAssimp.cpp:895`):
```cpp
std::string dirName = asset.file.getParent();
processGLTFMaterial(scene, material, materialName, dirName, outMaterials);
```
`textureFile` is the texture URI read out of the model. The two are joined with plain `std::string` concatenation, and the result goes to `loadTexture` (`MeshAssimp.cpp:316-334`), which calls `path.exists()` and then `stbi_load(path.getAbsolutePath().c_str(), ...)`. Nothing between those points checks that the joined path is still inside `textureDirectory`.
This is reached from `MeshAssimp::processGLTFMaterial`, i.e. when Assimp loads a glTF/GLB, via all five texture slots (`baseColorMap`, `metallicRoughnessMap`, `aoMap`, `normalMap`, `emissiveMap`).
## Reproduction
The behaviour is entirely in the path arithmetic, so it can be shown with Filament's own `utils::Path`, reproducing the same expression as line 476:
```cpp
std::string const textureDirectory = Path(modelFile).getParent();
std::string const joined = textureDirectory + textureUri;
Path const resolved = Path(joined).getAbsolutePath();
```
Result:
```
== control: a texture beside the model ==
textureDirectory : /root/massimp/models/
texture URI : wood.png
resolved : /root/massimp/models/wood.png
inside model dir : yes
== traversal: relative URI climbing out of the model directory ==
textureDirectory : /root/massimp/models/
texture URI : ../private/secret.png
resolved : /root/massimp/private/secret.png
exists : YES
inside model dir : NO - ESCAPED
== traversal: deeper climb ==
textureDirectory : /root/massimp/models/
texture URI : ../../../../etc/hostname
resolved : /etc/hostname
exists : YES
inside model dir : NO - ESCAPED
```
So a glTF whose `uri` is `../../../../some/other/path.png` causes `stbi_load` to be called on a file outside the model directory.
## Impact and limits
Being upfront about the bounds of this, since they matter for how it should be prioritised:
- `MeshAssimp` is part of `libs/filamentapp`, the sample-app support library, not a library shipped to applications, so this is not reachable in the Android/iOS distributions.
- The opened file is handed to `stbi_load`, so anything that does not decode as an image yields `nullptr`. In practice this reads *image* files from outside the model directory rather than arbitrary file contents. A file's existence can also be probed through `path.exists()`.
It is still a containment failure: the directory a model was loaded from is meant to bound where its resources come from, and right now it does not.
## Suggested fix
Resolve the joined path and confirm it is still under the model directory before opening it, rejecting it otherwise — comparing canonicalised paths, and comparing against the directory *with* a trailing separator so a sibling directory sharing the prefix (`models_evil/`) does not pass.
Sketch:
```cpp
Path const base = Path(textureDirectory).getCanonicalPath();
Path const target = Path(textureDirectory + textureFile.C_Str()).getAbsolutePath();
std::string baseStr = base.c_str();
if (!baseStr.empty() && baseStr.back() != '/') {
baseStr += '/';
}
if (std::string(target.c_str()).rfind(baseStr, 0) != 0) {
slog.e << "Texture " << target.c_str() << " is outside the model directory, skipping."
<< io::endl;
return;
}
```
A related note while you are in this area: `utils::Path::concat` (`libs/utils/src/Path.cpp:78-86`) returns the right-hand side outright when it is absolute, and does not canonicalise, so other callers that join an externally supplied name onto a base directory have the same shape of problem. `DirIncluder::operator()` (`libs/filament-matp/src/DirIncluder.cpp:30`) is one such caller, though there the input is material source at build time.
Happy to send a PR if you would like the fix in this form.
Contributor guide
Research direction
Start in libs/filamentapp/src/MeshAssimp.cpp at processGLTFMaterial, the texture load around lines 316-334 and 476, then review the model-directory setup near line 895. Read utils::Path canonical and absolute path behavior before checking the joined texture path. Done means in-directory textures still load while ../ and absolute or sibling-prefix paths are rejected before stbi_load.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- computer-graphics, security
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100