CVE-2022-47085: Potential risk of panic out of `catch_unwind()` · Issue #2775 · ostreedev/ostree

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

Closed

shinmao opened this issue

Nov 18, 2022

· 6 comments · Fixed by #2776

Comments

Hello guys, when I tried to review the implementation of filter_trampoline_unwindsafe(), I found that the function would call print_panic to print out the error message as following.

print_panic would call eprintln! to print out error message. However, based on the documentation of eprintln(https://doc.rust-lang.org/std/macro.eprintln.html), it will trigger panic if writing to io::stderr fails. This would cause to another unwinding out of catch_unwind(), and become undefined behavior when unwinding reaches the “C” ABI boundary.

Do you think such potential risk exist? Thanks.

Actually, I am trying to find some elegant use catch_unwind() on github. Your code comes to my sight because the repo is popular.

And yes, I would like to help finding out a way to fix this issue together!

cgwalters added a commit to cgwalters/ostree that referenced this issue

Nov 18, 2022

Actually, I am trying to find some elegant use catch_unwind() on github. Your code comes to my sight because the repo is popular.

Ah, heh. Well to be honest I didn’t personally write that code; it dates to https://gitlab.com/fkrull/ostree-rs/-/commit/2cf6602f973a8af4b45cb9d04e31496a127ffc49

For idiomatic use of catch_unwind…I’m not sure it’s the best; what’s your goal? To do the same thing and just print it to stderr?

Anyways, I did a PR in #2776

Yes. I saw your PR.

It seems that write_all would still possibly return variant of ErrorKind. It would be swallowed by the assigned variable _. However, I think it is much better than unexpected panic!.

cgwalters added a commit to cgwalters/ostree that referenced this issue

Nov 21, 2022

@shinmao yes there is still a chance of hitting error cases in that scenario, but I don’t see a better way of handling them over discarding them behind _. We’ll merge this code for now, but if any better idea comes up feel free to ping here.

Hi, is it possible for you guys to help me report issue to RUSTSEC? I am a security researcher trying to build some tools, and I might need some results on CVE/RUSTSEC to evaluate my tools. Though the issue has been closed, could you help me to explain the importance of finding this issue?

In filter_trampoline_unwindsafe(), what would happen if panics escaped to C function? I know unwinding from Rust function to foreign function is still undefined behavior. In your application scenario, will crash or something more serious happen?