Stream: wg-secure-code

Topic: integer-overflow


Alex Gaynor (Oct 17 2018 at 02:06, on Zulip):

So to kick off some brainstorming... the combination of integer-overflows and unsafe code seems to be a relatively common source of vulnerabilities in unsafe code. This was the cause of the recent str::repeat issue, it was the cause of the vulnerability in base64, and it's popped up a few times in some rust code review I've been doing.

It seems like there's two clear solutions: a) safe abstractions so that people don't have unsafe code, b) abstractions for allocation size computations so that people aren't doing their own arithmetic. I imagine (a) will be a common theme as we figure out what all those patterns are. (b) is interesting. I wonder if there's any general solutions to the integer-overflow + unsafe code problem?

Jake Goulding (Oct 17 2018 at 02:08, on Zulip):

(b) feels like there will be a (relative) explosion in use as more allocation-related pieces get closer to stabilization

Jake Goulding (Oct 17 2018 at 02:09, on Zulip):

Could you provide a tiny concrete example of the problem? I'm only tangentially familiar with the details behind str::repeat and missed the base64 one.

Alex Gaynor (Oct 17 2018 at 02:09, on Zulip):

https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html does exist, but it's difficult to imagine people use this for things like "compute Vec length", where a simple multiplication is a lot shorter

Zach Reizner (Oct 17 2018 at 02:11, on Zulip):

The integer overflow issue is what got me involved.

Alex Gaynor (Oct 17 2018 at 02:11, on Zulip):

Sure, the basic problem is that str::repeat did let mut new_data = vec![self.len() * n; 0] and then wrote to new_data using unsafe. If self.len() * n overflows you allocate a smaller space than expected, and your writes go off the end of the array.

Alex Gaynor (Oct 17 2018 at 02:12, on Zulip):

https://github.com/rust-lang/rust/pull/54399/files#diff-773807740b9d7f176c85b4e2e34b2607L420

Zach Reizner (Oct 17 2018 at 02:12, on Zulip):

Our particular problem is that we need to allocate and fill structures that interface with the kernel. These structures tend to end with an unsized array.

Alex Gaynor (Oct 17 2018 at 02:14, on Zulip):

varsize structs seems like they should just have a safe allocation API. They're generated by bindgen right?

Jake Goulding (Oct 17 2018 at 02:14, on Zulip):

I'll throw the strawman proposal (c) out there: always enable overflow checks.

Zach Reizner (Oct 17 2018 at 02:14, on Zulip):

https://elixir.bootlin.com/linux/latest/source/arch/x86/include/uapi/asm/kvm.h#L182

Zach Reizner (Oct 17 2018 at 02:14, on Zulip):

^ As an example of one such structure

Zach Reizner (Oct 17 2018 at 02:15, on Zulip):

I'll throw the strawman proposal (c) out there: always enable overflow checks.

That's what we're experimenting with now in crosvm at least.

Alex Gaynor (Oct 17 2018 at 02:16, on Zulip):

It seems unlikely that Rust will go that way until the cost is brought down, much as it'd totally solve this problem.

Alex Gaynor (Oct 17 2018 at 02:16, on Zulip):

@Zach Reizner you generate the rust versions of these with bindgen right?

Zach Reizner (Oct 17 2018 at 02:17, on Zulip):

That's right

Zach Reizner (Oct 17 2018 at 02:17, on Zulip):

(AFK now, getting pulled away)

Alex Gaynor (Oct 17 2018 at 02:19, on Zulip):

Hmm, does Rust even have native support for var-sized structs with inline arrays, or is bindgen doing some hack to represent them? This case feels like it deserves a safe-allocation API.

Zach Reizner (Oct 17 2018 at 02:19, on Zulip):

For crosvm, we check in generated bindgen code

Zach Reizner (Oct 17 2018 at 02:20, on Zulip):

So possibly we are using out of date bindings

Zach Reizner (Oct 17 2018 at 02:20, on Zulip):

But it uses a generated incomplete array type to do zero length types inline

Zach Reizner (Oct 17 2018 at 02:20, on Zulip):

It has no allocation API that I'm aware of.

Alex Gaynor (Oct 17 2018 at 02:21, on Zulip):

To spitball another idea: a clippy lint that detected unchecked arithmetic that flowed into an allocation API. That'd have caught str::repeat, not base64 though (the allocation computation is in a separate function).

Alex Gaynor (Oct 17 2018 at 02:21, on Zulip):

Ok. Proposing an allocation API in bindgen seems like a good outcome.

Zach Reizner (Oct 17 2018 at 02:25, on Zulip):

That would help but it would not be sufficient for a safe API

Zach Reizner (Oct 17 2018 at 02:26, on Zulip):

You would have to make sure that the count field in the structure agrees with the allocation

Alex Gaynor (Oct 17 2018 at 02:27, on Zulip):

I'm not sure you can solve that part in a general-purpose way -- I don't think bindgen has any way of knowing which field is the length (if there is one!)

Zach Reizner (Oct 17 2018 at 02:28, on Zulip):

Maybe we could use some kind of annotation

Jake Goulding (Oct 17 2018 at 02:38, on Zulip):

What about something akin to how calloc takes two arguments: one for element size and one for count? Then that math could always be checked.

Tony Arcieri (Oct 17 2018 at 13:35, on Zulip):

I've wished there was an AbortOnOverflow in the same way there's Wrapping

Tony Arcieri (Oct 17 2018 at 13:35, on Zulip):

right now abort-on-overflow semantics are... not very ergonomic

Tony Arcieri (Oct 17 2018 at 13:36, on Zulip):

I intend on replacing these with a proper Accumulator type but... ick https://github.com/iqlusioninc/crates/blob/master/subtle-encoding/src/macros.rs

nikomatsakis (Oct 17 2018 at 15:24, on Zulip):

incidentally, it has always been the intention that code can locally enable stricter overflow checks (e.g., libstd might do this) — I know that some of the layout code in FF does this — does that seem relevant?

nikomatsakis (Oct 17 2018 at 15:24, on Zulip):

right now I think that is a bit difficult to do though

nikomatsakis (Oct 17 2018 at 15:25, on Zulip):

or at least not as elegant as originally envisioned

Tony Arcieri (Oct 17 2018 at 17:05, on Zulip):

Is there some feature for this I don't know about?

Tony Arcieri (Oct 17 2018 at 17:05, on Zulip):

#[overflow(abort)] or thereabouts would be great

Tony Arcieri (Oct 17 2018 at 17:06, on Zulip):

I basically want the debug overflow behavior in release builds

Jake Goulding (Oct 17 2018 at 17:06, on Zulip):

If you want that, you can have it

Tony Arcieri (Oct 17 2018 at 17:06, on Zulip):

but... selectively

Tony Arcieri (Oct 17 2018 at 17:06, on Zulip):

I guess the stuff I expect to wrap is explicitly Wrapping

Tony Arcieri (Oct 17 2018 at 17:06, on Zulip):

so if that still works, and I get debug overflow on release everywhere else, that'd be great

Jake Goulding (Oct 17 2018 at 17:06, on Zulip):

https://stackoverflow.com/questions/34054669/how-to-compile-and-run-an-optimized-rust-program-with-overflow-checking-enabled

Jake Goulding (Oct 17 2018 at 17:07, on Zulip):

JIC you were not aware

Tony Arcieri (Oct 17 2018 at 17:07, on Zulip):

oof, well I use debug_assertions to gate other stuff I don't want in release builds

Tony Arcieri (Oct 17 2018 at 17:08, on Zulip):

#[overflow(abort)] at fn-level granularity would be great

Jake Goulding (Oct 17 2018 at 17:08, on Zulip):

Oh, let me update that answer; -C overflow-checks exists now

Jake Goulding (Oct 17 2018 at 17:09, on Zulip):

I agree about the fn level, but probably need to decide how it affects called functions

Tony Arcieri (Oct 17 2018 at 17:36, on Zulip):

nice re: -C overflow-checks

Zach Reizner (Oct 17 2018 at 21:09, on Zulip):

An overflow attribute applied to modules or at fn-level would be nice. I've certainly had times where I wanted that.

Zach Reizner (Oct 17 2018 at 21:12, on Zulip):

I don't think it makes sense for it to effect called functions. Thinking inductively, if you're in a position where an overflow in your called functions can hurt you in this function, you're fixing the wrong function.

Zach Reizner (Oct 17 2018 at 21:13, on Zulip):

For example, I'm not concerned with functions like Vec::with_capacity overflowing because I trust them to check the input parameters

Zach Reizner (Oct 17 2018 at 21:15, on Zulip):

I think that that attribute can also be linted against with respect to unsafe code. That is, unsafe code that does not have overflow protection in some form (Wrapping, -C overflow checks, #[overflow(abort)]) should throw up a warning on arithmetic.

Zach Reizner (Oct 17 2018 at 21:17, on Zulip):

I was reluctant about adding -C overflow to crosvm this week because I was afraid of performance regressions, but I would have enabled it in a heartbeat on some of our scarier unsafe modules.

Joshua Liebow-Feeser (Oct 17 2018 at 21:17, on Zulip):

What about functions which operate on integers but also return them? E.g., as a silly stramwn, a fn mul(a: usize, b: usize) -> usize

Joshua Liebow-Feeser (Oct 17 2018 at 21:17, on Zulip):

The author of mul is probably OK with overflow, and so doesn't check for it

Joshua Liebow-Feeser (Oct 17 2018 at 21:18, on Zulip):

But the consumer probably doesn't want overflow.

Alex Gaynor (Oct 17 2018 at 21:18, on Zulip):

That's the exact pattern in base64. compute_size had the overflow, but the actual flow into String::with_capacity() was elsewhere.

Joshua Liebow-Feeser (Oct 17 2018 at 21:20, on Zulip):

What is compute_size supposed to do? Because I'd argue that, if it does what I expect based on the name, it'd make more sense for it to return Option<usize> and be responsible for checking overflow.

Alex Gaynor (Oct 17 2018 at 21:20, on Zulip):

That was the fix for the security issue, yes :-)

Zach Reizner (Oct 17 2018 at 21:21, on Zulip):

Could you link that overflow bug for base64 compute_size for reference?

Alex Gaynor (Oct 17 2018 at 21:21, on Zulip):

https://github.com/alicemaz/rust-base64/commit/24ead980daf11ba563e4fb2516187a56a71ad319

Zach Reizner (Oct 17 2018 at 21:22, on Zulip):

Thanks

Zach Reizner (Oct 17 2018 at 21:25, on Zulip):

So it looks like the unsafe is in the same module as the integer overflow.

Zach Reizner (Oct 17 2018 at 21:26, on Zulip):

I think a lint or mod level #![overflow(abort)] would have caught that.

Zach Reizner (Oct 17 2018 at 21:26, on Zulip):

Or would have if that existed :P

Joshua Liebow-Feeser (Oct 17 2018 at 21:26, on Zulip):

It strikes me that there's a fundamental issue here, which is that unsafe code is not supposed to rely on safe code to be correct.

Joshua Liebow-Feeser (Oct 17 2018 at 21:27, on Zulip):

In this case, you're right that a mod level attribute would have done it, but what about unsafe code which uses another crate like the strawman offset_utils, which provides safe functions that do the math?

Zach Reizner (Oct 17 2018 at 21:30, on Zulip):

I don't have a great answer for that.

Alex Gaynor (Oct 17 2018 at 21:31, on Zulip):

Does cargo provide a way to set a compilation option for a specific dependency?

Joshua Liebow-Feeser (Oct 17 2018 at 21:31, on Zulip):

In fairness, that example demonstrates a broader issue: even if overflow is fixed, there's nothing to prevent that crate from providing non-overflowing but still wrong results that would still be just as bad for soundness.

Zach Reizner (Oct 17 2018 at 21:31, on Zulip):

My hot take is that we can't make it impossible to make unsafe code that will backfire, but it should at least be as obvious as possible.

Joshua Liebow-Feeser (Oct 17 2018 at 21:31, on Zulip):

That's a reasonable hot take.

Joshua Liebow-Feeser (Oct 17 2018 at 21:31, on Zulip):

@Alex Gaynor No, I don't think it does.

Joshua Liebow-Feeser (Oct 17 2018 at 21:31, on Zulip):

I suppose such crates could manually provide Cargo features to do the same, though.

Zach Reizner (Oct 17 2018 at 21:32, on Zulip):

What scared me was that I was pretty confident about the quality of our unsafe code, but there was the hidden Spectre of unchecked overflow

Alex Gaynor (Oct 17 2018 at 21:32, on Zulip):

Just filed https://github.com/rust-lang-nursery/rust-bindgen/issues/1423 to start the discussion on the bindgen side.

Zach Reizner (Oct 17 2018 at 21:33, on Zulip):

However, if someone tried to submit unsafe code that depended on a lot of outer module math, I'd ask them how confident they were in that outer module.

Joshua Liebow-Feeser (Oct 17 2018 at 21:34, on Zulip):

I wonder how reasonable it would be to advance a best practice of having unsafe code only rely on other code from the same crate or code from std/core?

Zach Reizner (Oct 17 2018 at 21:35, on Zulip):

I think that would make using FFI too hard.

Joshua Liebow-Feeser (Oct 17 2018 at 21:35, on Zulip):

Sorry, I should amend: unsafe code should only rely on code from other crates if that code is also unsafe, and thus the implementor has also opted in to reasoning about memory safety themselves.

Joshua Liebow-Feeser (Oct 17 2018 at 21:36, on Zulip):

Does that address the concern about FFI?

Zach Reizner (Oct 17 2018 at 21:37, on Zulip):

I think that idea is worth considering.

Joshua Liebow-Feeser (Oct 17 2018 at 21:37, on Zulip):

Although I suppose the counter-point is that we shouldn't be encouraging authors to write unsafe APIs.

Joshua Liebow-Feeser (Oct 17 2018 at 21:39, on Zulip):

E.g., I've been working recently on some utilities to add automatic allocation, freeing, and reference counting to C types. That has a partially-unsafe API for the bits that the FFI has to plug in, but it provides a safe API for most of its operations. In turn, I use it from unsafe code.

Zach Reizner (Oct 17 2018 at 21:39, on Zulip):

Is there any precedent for giving core/std extra levels of trust? That seems unfair to other high quality crates.

Joshua Liebow-Feeser (Oct 17 2018 at 21:39, on Zulip):

I guess not. That's a fair point about other high quality crates.

Zach Reizner (Oct 17 2018 at 21:40, on Zulip):

(not to tangent this stream too much, but was there an existing proposal to have a curated set of such high quality crates)

Joshua Liebow-Feeser (Oct 17 2018 at 21:40, on Zulip):

Yeah, I've got concerns about such an idea. I'm worried it'll stagnate the ecosystem too much.

Zach Reizner (Oct 17 2018 at 21:41, on Zulip):

I like the idea, but you bring up a very good point.

Joshua Liebow-Feeser (Oct 17 2018 at 21:42, on Zulip):

In general, I think we should prefer technical solutions over social ones if we can find them. Social solutions tend to have ramifications which are harder to predict and harder to control.

Joshua Liebow-Feeser (Oct 17 2018 at 21:42, on Zulip):

Plus, it's easier to reason about the extent to which technical solutions will solve the target problem.

Zach Reizner (Oct 17 2018 at 21:42, on Zulip):

And even applying the high quality label can be concerning. Zlib2's website is a gigantic list of CVEs, libssh2 had a pretty bad bug this week, and even libstd has it's growing collecting of CVEs

Jake Goulding (Oct 17 2018 at 21:43, on Zulip):

not to tangent this stream too much,

Remember you can make new zulip threads very easy ;-)

Joshua Liebow-Feeser (Oct 17 2018 at 21:43, on Zulip):

Yeah, that's a good point.

Alex Gaynor (Oct 17 2018 at 21:43, on Zulip):

(libssh, not libssh2, I think?)

Zach Reizner (Oct 17 2018 at 21:44, on Zulip):

Sorry, you're correct.

Zach Reizner (Oct 17 2018 at 21:44, on Zulip):

(that was directed at Jake and Alex)

nikomatsakis (Oct 18 2018 at 19:00, on Zulip):

It strikes me that there's a fundamental issue here, which is that unsafe code is not supposed to rely on safe code to be correct.

This is too strong. But it's true that the code which unsafe code uses is part of its TCB. But as an extreme example I think it's right and proper for unsafe code to rely on Vec.

nikomatsakis (Oct 18 2018 at 19:01, on Zulip):

I would say something like "unsafe code should be clear about which other code it relies upon and what it requires of that code"

Joshua Liebow-Feeser (Oct 18 2018 at 19:01, on Zulip):

Right.

Joshua Liebow-Feeser (Oct 18 2018 at 19:03, on Zulip):

It's just made confusing by the fact that, e.g., the following is generally considered unsound because it doesn't compose well with the space of all safe code: pub fn deref<T>(ptr: usize) -> &T { ... }

Joshua Liebow-Feeser (Oct 18 2018 at 19:03, on Zulip):

In other words, there's at least part of the API surface of unsafe code which is supposed to be robust against arbitrary safe code.

Joshua Liebow-Feeser (Oct 18 2018 at 19:04, on Zulip):

It's probably worth trying to make that explicit - which parts need to be robust against arbitrary safe code, and which parts are allowed to call the safe code part of the TCB?

Shnatsel (Oct 18 2018 at 20:04, on Zulip):

#[overflow(abort)] at fn-level granularity would be great

https://github.com/llogiq/overflower does pretty much this

Alex Gaynor (Oct 18 2018 at 20:17, on Zulip):

Looks like the bindgen folks are up for providing an allocation API for varsized structs: https://github.com/rust-lang-nursery/rust-bindgen/issues/1423 :tada: If anyone wants to run with this, go for it, otherwise I'll probably pick it up in a week or two.

Joshua Liebow-Feeser (Oct 18 2018 at 20:18, on Zulip):

@Shnatsel That's really cool. I hadn't seen that before.

Shnatsel (Oct 18 2018 at 20:18, on Zulip):

Sadly it's nightly only and will probably stay that way. A.k.a. it's a no-go for actual production code.

Shnatsel (Oct 18 2018 at 20:23, on Zulip):

And even applying the high quality label can be concerning. Zlib2's website is a gigantic list of CVEs, libssh2 had a pretty bad bug this week, and even libstd has it's growing collecting of CVEs

I have been (tangentially) involved in both Rust libstd CVEs, and based on what I've seen, libstd is going to get a lot more CVEs as soon as someone decides to actually audit or fuzz it. It doesn't have more CVEs against it due to lack of auditing, not due to lack of vulnerabilities in it.

This led me to contemplate automatically generating fuzz harnesses for stateless stdlib functions. I've written a prototype with just the vulnerable function where only 3 lines are function-specific, without any auto-generation for now, but it fails to discover the issue on my machine - probably because only one billionth of the 64-bit address space is actually sensible, everything else gets you an OOM crash. I need to try this in a 32-bit chroot, it will probably crash there, but I didn't get around to it. There is an alternative take on the str::repeat fuzz harness, not sure if that detects the bug though.

Anyway, I will figure out the individual fuzz harness on my own. I could use some help with auto-generating code based on stdlib function definitions. The task is fairly trivial, but I've never used syn before, and didn't get a chance to spend some time figuring it out yet.

Zach Reizner (Oct 18 2018 at 20:26, on Zulip):

Anyway, I will figure out the individual fuzz harness on my own. I could use some help with auto-generating code based on stdlib function definitions. The task is fairly trivial, but I've never used syn before, and didn't get a chance to spend some time figuring it out yet.

syn and quote are ok once you work an example or two.

Zach Reizner (Oct 18 2018 at 20:28, on Zulip):

Looks like the bindgen folks are up for providing an allocation API for varsized structs: https://github.com/rust-lang-nursery/rust-bindgen/issues/1423 :tada: If anyone wants to run with this, go for it, otherwise I'll probably pick it up in a week or two.

That's great news. It looks like the author wants someone else to write it though.

Alex Gaynor (Oct 18 2018 at 20:29, on Zulip):

Yeah. I'll plan to get to it it, unless someone beats me to it.

Tony Arcieri (Oct 18 2018 at 23:50, on Zulip):

@Shnatsel neat

Tony Arcieri (Oct 19 2018 at 15:12, on Zulip):

heh, forgot about this re: -C overflow-checks

Tony Arcieri (Oct 19 2018 at 15:12, on Zulip):

warning: profiles for the non root package will be ignored, specify profiles at the workspace root:

Tony Arcieri (Oct 19 2018 at 15:13, on Zulip):

overflower looks like it does what I want though

Tony Arcieri (Oct 19 2018 at 15:13, on Zulip):

oh wait, nightly-only

Zach Reizner (Oct 19 2018 at 16:51, on Zulip):

Could we do an RFC to include that functionality in the stable compiler?

Tony Arcieri (Oct 19 2018 at 17:02, on Zulip):

I would be a big fan of that

Shnatsel (Oct 19 2018 at 17:12, on Zulip):

SMACK can also be used for auditing code for integer overflows statically, so you don't have to inject runtime checks that degrade performance: http://soarlab.org/publications/atva2018-bhr.pdf

Shnatsel (Oct 19 2018 at 17:14, on Zulip):

It only works in reasonable time on standalone functions, not entire projects, but that's not really an issue since you usually want individual functions to be secure, not some functions behaving poorly but the rest of the code being structured in a way that masks that. That would be a pretty nasty non-local invariant.

Tony Arcieri (Oct 19 2018 at 17:16, on Zulip):

@Zach Reizner guess I'll make a topic for RFC ideas

Tony Arcieri (Nov 18 2018 at 21:17, on Zulip):

so, this change really is equivalent, right? /cc @nikomatsakis

Tony Arcieri (Nov 18 2018 at 21:17, on Zulip):

aside from the specifics of how I was aborting

Tony Arcieri (Nov 18 2018 at 21:17, on Zulip):

because if so that's pretty awesome

Tony Arcieri (Nov 18 2018 at 21:17, on Zulip):

https://github.com/iqlusioninc/crates/pull/122/files

Tony Arcieri (Nov 18 2018 at 21:18, on Zulip):

particularly awesome in that it's a one-liner for an entire workspace

Tony Arcieri (Nov 18 2018 at 21:20, on Zulip):

it's also a little terrifying in that it's a single place someone could inadvertently shut it off for a whole workspace :open_mouth:

Alex Gaynor (Nov 18 2018 at 21:20, on Zulip):

Write a test!

Tony Arcieri (Nov 18 2018 at 21:21, on Zulip):

I mean, how can I test it short of finding an integer overflow bug and exploiting it?

Tony Arcieri (Nov 18 2018 at 21:21, on Zulip):

I guess I could test in each crate that it's on at all

Tony Arcieri (Nov 18 2018 at 21:21, on Zulip):

I guess it'd need to be something in a release build

Alex Gaynor (Nov 18 2018 at 21:22, on Zulip):

Right, have a test that verifies that usize::MAX + 1 panics.

Tony Arcieri (Nov 18 2018 at 21:22, on Zulip):

how would I test the release profile though?

Tony Arcieri (Nov 18 2018 at 21:23, on Zulip):

there's uhh... #[lazy] or whatever, but that seems like the wrong solution

Alex Gaynor (Nov 18 2018 at 21:23, on Zulip):

Oh hmm; does cargo test have a --release flag?

Tony Arcieri (Nov 18 2018 at 21:23, on Zulip):

not sure, never tried to run tests in release mode

RalfJ (Nov 19 2018 at 08:47, on Zulip):

Oh hmm; does cargo test have a --release flag?

It does

nikomatsakis (Nov 19 2018 at 19:54, on Zulip):

so, this change really is equivalent, right? /cc @nikomatsakis

@Tony Arcieri btw I am not sure which change you are referring to, or what it should be equivalent to -- but I am in favor of allowing people to enable overflow checks on stable even in release builds.

I'm less sure about them disable overflow checks in debug builds.

nikomatsakis (Nov 19 2018 at 19:56, on Zulip):

@Shnatsel

This led me to contemplate automatically generating fuzz harnesses for stateless stdlib functions. I

that sounds great btw =)

Tony Arcieri (Nov 19 2018 at 20:15, on Zulip):

@nikomatsakis aah sorry, it was this PR https://github.com/iqlusioninc/crates/pull/122/files

Tony Arcieri (Nov 19 2018 at 20:16, on Zulip):

it replaces usage of explicit checked_* with a workspace-wide [profile.release] overflow-checks = true

nikomatsakis (Nov 19 2018 at 20:24, on Zulip):

ah, I see

nikomatsakis (Nov 19 2018 at 20:24, on Zulip):

presumably you have panic=abort too

nikomatsakis (Nov 19 2018 at 20:25, on Zulip):

sounds equiv, anyway

Tony Arcieri (Nov 19 2018 at 20:30, on Zulip):

I don't have panic = abort and I suppose that's notable.

Tony Arcieri (Nov 19 2018 at 20:30, on Zulip):

however the previous behavior was just a panic. an abort() would probably be preferred for the integer overflow cases

Tony Arcieri (Nov 19 2018 at 20:31, on Zulip):

the change just seemed like such an ergonomic improvement in the code I went through with it

Tony Arcieri (Nov 19 2018 at 20:31, on Zulip):

eliminating arithmetic macros with... normal arithmetic syntax

nikomatsakis (Nov 19 2018 at 20:32, on Zulip):

I don't have panic = abort and I suppose that's notable.

well I just saw this comment

# Enable integer overflow checks in release builds
# WARNING: DO NOT REMOVE THIS! We rely on this for abort-on-overflow semantics
# in the event of integer arithmetic bugs.

perhaps I took "abort" too literally, @Tony Arcieri

Tony Arcieri (Nov 19 2018 at 21:06, on Zulip):

I think ideally I'd abort instead of panic on overflow

Tony Arcieri (Nov 19 2018 at 21:07, on Zulip):

like if I could have an annotation that does exactly what I'd want, it'd probably be #[overflow(abort)] or thereabouts

Shnatsel (Nov 19 2018 at 21:37, on Zulip):

Why is integer overflow so special that it deserves an abort while other issues may panic? If anything, I'd make overflows panic and other issues about. Although a consistent policy sounds like the best option

Joshua Liebow-Feeser (Nov 19 2018 at 22:47, on Zulip):

Is overflow behavior configurable on a per-crate basis? I didn't realize that.

Tony Arcieri (Nov 20 2018 at 18:02, on Zulip):

@Joshua Liebow-Feeser per-workspace basis, but yes

Tony Arcieri (Nov 20 2018 at 18:02, on Zulip):

in my case I consider per-workspace an advantage

Joshua Liebow-Feeser (Nov 20 2018 at 20:06, on Zulip):

Ah, so I couldn't publish a crate on crates.io with that configuration?

nikomatsakis (Nov 20 2018 at 20:49, on Zulip):

you can

nikomatsakis (Nov 20 2018 at 20:49, on Zulip):

it's not recommended

nikomatsakis (Nov 20 2018 at 20:49, on Zulip):

er, not sure what exactly you are referring to actually

nikomatsakis (Nov 20 2018 at 20:50, on Zulip):

the panic=abort vs panic=unwind setting is "global" -- you can force it one way or the other, but it means you won't be interoperable with crates that force the other way

Joshua Liebow-Feeser (Nov 20 2018 at 20:50, on Zulip):

I'm referring to putting the appropriate configuration in my crate's Cargo.toml before publishing, with the expectation that the compiler would emit overflow checks for all of the code in my crate.

nikomatsakis (Nov 20 2018 at 20:50, on Zulip):

ok, you mean the overflow checks

Joshua Liebow-Feeser (Nov 20 2018 at 20:50, on Zulip):

yeah

nikomatsakis (Nov 20 2018 at 20:50, on Zulip):

I'm actually .. not entirely sure how that works, but our intention when discussing the overflow checks way back when was crates could opt in

nikomatsakis (Nov 20 2018 at 20:50, on Zulip):

i.e., independently from one another

nikomatsakis (Nov 20 2018 at 20:50, on Zulip):

but also that it could be forced on you

Joshua Liebow-Feeser (Nov 20 2018 at 20:51, on Zulip):

OK interesting.

nikomatsakis (Nov 20 2018 at 20:51, on Zulip):

basically, I wanted to encourage people to think of overflow as a bug, even if it's too costly to check for it all the time

Joshua Liebow-Feeser (Nov 20 2018 at 20:51, on Zulip):

Right

nikomatsakis (Nov 20 2018 at 20:51, on Zulip):

(and if you want wrapping, you should use the methods)

Joshua Liebow-Feeser (Nov 20 2018 at 20:51, on Zulip):

And I guess it's a pre-linking thing, even for #[inline] functions, so it's fine to be a per-crate option

nikomatsakis (Nov 20 2018 at 20:51, on Zulip):

yeah, so, I remember that inlining was a tricky case

nikomatsakis (Nov 20 2018 at 20:52, on Zulip):

(in particular, I think that libstd is always compiled release and hence without overflow checks, and we were inlining the MIR for Add impls and so forth, and it was therefore without checks, but I think we fixed that .. somehow)

nikomatsakis (Nov 20 2018 at 20:53, on Zulip):

@eddyb would maybe remember

nikomatsakis (Nov 20 2018 at 20:53, on Zulip):

Ah, https://github.com/rust-lang/rust/pull/33905

nikomatsakis (Nov 20 2018 at 20:53, on Zulip):

anyway, not answering your question :) just reminiscing

nikomatsakis (Nov 20 2018 at 20:53, on Zulip):

your question is sort of a cargo one and I don't really know

nikomatsakis (Nov 20 2018 at 20:53, on Zulip):

but there should be some way

nikomatsakis (Nov 20 2018 at 20:53, on Zulip):

if not, we should fix that

nikomatsakis (Nov 20 2018 at 20:53, on Zulip):

(imo)

nikomatsakis (Nov 20 2018 at 20:54, on Zulip):

I'd be ok with an attribute too in the crate, personally

nikomatsakis (Nov 20 2018 at 20:54, on Zulip):

#![overflow_check(panic)] or something

Joshua Liebow-Feeser (Nov 20 2018 at 20:54, on Zulip):

Ah, gotcha. Either would work.

nikomatsakis (Nov 20 2018 at 20:55, on Zulip):

I just personally don't want people to be able to turn them off, because then the meaning of a + b is context dependent

Joshua Liebow-Feeser (Nov 20 2018 at 20:55, on Zulip):

Sure, but presumably if the release-mode default is not to check, then opting in can only make things more robust, not less?

Tony Arcieri (Nov 24 2018 at 19:52, on Zulip):

yeah the whole abort thing is a big red herring, sorry. and I should probably change my comment to reflect it's a panic but not an abort

Tony Arcieri (Nov 24 2018 at 19:52, on Zulip):

comment in my code above

Tony Arcieri (Nov 24 2018 at 19:54, on Zulip):

I'd have a weak preference for aborting instead of panicking, lest an attack figure out a way to leverage catch_unwind to avoid an exit and exploit the overflow, but I'll take a panic for now

Alex Gaynor (Nov 24 2018 at 19:56, on Zulip):

Is there any reason to think panic might be more severe than a DoS?

Tony Arcieri (Nov 24 2018 at 20:03, on Zulip):

yeah, depends on the circumstances, and the severity of failure to handle an integer overflow correctly

Tony Arcieri (Nov 24 2018 at 20:03, on Zulip):

a panic is probably sufficient for most

Tony Arcieri (Nov 24 2018 at 20:05, on Zulip):

aborting would be nice if the overflow could be used to say, corrupt memory or leak cryptographic key material

Shnatsel (Nov 24 2018 at 22:09, on Zulip):

Actually, panicking is less safe in an environment where you want to prevent arbitrary code execution because writing unsafe code that's panic-safe and fast at the same time is really, really hard. To wit: https://rustsec.org/advisories/RUSTSEC-2018-0003.html - and that's code written by Servo developers, who know their stuff and many of whom have worked on Rust itself

Shnatsel (Nov 24 2018 at 22:14, on Zulip):

It was fixed by leaking memory on panic instead of performing a double free.

briansmith (Nov 25 2018 at 03:20, on Zulip):

@Tony Arcieri You probably do want to run your tests in release mode. If you have to choose one of release/debug I would choose release.

Tony Arcieri (Nov 25 2018 at 03:42, on Zulip):

@briansmith as it were, my tests include #[cfg(debug_assertions))]-gated mocks which generate compile_error!s if they are compiled into release builds

Tony Arcieri (Nov 25 2018 at 03:50, on Zulip):

I would really really really like to keep those mocks out of production builds, so them's the breaks

Tony Arcieri (Nov 25 2018 at 04:48, on Zulip):

(mocks for HSMs)

Tony Arcieri (Nov 25 2018 at 19:19, on Zulip):

@briansmith well as it were I was just bitten by not running tests in release mode, and just changed the impacted repository to do so (or rather, there is an open PR)

Tony Arcieri (Jan 17 2019 at 08:18, on Zulip):

sorry to bring this up again, but I just want to double/triple check here

Tony Arcieri (Jan 17 2019 at 08:18, on Zulip):

setting the following on a workspace's Cargo.toml:

Tony Arcieri (Jan 17 2019 at 08:18, on Zulip):
[profile.release]
overflow-checks = true
Tony Arcieri (Jan 17 2019 at 08:18, on Zulip):

that only applies to crates in that workspace, correct?

Tony Arcieri (Jan 17 2019 at 08:19, on Zulip):

i.e. does it or does it not enable those checks on transitive dependencies?

nikomatsakis (Jan 17 2019 at 14:00, on Zulip):

I'm actually not sure about that

nikomatsakis (Jan 17 2019 at 14:00, on Zulip):

@Alex Crichton would likely know, or maybe @simulacrum

nikomatsakis (Jan 17 2019 at 14:00, on Zulip):

and/or @Pietro Albini =)

nikomatsakis (Jan 17 2019 at 14:00, on Zulip):

/me tags everybody

Pietro Albini (Jan 17 2019 at 14:01, on Zulip):

let's also tag @kennytm, just to get everyone in there :D

nikomatsakis (Jan 17 2019 at 14:01, on Zulip):

or we could RTFM

nikomatsakis (Jan 17 2019 at 14:01, on Zulip):

Cargo supports custom configuration of how rustc is invoked through profiles at the top level. Any manifest may declare a profile, but only the top level package’s profiles are actually read. All dependencies’ profiles will be overridden. This is done so the top-level package has control over how its dependencies are compiled.

Pietro Albini (Jan 17 2019 at 14:01, on Zulip):

dunno by the way

nikomatsakis (Jan 17 2019 at 14:01, on Zulip):

from https://doc.rust-lang.org/cargo/reference/manifest.html

nikomatsakis (Jan 17 2019 at 14:01, on Zulip):

suggests that it applies to everything, @Tony Arcieri

nikomatsakis (Jan 17 2019 at 14:02, on Zulip):

@Pietro Albini sorry :blush: I somehow imagine you know everything about cargo after having done enough crater runs

nikomatsakis (Jan 17 2019 at 14:02, on Zulip):

not sure why that would be

Pietro Albini (Jan 17 2019 at 14:02, on Zulip):

I mean, I learned a lot about cargo :P

Pietro Albini (Jan 17 2019 at 14:02, on Zulip):

but we just strip workspace configuration during crater runs, so shrugs

nikomatsakis (Jan 17 2019 at 14:02, on Zulip):

=)

simulacrum (Jan 17 2019 at 15:08, on Zulip):

Yeah it should apply to all crates within the workspace (not sure if you've figured that out yet)

simulacrum (Jan 17 2019 at 15:08, on Zulip):

er, all crates + dependencies

simulacrum (Jan 17 2019 at 15:09, on Zulip):

Though I believe dependencies could override that, not sure

Tony Arcieri (Jan 17 2019 at 15:54, on Zulip):

I knew crates in the workspace... the dependencies are what I'm unclear on

Shnatsel (Jan 17 2019 at 18:20, on Zulip):

I'm sure overflow checks do not apply to the standard library. That's something to keep in mind.

Tony Arcieri (Jan 17 2019 at 18:21, on Zulip):

okay saw the docs now, hmm. that really implies I should really be using explicit checked I/O syntax everywhere in my crates and not depending on overflow-checks = true

Shnatsel (Jan 17 2019 at 18:49, on Zulip):

Maybe you could expose that as a feature and make it default?
Also, https://github.com/llogiq/overflower would come in handy if it weren't nightly-only

nikomatsakis (Jan 17 2019 at 23:37, on Zulip):

@Tony Arcieri yeah it seems like there may be a missing knob here

Tony Arcieri (Jan 17 2019 at 23:39, on Zulip):

@nikomatsakis I was a bit weirded out by using the profile setting in the first place, because it seems non-explicit in the code, and I think this pushed me over again

Tony Arcieri (Jan 17 2019 at 23:40, on Zulip):

@nikomatsakis what I want now is this, but as a first-class language feature https://crates.io/crates/checked

Tony Arcieri (Jan 17 2019 at 23:40, on Zulip):

effectively the opposite of Wrapping

Tony Arcieri (Jan 17 2019 at 23:40, on Zulip):

I'll probably start with the crate, but uhh

Tony Arcieri (Jan 17 2019 at 23:41, on Zulip):

I don't want to tell everyone doing cryptography in Rust to use this one crate and then that one crate becomes a single point of compromise for any cryptography in Rust. :scream:

Shnatsel (Jan 17 2019 at 23:46, on Zulip):

Has anyone done any tests to check if overflow behavior specified in Cargo in a dependency actually applies to that dependency if toplevel crate does not specify anything? If the toplevel binary crate does not force any specific behavior, keeping the library's behavior sounds reasonable, and if it's not done currently it can be introduced as a (hopefully) non-breaking change

Tony Arcieri (Jan 17 2019 at 23:48, on Zulip):

that's what we were just talking about. it is documented as parent crates overriding their dependencies

Tony Arcieri (Jan 17 2019 at 23:48, on Zulip):

but now I'm just terrified to do it implicitly

Tony Arcieri (Jan 17 2019 at 23:48, on Zulip):

Checked gets you all of the ergonomics of doing it implicitly, except it's explicit and not controlled in a spooky-action-at-a-distance fashion by some knob located who knows where

Tony Arcieri (Jan 17 2019 at 23:49, on Zulip):

also having Wrapping be a first-class part of the language, but not Checked, feels... inconsistent

briansmith (Jan 18 2019 at 01:46, on Zulip):

@Tony Arcieri Adding Checkedto the standard library seems like a reasonable task for this WG to take up.

briansmith (Jan 18 2019 at 01:48, on Zulip):

I also think we should be able to allow/deny/forbid the default unchecked arithmetic at granularity of our choosing.

Tony Arcieri (Jan 18 2019 at 05:55, on Zulip):

I agree! I might take a crack at writing up a pre-RFC post to rust-internals

Tony Arcieri (Jan 18 2019 at 05:55, on Zulip):

I remember several years ago finding Wrapping and being like "ok, great, now where's Checked?" (I was implementing something that happened to need them both)

Alex Gaynor (Feb 06 2019 at 01:24, on Zulip):

So, an interesting idea was suggested to me recently: enable overflow-check-and-panic on release builds for usize only. A huge percentage of exploitable integer overflow -> heap buffer overflow will be with usize (citation needed, but both of the ones I just checked were usize). And since it's only a small fraction of overall arithmetic, it'll have a much lower performance penalty than doing _all_ arithmetic.

Zach Reizner (Feb 06 2019 at 01:31, on Zulip):

What about u32 * u32 as usize style code?

Alex Gaynor (Feb 06 2019 at 04:08, on Zulip):

as in (u32 * u32) as usize. It wouldn't get protection.

Alex Gaynor (Feb 06 2019 at 04:08, on Zulip):

The design criteria were a) minimize performance overhead, b) maximize protection, c) easy to reason about

Tony Arcieri (Feb 06 2019 at 17:05, on Zulip):

@Alex Gaynor I tried enabling it across the board in projects... the problem is you don't get fine-grained control right now

Tony Arcieri (Feb 06 2019 at 17:06, on Zulip):

e.g. if I do that to prevent overflow bugs in my app logic, it also applies to curve25519-dalek

Tony Arcieri (Feb 06 2019 at 17:06, on Zulip):

where they want the checks in debug builds, but not in release builds for performance reasons

Alex Gaynor (Feb 06 2019 at 22:29, on Zulip):

They should use .wrapping_add() then

Tony Arcieri (Feb 06 2019 at 22:55, on Zulip):

like I can ask HdV but I think they want the overflow checks during debug builds

Alex Gaynor (Feb 06 2019 at 22:57, on Zulip):

Hmm, there's not an actual explicit API for that, just the default-but-not-garaunteed behavior.

Tony Arcieri (Feb 06 2019 at 22:57, on Zulip):

yeah...

Tony Arcieri (Feb 06 2019 at 22:58, on Zulip):

that's why I'm... less excited

Tony Arcieri (Feb 06 2019 at 22:58, on Zulip):

about overflow-checks = true for release builds

Alex Gaynor (Feb 06 2019 at 22:58, on Zulip):

Does curve25519-dalek actually use usize for representing limbs?

Tony Arcieri (Feb 06 2019 at 23:04, on Zulip):

going to guess not ¯\_(ツ)_/¯

Alex Gaynor (Feb 06 2019 at 23:05, on Zulip):

So it's unimpacted by my proposal :-)

Tony Arcieri (Feb 06 2019 at 23:06, on Zulip):

probably!

Alex Gaynor (Feb 06 2019 at 23:18, on Zulip):

If no one wants to tell me this is a horrible idea, I guess I'll write up an RFC this weekend

Tony Arcieri (Feb 06 2019 at 23:19, on Zulip):

curve25519-dalek is probably a good acid test... maybe ask them if they foresee any slowdowns... but otherwise SGTM

Tony Arcieri (Feb 06 2019 at 23:19, on Zulip):

usize in general seems like not the thing to use in performance-oriented code

Tony Arcieri (Feb 06 2019 at 23:20, on Zulip):

so hopefully no collateral damage

Tony Arcieri (Feb 06 2019 at 23:20, on Zulip):

I think usize is (or will be) 128-bit on riscv64 targets :thinking:

Shnatsel (Feb 07 2019 at 18:45, on Zulip):

There was a bit of non-public discussion about it, and someone from the Midori team in Microsoft shared their experiences on this. In their experience overflow checks introduce negligible performance overhead across the board; very rarely you would need to explicitly opt out of them for performance.

Shnatsel (Feb 07 2019 at 18:48, on Zulip):

I think overflow checks for usize alone are actually a great idea. I would prefer to have a way to express "I want this optimized out in release mode, but overflow in here is still a bug" though, and check that in debug mode.

Last update: Nov 11 2019 at 22:40UTC