Filed an advisory for ::safemem::prepend() reference to uninit memory

[danielhenrymantilla]

I don't know the severity of this soundness issue, as it's currently rather theoretical (currently, reference to uninit memory = UB), but I've preferred to submit an advisory anyways as suggested by @CAD97 in abonander/safemem#7. I'll let you be the judges of that.

[tarcieri]

Thanks for filing this. I think it's worth including but I'm double checking with others about it.

[CAD97]

My position is that ideally we want to normalize the practice of reporting safe ways of breaking soundness. It's easier to get things reported/recorded if there's less of a "is this severe enough" question.

The standard for a "please update" advisory should be "an unsafe contract was violated", not "a theoretically exploitable vector", because any UB is a theoretical point of entry.

(Perhaps there could be tiers for "exploitable UB" versus "dormant UB"? I also do sympathize with the human cost of maintaining the database.)

[tarcieri]

@CAD97 the larger issue is false positives / low exploitability, leading to alert fatigue. RUSTSEC-2019-0011 is an example of one of the more controversial advisories we still published, where #149 is an example we deemed unexploitable and therefore didn't publish.

[RalfJung]

So the bad ref stays inside that code, is never leaked to clients?

In terms of severity this is less severe than #149. There, we had unaligned accesses; those are UB in the LLVM IR we generate (and then God knows what LLVM does with that). I am sure there is a way to turn that into a miscompilation but honestly for me personally that's not an interesting exercise.

For uninitialized memory behind a reference, AFAIK the LLVM IR we generate is UB-free in current compiler versions (and we also don't exploit them on the MIR). There cannot be a miscompilation as of now. (Even current memoffset has uninit-mem-behind-a-ref as there is no way to avoid that when implementing offset_of.)

Note that this statement is based solely on the summary of the issue given here; I don't have time right now to dig deeper.

[tarcieri]

Based on what @RalfJung said I'm inclined to reject this one

[Shnatsel]

TL;DR: since the UB appears purely theoretical, i.e. the generated assembly is at present UB-free, I'd prefer not to alert people about this one.

We want RustSec alerts to always mean "yes this is really bad and you must upgrade" so that people do not get into the habit of ignoring security advisories. For example, npm has a history of low-impact advisories, so people tend to ignore the output completely - even though it contains important items, those are largely lost among the noise.

Since this is theoretically UB under stacked borrows model, but neither LLVM nor rustc takes advantage of this in any way and the reference is not exposed to external code (according to brief description and Ralf's post above), I do not see a way this could lead to UB in practice, i.e. at present the generated assembly is UB-free. And since the fixed version is available in a semver-compatible update, I do not expect people to get stuck on this particular version, so this is unlikely to become once a theoretical future compiler taking advantage of this UB is released.

[danielhenrymantilla]

Ok, I see we all agree this is currently unexploitable, unlikely to stay in the future due to semver, and that it would thus end up as a "practical" false positive, and we all know how damaging false positives can be.

So I will go and close this, but for other people doubting about submitting a PR, I think it is important to communicate that they should still go and submit a PR; that a closed PR which leads to several people discussing about a question of safety is not bad, even if the PR gets closed.

Moreover, having a tag like "Non-exploited [by the compiler] UB" (or something like that) be added on such PRs can be useful for future reference, to let these PRs be reopened in case of new cases of UB being exploited / used by the compiler.

[tarcieri]

@danielhenrymantilla absolutely! People should open PRs whenever they’re concerned there might be any sort of potential vulnerability