From c5db2b1dc5f3b34b10b3b30c2ebcd9028d1525ac Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Sat, 9 Feb 2019 20:52:54 +0000 Subject: [PATCH 1/6] RFC changing the overflow behavior for usize in release builds to panic --- text/0000-usize-panic-overflow.md | 185 ++++++++++++++++++++++++++++++ 1 file changed, 185 insertions(+) create mode 100644 text/0000-usize-panic-overflow.md diff --git a/text/0000-usize-panic-overflow.md b/text/0000-usize-panic-overflow.md new file mode 100644 index 00000000000..cfa5d955d83 --- /dev/null +++ b/text/0000-usize-panic-overflow.md @@ -0,0 +1,185 @@ +- Feature Name: `usize_panic_on_overflow` +- Start Date: 2019-02-02 +- RFC PR: +- Rust Issue: + +# Summary +[summary]: #summary + +In `release` builds, adopt the current `debug` behavior of panicking on overflow +for arithmetic with `usize` (but not other sizes/types of integers). + +# Motivation +[motivation]: #motivation + +One of Rust's motivations is to make programming safer, particularly against +memory corruption vulnerabilities. In safe Rust code, integer overflows do not +lead to memory unsafety, however when combined with `unsafe`, integer overflow +is a frequent source of security vulnerabilities. We consider four examples: + +- [CVE-2018-1000810](https://groups.google.com/d/msg/rustlang-security-announcements/CmSuTm-SaU0/AzVznVcTCgAJ): + Integer overflow leading to heap buffer overflow in `str::repeat`. +- [CVE-2017-1000430](https://github.com/alicemaz/rust-base64/commit/24ead980daf11ba563e4fb2516187a56a71ad319): + Integer overflow leading to heap buffer overflow in the `base64` create. +- [CrosVM](https://bugs.chromium.org/p/chromium/issues/detail?id=892904): Integer + overflow leading to heap buffer overflow in ChromeOS's hypervisor. +- [ring](https://github.com/briansmith/ring/issues/742): Near-miss integer + overflow, which would have led to heap buffer overflow. + +A panic-by-default behavior would have ensured that none of these bugs would +have been a vulnerability. All four of these cases involved operations on +`usize` values. Because Rust is very strict in how integer types are handled, it +is exceedingly likely that overflows which lead to memory corruption will happen +on `usize` values: `usize` is consistently used in `core`/`std` APIs which deal +in lengths, buffer sizes, etc., the kind of values where overflow can be +dangerous, so it's no coincidence that historic integer-overflow vulnerabilities +occured with `usize` values. + +Rust's current behavior of `panic` in `debug` builds and twos complement +overflow in `release` builds does not provide protection against these +vulnerabilities. + +# Guide-level explanation +[guide-level-explanation]: #guide-level-explanation + +When an integer overflow is encountered when performing an arithmetic operation +(e.g. `+` or `*`), Rust has two possible different behaviors. In `debug` builds, +this will always cause a `panic`. In `release` builds the operation will succeed +and twos complement wrapping will occur - with one exception, if the operation +is being performed on `usize` integers you'll get the same `panic` as in a +`debug` build. + +For most use cases, simply using the default arithmetic operators works well, +however if you need more control, such as to avoid the `panic` and return a +clear error, several methods are available: + +- `checked_add` will return an `Option<$integer_type>`. + `200u8.checked_add(200u8)` will return `None`. +- `saturating_add` will return the maximum value the type can hold when an + integer overflow occurs. `200u8.saturating_add(200u8)` will return `255`. +- `wrapping_add` performs the arithmetic with twos complement overflow. + `200u8.wrapping_add(200u8)` will return `145`. + +If you are using `unsafe`, it's important that you be aware that wrapping +overflow can lead to memory corruption. See +[CWE-190](https://cwe.mitre.org/data/definitions/190.html) for more details. + +# Reference-level explanation +[reference-level-explanation]: #reference-level-explanation + +All current integer overflow semantics would remain the same, with one +exception: `usize` in `release` builds would gain the `panic` behavior it +currently has in `debug` builds. + +# Drawbacks +[drawbacks]: #drawbacks + +There are three arguments against making this change: + +1. Performance: Adding additional overflow checks on all `usize` arithmetic will + slow down programs. +2. Consistency: Having special behavior for just `usize` is weird, if we want + this behavior it should apply to all primitive integer types. +3. No changes at all: The current behavior is the desired end state, and thus we + shouldn't deviate from it. + +# Rationale and alternatives +[rationale-and-alternatives]: #rationale-and-alternatives + +The three design considerations I was attempting to balance in designing this +solution were performance, protection, and predictability. + +Performance is best served by adding the minimum number of checks in `release` +builds. We maximize protection against vulnerabilities by checking as many +things as possible. And a solution which is most consistent and easy to reason +about is best for predictability. + +To consider the extremes of these: + +- Adding no checks by default in `release` builds is best for performance, but + worst for protection. Is it predictable (though suffers from the fact that + many programmers do not think proactively about integer arithmetic as + fallible). +- Checking *all* arithmetic is worst for performance (and indeed was previously + rejected on these grounds in RFC 560), but best for protection. It's very + predictable. +- A data-flow analysis based insertion of overflow checks where the result + flowed into an allocation would be good for performance, because it would + avoid unnecessary checks. It would be ok for protection, depending on how + advanced it was (e.g. would it be inter-procedural?) However it would be + extremely unpredictable and difficult for programmers to reason about either + the performance or protection they were getting. + +Given these criteria, here's how this proposal fairs: + +**Protection**: because most of the sinks for APIs which are dangerous when +combined with overflows and unsafe operate on `usize`, this will cover the +majority of cases at risk of becoming vulnerabilities. A review of historical +integer-overflow vulnerabilities in Rust found that they were *all* `usize` +arithmetic, and thus protected by this proposal. + +**Performance**: This has a more mild performance impact than the previously +rejected check-all-arithmetic. I hypothesize that programs which rely most +heavily on arithmetic are not using `usize` (e.g. `curve25519-dalek`, which +was suggested to me as a good test of a such a crate, does not use `usize` for +representing limbs), however validating that the performance impact is +acceptable will require implementing this and measuring. + +**Predictability**: This proposal makes it relatively easy to look at a piece of +code and see whether it is protected, but the behavior may not be the most +intuitive to those not familiar with it. The biggest challenges I anticipate are +with code that looks like: + +``` +fn explode(x: u32, y: u32) -> Vec { + let v = vec![0; (x * y) as usize]; + for i in 0..x { + for j in 0..y { + unsafe { + v.set_unchecked(i * y + j, VALUE); + } + } + } + return v; +} +``` + +Which someone may expect to be protected, but is not. + +# Prior art +[prior-art]: #prior-art + +This builds on RFC 560, which defined the current semantics for integer +overflows. In many respects I see this as the minimal change to advance towards +the world I see as the desired-but-yet-realistic consequence of that RFC - +checking for overflow by default everywhere. + +I'm not aware of any other language which varies i's overflow behavior by type. +This suggests that the behavior may be surprising to many users. + +My assessment is that this is balanced by the fact that this would have +prevented all the vulnerabilities that have been seen thus far. + +# Unresolved questions +[unresolved-questions]: #unresolved-questions + +There are two major questions I see: + +- Is the performance really within the acceptable bounds? What are the + acceptable bounds? +- Do we need a method for obtaining the previous behavior of `panic` in `debug` + builds, but no checks in `release` builds? None of the overflow methods + currently provide this behavior, and so for people who explicitly want it, + under this proposal they'd need to write their own? +- Are additional options for controlling overflow behavior at the sub-crate + level required? + +# Future possibilities +[future-possibilities]: #future-possibilities + +The natural evolution would be towards overflow checking all arithmetic. It is +my hope that one product of this effort will be identifying small places that +optimizations and code generation can be improved to either further minimize the +overhead of overflow checks, and thus help enable this future. Nevertheless, I +do not believe that checking all arithmetic by default is a realistic short-term +possibility. From e73f65448d65cf96ecea9e407d1d4c0f77f031bd Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Sun, 10 Feb 2019 21:16:15 +0000 Subject: [PATCH 2/6] Fixed a handful of typos --- text/0000-usize-panic-overflow.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/text/0000-usize-panic-overflow.md b/text/0000-usize-panic-overflow.md index cfa5d955d83..dde9b7877a7 100644 --- a/text/0000-usize-panic-overflow.md +++ b/text/0000-usize-panic-overflow.md @@ -110,7 +110,7 @@ To consider the extremes of these: extremely unpredictable and difficult for programmers to reason about either the performance or protection they were getting. -Given these criteria, here's how this proposal fairs: +Given these criteria, here's how this proposal fares: **Protection**: because most of the sinks for APIs which are dangerous when combined with overflows and unsafe operate on `usize`, this will cover the @@ -151,10 +151,10 @@ Which someone may expect to be protected, but is not. This builds on RFC 560, which defined the current semantics for integer overflows. In many respects I see this as the minimal change to advance towards -the world I see as the desired-but-yet-realistic consequence of that RFC - +the world I see as the desired-but-not-yet-realistic consequence of that RFC - checking for overflow by default everywhere. -I'm not aware of any other language which varies i's overflow behavior by type. +I'm not aware of any other language which varies its overflow behavior by type. This suggests that the behavior may be surprising to many users. My assessment is that this is balanced by the fact that this would have From d3600e5856dd5b0da0abb52ede283f297db25c8f Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Sun, 10 Feb 2019 21:21:51 +0000 Subject: [PATCH 3/6] My own inability to count is proof of the necessity of this RFC --- text/0000-usize-panic-overflow.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-usize-panic-overflow.md b/text/0000-usize-panic-overflow.md index dde9b7877a7..e3f4ce401c1 100644 --- a/text/0000-usize-panic-overflow.md +++ b/text/0000-usize-panic-overflow.md @@ -163,14 +163,14 @@ prevented all the vulnerabilities that have been seen thus far. # Unresolved questions [unresolved-questions]: #unresolved-questions -There are two major questions I see: +There are three major questions I see: - Is the performance really within the acceptable bounds? What are the acceptable bounds? - Do we need a method for obtaining the previous behavior of `panic` in `debug` builds, but no checks in `release` builds? None of the overflow methods currently provide this behavior, and so for people who explicitly want it, - under this proposal they'd need to write their own? + under this proposal they'd need to write their own. - Are additional options for controlling overflow behavior at the sub-crate level required? From b25a927ead2e12dfa33e2f438cbb49e6ef204b6d Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Sun, 10 Feb 2019 22:04:18 +0000 Subject: [PATCH 4/6] Remove some stray words --- text/0000-usize-panic-overflow.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/text/0000-usize-panic-overflow.md b/text/0000-usize-panic-overflow.md index e3f4ce401c1..3d293e912d7 100644 --- a/text/0000-usize-panic-overflow.md +++ b/text/0000-usize-panic-overflow.md @@ -179,7 +179,7 @@ There are three major questions I see: The natural evolution would be towards overflow checking all arithmetic. It is my hope that one product of this effort will be identifying small places that -optimizations and code generation can be improved to either further minimize the -overhead of overflow checks, and thus help enable this future. Nevertheless, I -do not believe that checking all arithmetic by default is a realistic short-term +optimizations and code generation can be improved further minimize the overhead +of overflow checks, and thus help enable this future. Nevertheless, I do not +believe that checking all arithmetic by default is a realistic short-term possibility. From e67268e5795135beeffc443928f085b049a50f7f Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Mon, 11 Feb 2019 13:49:29 +0000 Subject: [PATCH 5/6] Clarify debug vs. debug_assertions --- text/0000-usize-panic-overflow.md | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/text/0000-usize-panic-overflow.md b/text/0000-usize-panic-overflow.md index 3d293e912d7..5996205105a 100644 --- a/text/0000-usize-panic-overflow.md +++ b/text/0000-usize-panic-overflow.md @@ -6,8 +6,8 @@ # Summary [summary]: #summary -In `release` builds, adopt the current `debug` behavior of panicking on overflow -for arithmetic with `usize` (but not other sizes/types of integers). +In `release` builds, adopt the current `debug_assertions` behavior of panicking +on overflow for arithmetic with `usize` (but not other sizes/types of integers). # Motivation [motivation]: #motivation @@ -35,19 +35,19 @@ in lengths, buffer sizes, etc., the kind of values where overflow can be dangerous, so it's no coincidence that historic integer-overflow vulnerabilities occured with `usize` values. -Rust's current behavior of `panic` in `debug` builds and twos complement -overflow in `release` builds does not provide protection against these -vulnerabilities. +Rust's current behavior of `panic` in `debug_assertions` builds and twos +complement overflow in `release` builds does not provide protection against +these vulnerabilities. # Guide-level explanation [guide-level-explanation]: #guide-level-explanation When an integer overflow is encountered when performing an arithmetic operation -(e.g. `+` or `*`), Rust has two possible different behaviors. In `debug` builds, -this will always cause a `panic`. In `release` builds the operation will succeed -and twos complement wrapping will occur - with one exception, if the operation -is being performed on `usize` integers you'll get the same `panic` as in a -`debug` build. +(e.g. `+` or `*`), Rust has two possible different behaviors. In +`debug_assertions` builds, this will always cause a `panic`. In `release` builds +the operation will succeed and twos complement wrapping will occur - with one +exception, if the operation is being performed on `usize` integers you'll get +the same `panic` as in a `debug_assertions` build. For most use cases, simply using the default arithmetic operators works well, however if you need more control, such as to avoid the `panic` and return a @@ -69,7 +69,7 @@ overflow can lead to memory corruption. See All current integer overflow semantics would remain the same, with one exception: `usize` in `release` builds would gain the `panic` behavior it -currently has in `debug` builds. +currently has in `debug_assertions` builds. # Drawbacks [drawbacks]: #drawbacks @@ -167,10 +167,10 @@ There are three major questions I see: - Is the performance really within the acceptable bounds? What are the acceptable bounds? -- Do we need a method for obtaining the previous behavior of `panic` in `debug` - builds, but no checks in `release` builds? None of the overflow methods - currently provide this behavior, and so for people who explicitly want it, - under this proposal they'd need to write their own. +- Do we need a method for obtaining the previous behavior of `panic` in + `debug_assertions` builds, but no checks in `release` builds? None of the + overflow methods currently provide this behavior, and so for people who + explicitly want it, under this proposal they'd need to write their own. - Are additional options for controlling overflow behavior at the sub-crate level required? From 2ded5c90907d11ee427b72763f30f6045e98230b Mon Sep 17 00:00:00 2001 From: James May Date: Thu, 2 May 2019 07:42:04 -0400 Subject: [PATCH 6/6] Update text/0000-usize-panic-overflow.md Co-Authored-By: alex --- text/0000-usize-panic-overflow.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-usize-panic-overflow.md b/text/0000-usize-panic-overflow.md index 5996205105a..f69f5bf8506 100644 --- a/text/0000-usize-panic-overflow.md +++ b/text/0000-usize-panic-overflow.md @@ -20,7 +20,7 @@ is a frequent source of security vulnerabilities. We consider four examples: - [CVE-2018-1000810](https://groups.google.com/d/msg/rustlang-security-announcements/CmSuTm-SaU0/AzVznVcTCgAJ): Integer overflow leading to heap buffer overflow in `str::repeat`. - [CVE-2017-1000430](https://github.com/alicemaz/rust-base64/commit/24ead980daf11ba563e4fb2516187a56a71ad319): - Integer overflow leading to heap buffer overflow in the `base64` create. + Integer overflow leading to heap buffer overflow in the `base64` crate. - [CrosVM](https://bugs.chromium.org/p/chromium/issues/detail?id=892904): Integer overflow leading to heap buffer overflow in ChromeOS's hypervisor. - [ring](https://github.com/briansmith/ring/issues/742): Near-miss integer