[BUG] owning_ref vulnerability

gyscos / cursive

A Text User Interface library for the Rust programming language

MIT License

4.25k stars 244 forks source link

[BUG] owning_ref vulnerability #678

Open DJDuque opened 1 year ago

DJDuque commented 1 year ago

cursive-core uses owning-ref crate, which has a couple of issues as described here: https://github.com/noamtashma/owning-ref-unsoundness

The maintainer of owning_ref seems unresponsive, and I was wondering if there are any plans to move away from having this crate as a dependency? Is Cursive affected by the unsound api of owning_ref?

gyscos commented 1 year ago

Hi,

I don't think we use the affected API directly form cursive. We do re-export a OwningHandle type, so users could possibly call the unsound APIs themselves on this, but that's not exactly something we can do anything about (and its' not a usage we even mention).

If a more supported alternative exists (or if/when the same simple use-case we need can be handled without unsafe), I could absolutely consider switching.

dbrgn commented 1 year ago

More details here: https://rustsec.org/advisories/RUSTSEC-2022-0040

gyscos commented 1 year ago

What we want is Arc/Rc projection. Here's a thread from last year about it (also mentioning owning-ref, and its soundness issues): https://internals.rust-lang.org/t/field-projection-for-rc-and-arc/15827

For some restricted use-cases of common combinations (Rc<RefCell<T>> and Arc<Mutex<T>>), it should be possible to have a safe API - which is exactly what owning-ref does (only owning-ref also adds other less safe APIs). Something like shared-rc (though that particular crate is still very new).

yoke might also be an interesting replacement.

alexanderkjall commented 1 year ago

shared-rc looks promising

gyscos commented 8 months ago

After toying with ouroboros, I ended up using parking_lot and its ArcMutexGuard, which looks exactly like what we need: a popular, maintained crate with a specific-purpose implementation avoiding most of the pitfalls from general-purpose self-referencing libs.

yoke is mostly focused on zero-copy deserialization, and it shows in the Yokeable trait, which is not implemented for anything close to a mutex.
self_cell looked promising but does not support generics (we need to be generic on V: View).
ouroboros was interesting but did not support DerefMut<Target = V> for soundness reason in the general case, which would have broken the current API.
guardian looked like it could be a fit, but with no dependent and low usage, it felt a bit risky.

TobiPeterG commented 6 months ago

Would it be possible to just switch to https://crates.io/crates/safer_owning_ref? It's owning_ref but with the security issue fixed :)

gyscos commented 5 months ago

In my previous post I mentioned switching to parking_lot, but apparently I only did that for NamedView - the main use of owning_ref I had.

Turns out there was another use of it in TextView, but here it was entirely useless. I have now removed owning_ref entirely from the dependencies.