rustsec / advisory-db

Security advisory database for Rust crates published through crates.io
https://rustsec.org
Other
902 stars 349 forks source link

Unsoundness notice for xmp_toolkit < 1.9.0 #2029

Closed scouten-adobe closed 1 month ago

Shnatsel commented 1 month ago

Thank you for the report!

The file format requires the TOML to be followed by a human-readable description of the vulnerability in Markdown. See the example advisory: https://github.com/rustsec/advisory-db/blob/main/EXAMPLE_ADVISORY.md - and especially its raw text view.

scouten-adobe commented 1 month ago

@Shnatsel thank you; the linter alerted me to that and a couple of other issues (yay linters!). Please re-review: I think it's in the proper format now.

Shnatsel commented 1 month ago

I understand no actual memory corruption is possible, and the extent of the issue is merely terminating the process, similar to a panic with -C panic=abort?

I cannot see any way for an attacker to trigger the issue either.

So I don't think this fits our criteria for a security vulnerability, or even a soundness issue.

scouten-adobe commented 1 month ago

I agree that this isn't a security issue.

The API in question is not designated as unsafe and IMHO fails to uphold the contract of a safe API having no UB, which is why I felt it met criteria for unsoundness.

Shnatsel commented 1 month ago

Ah, you mean that a C++ exception being propagated across FFI boundary is not well-defined to abort, but is actually undefined behavior and may result in arbitrary miscompilations?

scouten-adobe commented 1 month ago

Yes, the FFI boundary was flawed.

Shnatsel commented 1 month ago

In that case, please add a mention of this possibly resulting in arbitrary UB and not just aborts to the advisory description.

scouten-adobe commented 1 month ago

This crate is a Rust wrapper around a C++ library and it was the wrapper that had the C++ exception leak, making a supposedly safe function actually insane.

Shnatsel commented 1 month ago

I consulted other people just to double-check that this is UB, and it seems to be the case. So I'll go ahead and merge this. Thanks!