Stream: t-lang/wg-unsafe-code-guidelines

Topic: Suggesting UB workarounds to crate authors


ecstatic-morse (Oct 18 2019 at 16:25, on Zulip):

There's a crate that tries to implement memoffset inside a static. As noted in that issue, there is currently no blessed way to do this inside a const context.

Unfortunately, the author used 0 as the address for the fake reference, which is no longer allowed by MIRI won't compile on the new beta. The obvious workaround for this is to use a non-zero usize as the address. I'm guessing the crate author will figure this out themselves. However, my question is whether should someone suggest this in a highly visible forum (like the linked github issue)? It may give the impression that this approach is officially sanctioned when it's not.

The other question is whether this deserves a CVE. There's a similar one for memoffset.

gnzlbg (Oct 18 2019 at 16:43, on Zulip):

The other question is whether this deserves a CVE. There's a similar one for memoffset.

cc @Shnatsel ^^

Shnatsel (Oct 18 2019 at 20:39, on Zulip):

In a nutshell: if that can cause an out-of-bounds read or write, or read uninitialized memory, or read/write memory that's been freed, open a CVE. Otherwise, don't.
I don't think a null reference observed by const evaluator would be exploitable. Maybe it can set off some chain of events that may lead up to one of the above - if that's the case, file a CVE; otherwise, don't.

ecstatic-morse (Oct 18 2019 at 21:08, on Zulip):

My guess is that it cannot, but I'm no good at exploits. Thanks Shnatsel .

RalfJ (Oct 19 2019 at 09:00, on Zulip):

As noted in that issue, there is currently no blessed way to do this inside a const context.

there's no blessed way to do this at all, in any context

RalfJ (Oct 19 2019 at 09:00, on Zulip):

what memoffset does is not blessed

RalfJ (Oct 19 2019 at 09:00, on Zulip):

it's just less un-blessed than the const "solutions" :D

RalfJ (Oct 19 2019 at 09:01, on Zulip):

@Shnatsel this is UB only in const context, I think. There's no way this can leak into run-time UB with the current compiler. The result of const-context UB is either (a) it does what the programmer expected [the UB was not detected] or (b) compilation aborts.

RalfJ (Oct 19 2019 at 09:01, on Zulip):

so I don't think this needs any kind of advisory, TBH

RalfJ (Oct 19 2019 at 09:02, on Zulip):

so, basically what you said :)

RalfJ (Oct 19 2019 at 09:04, on Zulip):

the memoffset CVE was IMO an overreaction; there's no known case of a program actually having any bad behavior due to this. sure, safe code could have caused drop-of-uninit-data with the API, but AFAIK the bar for a CVE isn't "the API is unsound", is it? Then every C library in existence would need a CVE as there is no such thing as a sound C API :D

RalfJ (Oct 19 2019 at 09:04, on Zulip):

to come back to @ecstatic-morse's question, I personally fairly freely share such alternative hacks, always with a big disclaimer that thy are wrong and bad hacks. I am pretty sure I posted the one for const-context offsetof on IRLO somewhere, let me check.

RalfJ (Oct 19 2019 at 09:05, on Zulip):

Here it is

Shnatsel (Oct 19 2019 at 09:06, on Zulip):

@ecstatic-morse for future reference, there is also https://github.com/RustSec/advisory-db which is Rust-specific but machine-readable. There is also cargo-audit that lets you audit your crate's dependencies against that database. This is complementary to CVE, usually with faster turnaround

RalfJ (Oct 19 2019 at 09:06, on Zulip):

notice however that replacing the 0 by a 1 will not actually make the example work -- and also the code I posted there doesn't work with current rustc betas. There isn't just no blessed way of doing this in const context right now, I literally do not know of a way to do it.

Shnatsel (Oct 19 2019 at 09:09, on Zulip):

"The API is unsound" could be a reason for CVE. It's just that Rust encodes the invariants in the type system while C relies on documentation. If there is something specifically blessed by documentation for C function and it causes a memory error, then it's a reason to open a CVE.

centril (Oct 19 2019 at 09:11, on Zulip):

Then every C library in existence would need a CVE as there is no such thing as a sound C API :D

You know, there's a reason we're not using C :slight_smile:
"The whole of C is a CVE" is not the worst idea. ^^

Shnatsel (Oct 19 2019 at 09:14, on Zulip):

In practice it basically is.
I have once reported a heap buffer overflow bug in a library that Firefox uses to decode VP8. It is exposed to untrusted inputs because HTML5 video autoplays, and the bug is relatively easy to exploit. The maintainers refused to file a CVE because "if they'd open a CVE for every such bug they fix, they'd never get any actual work done".

Shnatsel (Oct 19 2019 at 09:16, on Zulip):

And the horrifying state of Linux kernel is something I try really hard not to think about.

RalfJ (Oct 19 2019 at 09:16, on Zulip):

If there is something specifically blessed by documentation for C function and it causes a memory error, then it's a reason to open a CVE.

That's a good argument, thanks.

RalfJ (Oct 19 2019 at 09:16, on Zulip):

"if they'd open a CVE for every such bug they fix, they'd never get any actual work done"

:scream:

Shnatsel (Oct 19 2019 at 09:19, on Zulip):

Yeah, that was my reaction as well. This is when I gave up looking for bugs in commonly used C code and internalized that everything is broken.

RalfJ (Oct 19 2019 at 09:21, on Zulip):

I wonder what that means for all these studies that use CVEs as a measure to count security-critical bugs... better not to think about it^^

RalfJ (Oct 19 2019 at 09:22, on Zulip):

just that it makes Rust look relatively worse if we are more honest about reporting them :/

Shnatsel (Oct 19 2019 at 09:22, on Zulip):

Oh yeah, and this stance about CVEs is also fairly common among Linux kernel developers. I could probably find a dozen security bugs in the kernel right now that were fixed but not properly disclosed and as a result are not shipped by my distro.

Shnatsel (Oct 19 2019 at 09:24, on Zulip):

Aaaactually, security bugs in stdlib in Rust are also routinely fixed without opening a CVE. For like 2 out of 3 such bugs the CVE was filed retroactively by me, that's the only reason it exists.

RalfJ (Oct 19 2019 at 09:25, on Zulip):

fair^^

Shnatsel (Oct 19 2019 at 09:27, on Zulip):

But yeah, CVEs are wildly under-reported. And Rust still has less CVEs per year than, say, Python!

RalfJ (Oct 19 2019 at 09:30, on Zulip):

well Python is also much much much more widely used^^

HeroicKatora (Oct 19 2019 at 14:43, on Zulip):

Using CVEs for counting bugs is broken anyways. For the Linux Kernel there are plenty of stories of CVEs being filed to pad resumes, the average resolution time (days from filing to resolution) is -1000 days. Yes, negative. And then there is also CVE being used to circumvent bad corporate policies such as allowing backports in longterm branches strictly only in case of security fixes.

Lokathor (Oct 19 2019 at 16:13, on Zulip):

"There is no point to Rust, because it solves a problem that isn't there. I never have memory problems to begin with!"

gnzlbg (Oct 20 2019 at 15:44, on Zulip):

@Shnatsel is the term "CVE" in the context of Rust defined somewhere ?

gnzlbg (Oct 20 2019 at 15:44, on Zulip):

For Rust a "Rust-CVE" might be due to a safe API that's unsound

gnzlbg (Oct 20 2019 at 15:45, on Zulip):

That does not mean that there is an exploit in the wild (like CVEs document), but that there _might_ be one, which is a much higher bar.

gnzlbg (Oct 20 2019 at 15:45, on Zulip):

For C this bar doesn't make sense because then you need to use these CVEs for all code, but for Rust safe abstraction such a bar does make sense IMO

Shnatsel (Oct 20 2019 at 16:33, on Zulip):

Well, there is https://github.com/RustSec/advisory-db maintained by Rust Secure Code WG, and cargo audit checking dependencies against it. The bar for an advisory is "it's possible to write safe code that leads to memory safety violation in practice".

Shnatsel (Oct 20 2019 at 16:34, on Zulip):

https://rustsec.org/advisories/RUSTSEC-2019-0010.html - this is hard to exploit in practice and requires a very specific code to use the API, but it's not impossible, so it's an advisory.

RalfJ (Oct 21 2019 at 16:49, on Zulip):

Shnatsel is the term "CVE" in the context of Rust defined somewhere ?

uh... CVE is not ours to define, it's literally a thing that exists in the real world.^^

RalfJ (Oct 21 2019 at 16:50, on Zulip):

or do you mean if Rust has common standards for what is considered CVE-worthy? I doubt it.

RalfJ (Oct 21 2019 at 16:51, on Zulip):

@Shnatsel

Well, there is https://github.com/RustSec/advisory-db maintained by Rust Secure Code WG, and cargo audit checking dependencies against it. The bar for an advisory is "it's possible to write safe code that leads to memory safety violation in practice".

what does "in practice" mean here? is it sufficient to argue that e.g. LLVM has done similar optimizations in comparable cases in the past so the optimizations this UB enables could realistically occur -- or does one need to produce a SIGSEGV/SIGILL/...?

Shnatsel (Oct 21 2019 at 16:59, on Zulip):

I'd say "could realistically occur" is good enough, but we're still figuring that stuff out.
cargo-audit now has a mechanism for issuing warnings instead of hard failures, so if something e.g. uses a feature that's technically UB and happens to work now but we expect it to be broken in the future, then we can file that instead of a straight-up security advisory.

RalfJ (Oct 21 2019 at 16:59, on Zulip):

hm... I know of some such cases but the issue is they are not actionable

RalfJ (Oct 21 2019 at 17:00, on Zulip):

either no UB-free alternative exists or the maintainer doesn't want to use it because of a 30% hit in microbenchmarks.

Shnatsel (Oct 21 2019 at 17:00, on Zulip):

the latter could be a basis for a warning

Shnatsel (Oct 21 2019 at 17:01, on Zulip):

I believe offset_of is one such thing?

Shnatsel (Oct 21 2019 at 17:02, on Zulip):

Actually both could be a basis for a "warning" advisory. It could be helpful to inform people about this, maybe they could use a different approach instead of offset_of

RalfJ (Oct 21 2019 at 17:19, on Zulip):

I believe offset_of is one such thing?

it is the former, yes

RalfJ (Oct 21 2019 at 17:20, on Zulip):

Actually both could be a basis for a "warning" advisory. It could be helpful to inform people about this, maybe they could use a different approach instead of offset_of

do warnings show transitively?

RalfJ (Oct 21 2019 at 17:20, on Zulip):

crossbeam depends on memoffset so really most people cant do anything about it

gnzlbg (Oct 21 2019 at 18:07, on Zulip):

uh... CVE is not ours to define, it's literally a thing that exists in the real world.^^

I know, but Rust CVEs are CVEs that are also often filled in the advisory-db repo

gnzlbg (Oct 21 2019 at 18:07, on Zulip):

I think it is fine for advisory-db to accept issues that might not be accepted as CVEs

gnzlbg (Oct 21 2019 at 18:08, on Zulip):

Like "an unsound safe Rust API"

gnzlbg (Oct 21 2019 at 18:08, on Zulip):

that might not have a known CVE yet

RalfJ (Oct 21 2019 at 19:52, on Zulip):

I think it is fine for advisory-db to accept issues that might not be accepted as CVEs

I dont think anyone disputes that :)

DPC (Oct 27 2019 at 15:42, on Zulip):

We could call them "recommendations" or "guidelines"

Last update: Nov 19 2019 at 17:50UTC