Undefined behavior in Rand

[vks]

No description provided.

[Shnatsel]

Is there any practical impact out of this? I'm not aware of unaligned accesses being exploitable in practice.

Does "provenance rules" mean "2 mutable pointers to the same thing"?

[dhardy]

Considering that the code in question should have been thoroughly tested on all our CI platforms to produce correct results, I think it quite unlikely there was any exploitable bug.

[vks]

Does "provenance rules" mean "2 mutable pointers to the same thing"?

No, they mean in this case that &mut slice[0] as *mut _ is a raw pointer that can only be used to access the first element. However, Rand used this pointer to access higher elements as well. slice.as_mut_ptr() has to be used instead.

[RalfJung]

Does this refer to rust-random/rand#780 or to rust-random/rand#784?

The former (rust-random/rand#780) is, to my knowledge, not de-facto UB with any existing Rust versions. Whether it is de jure UB is hard to say as Rust does not have an aliasing model at this point, but the code might become de jure and also de facto UB in the future. The provenance violation comes up in an experimental pointer provenance model for Rust (Stacked Borrows). While we do exploit provenance to some extend with noalias, I do not think rand violated noalias (but I will not make any definite statement about noalias; the LLVM spec is extremely fuzzy in that regard and likely inconsistent).

The latter (rust-random/rand#784) is de-facto UB as we tell LLVM that those accesses are aligned. It has nothing to do with pointer provenance though. Whether this UB actually leads to bad code generation or is exploitable, I do not know. (This comes back to the question I raised a while ago: is UB [de facto or de jure] sufficient for an advisory?)

EDIT: Oh I see, this adds advisories for both. Btw, it might be a good idea to also link to the issue and/or pull request so that one can get a better idea of what the flaw and fix look like. Not sure if the url field can be used multiple times?

[vks]

Btw, it might be a good idea to also link to the issue and/or pull request so that one can get a better idea of what the flaw and fix look like. Not sure if the url field can be used multiple times?

I added a link to the changelog, with contains the issue numbers. I'm also not sure whether I can specify multiple URLs instead.

[tarcieri]

We've had a few tough calls on whether something deserves an advisory in the past. In #124, the more we studied it, the more it looked exploitable. As I look at this one, I feel the opposite: with more analysis I'm less and less convinced there's anything exploitable here. My feeling on this is it isn't worth an advisory unless someone can present a credible threat (even if hypothetical and far fetched).

[tarcieri]

Closing this as unexploitable. Please reopen or refile if there is a credible threat.

[RalfJung]

Would it make sense to reopen this an an "informational (unsound)" advisory?

[tarcieri]

@RalfJung reopened for discussion at least

[vks]

I guess it would encourage people to upgrade Rand. Does RustSec have a policy for "informational" advisories?

[vks]

I rebased on master and removed the mention of pointer provenance rules.

[RalfJung]

This doesn't look like the right toml format (maybe it changed); see https://github.com/RustSec/advisory-db#advisory-format.

[RalfJung]

@vks that is probably also the cause of the CI failure, though the message is quite cryptic:

error: error loading advisory DB repo from .: RustSec error: parse error: missing field `versions` at line 1 column 1