Skip to content

Latest commit

 

History

History
185 lines (148 loc) · 8.31 KB

0000-usize-panic-overflow.md

File metadata and controls

185 lines (148 loc) · 8.31 KB
Raw
  • Feature Name: usize_panic_on_overflow
  • Start Date: 2019-02-02
  • RFC PR:
  • Rust Issue:

Summary

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

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: Integer overflow leading to heap buffer overflow in str::repeat.
  • CVE-2017-1000430: Integer overflow leading to heap buffer overflow in the base64 crate.
  • CrosVM: Integer overflow leading to heap buffer overflow in ChromeOS's hypervisor.
  • ring: 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_assertions builds and twos complement overflow in release builds does not provide protection against these vulnerabilities.

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_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 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 for more details.

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_assertions builds.

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

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 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 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<u8> {
    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

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-not-yet-realistic consequence of that RFC - checking for overflow by default everywhere.

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 prevented all the vulnerabilities that have been seen thus far.

Unresolved questions

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_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?

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 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.