Security advisory for AtheMathmo/rulinalg#201

[Qwaz]

The affected version of rulinalg has incorrect lifetime boundary definitions for RowMut::raw_slice and RowMut::raw_slice_mut. They do not conform with Rust's borrowing rule and allows the user to create multiple mutable references to the same location.

AtheMathmo/rulinalg#201

The incorrectness of raw_slice_mut() is straightforward. However, I'm not completely sure about raw_slice(). Naively downgrading mut refs to shared refs is semantically incorrect, and that's why I think raw_slice() should be fixed, too. However, I'm not completely sure about it and any input to this is appreciated.

Also, there was a comment in the original issue that recommends nalgebra instead of rulinalg. Maybe we need to consider filing an informational advisory for that.

[Shnatsel]

Looks good to me. I'd also include something about what violation of aliasing rules can actually result in, something like

This can lead to a variety of memory safety issues, including use-after-free, which may allow arbitrary code execution.

Thanks for reporting this issue, and for the audit!

[Qwaz]

I have reflected the feedback and created a new commit. I agree that it's possible to cause use-after-free and arbitrary code execution with complicated element type. However, considering that most users will use this library with numeric types, I think it might scare the users too much.

I tried to tone down your suggestion a little bit. Please share your thoughts if you have additional feedback or merge this if it looks good, thanks! :)

[Shnatsel]

Ah, this makes the issue less severe than I originally thought. Thanks for clarifying!

[Shnatsel]

Sorry for the delay on this PR.

I agree that it's possible to cause use-after-free and arbitrary code execution with complicated element type. However, considering that most users will use this library with numeric types, I think it might scare the users too much.

Do you think this warrants downgrading to informational then? Right now the severity of it as a vulnerability kind of conflicts with the relatively benign description.

Other than that LGTM

[Qwaz]

Do you think this warrants downgrading to informational then?

It's hard to tell whether a bug should get a security advisory or an informational advisory without (possibly provisional) shared standard for them. I'm opening a PR as a security advisory if we need to restrict the users of a library in a non-trivial way because of the security impact of a bug. This might be different than other people's standard, and the "non-trivial" part is also unfortunately subjective.

If this library was used with BigInt, which internally allocates, it may introduce an exploitable bug to the end user. The exact pattern to trigger the bug will be admittedly uncommon even in that case, but I think that's the nature of library bugs. (a bug triggered by an integer overflow requires an overflowing input, an injection bug requires the program to pass an attacker-controlled input to the vulnerable function, etc.)

In summary, my position is:

• Is this a security bug?
  • Yes, and I'm strongly against saying "this is a non-security bug" or "this is not a vulnerability"
  • It means the bug has security implication, not something like "you shouldn't use this library"
• Is this bug severe enough to get a security advisory than an informational advisory?
  • Hard to tell without the definition of "severe enough". I'm fine with either choice as long as there is consistency.
  • If RustSec maintainers or the library author think that this bug should get an informational advisory, I can update the PR.

[Shnatsel]

Ah, so there is arbitrary code execution potential in here. This has to be enough for an advisory. I'll merge it as-is then. Thanks again!

[Qwaz]

Thank you :)