google / google/filament

MeshAssimp: glTF texture URI is joined without a containment check, allowing path traversal outside the model directory

Open
#10,431 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.