Rust: Query for uncontrolled allocation size

[geoffw0]

New query rust/uncontrolled-allocation-size for uncontrolled allocations, that is, where user input is allowed to control the size of a memory allocation with no guard.

Note that there's a related issue when a size calculation can overflow, possibly leading to an under-allocation and consequent buffer overrun. The worst (but not uncommon) case is when these two issues coincide, giving an attacker the ability to control the buffer overrun. We probably want another query for overflowing arithmetic at some point to get good overall coverage.

TODO:

• code review
• docs review
• DCA run

[github-actions]

QHelp previews:

rust/ql/src/queries/security/CWE-770/UncontrolledAllocationSize.qhelp

Uncontrolled allocation size

Allocating memory with a size based on user input may allow arbitrary amounts of memory to be allocated, leading to a crash or denial of service incident.

If the user input is multiplied by a constant, such as the size of a type, the result may overflow. In a build with the --release flag Rust performs two's complement wrapping, with the result that less memory may be allocated than expected. This can lead to buffer overflow incidents.

Recommendation

Implement a guard to limit the amount of memory that is allocated, and reject the request if the guard is not met. Ensure that any multiplications in the calculation cannot overflow, either by guarding their inputs, or using a multiplication routine such as checked_mul that does not wrap around.

Example

In the following example, an arbitrary amount of memory is allocated based on user input. In addition, due to the multiplication operation the result may overflow if a very large value is provided, leading to less memory being allocated than other parts of the program expect.

fn allocate_buffer(user_input: String) -> Result<*mut u8, Error> {
    let num_bytes = user_input.parse::<usize>()? * std::mem::size_of::<u64>();

    let layout = std::alloc::Layout::from_size_align(num_bytes, 1).unwrap();
    unsafe {
        let buffer = std::alloc::alloc(layout); // BAD: uncontrolled allocation size

        Ok(buffer)
    }
}

In the fixed example, the user input is checked against a maximum value. If the check fails an error is returned, and both the multiplication and alloaction do not take place.

const BUFFER_LIMIT: usize = 10 * 1024;

fn allocate_buffer(user_input: String) -> Result<*mut u8, Error> {
    let size = user_input.parse::<usize>()?;
    if size > BUFFER_LIMIT {
        return Err("Size exceeds limit".into());
    }
    let num_bytes = size * std::mem::size_of::<u64>();

    let layout = std::alloc::Layout::from_size_align(num_bytes, 1).unwrap();
    unsafe {
        let buffer = std::alloc::alloc(layout); // GOOD

        Ok(buffer)
    }
}

References

• The Rust Programming Language: Data Types - Integer Overflow.
• Common Weakness Enumeration: CWE-770.
• Common Weakness Enumeration: CWE-789.

[Copilot]

Pull Request Overview

This pull request introduces a new query for detecting uncontrolled allocation sizes in Rust and adds corresponding test cases and model adjustments. Key changes include adding new test cases under CWE-770, implementing example queries for both good and bad allocation practices, and extending the CodeQL model definitions for allocation functions.

Reviewed Changes

Copilot reviewed 9 out of 15 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
rust/ql/test/query-tests/security/CWE-770/rust-toolchain.toml	Updates toolchain configuration for Rust tests
rust/ql/test/query-tests/security/CWE-770/options.yml	Adds dependency and cargo check options
rust/ql/test/query-tests/security/CWE-770/main.rs	Adds extensive test cases for uncontrolled allocation size scenarios
rust/ql/src/queries/security/CWE-770/UncontrolledAllocationSizeGood.rs	Provides a safe allocation example with proper bounds checking
rust/ql/src/queries/security/CWE-770/UncontrolledAllocationSizeBad.rs	Provides an unsafe allocation example without guards
rust/ql/lib/codeql/rust/frameworks/stdlib/lang-core.model.yml rust/ql/lib/codeql/rust/frameworks/stdlib/lang-alloc.model.yml rust/ql/lib/codeql/rust/frameworks/libc.model.yml	Updates and adds model entries pertaining to allocation functions

Files not reviewed (6)

• rust/ql/lib/codeql/rust/security/UncontrolledAllocationSizeExtensions.qll: Language not supported
• rust/ql/src/queries/security/CWE-770/UncontrolledAllocationSize.qhelp: Language not supported
• rust/ql/src/queries/security/CWE-770/UncontrolledAllocationSize.ql: Language not supported
• rust/ql/src/queries/summary/Stats.qll: Language not supported
• rust/ql/test/query-tests/diagnostics/SummaryStats.expected: Language not supported
• rust/ql/test/query-tests/security/CWE-770/UncontrolledAllocationSize.qlref: Language not supported

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

[geoffw0]

Yes, it's almost identical to the CPP version though.

[paldepind]

Looks great. I few comments and questions.

I wonder if it would also make sense to model Vec::with_capacity as a sink? That is higher level, so it might be used more out there.

[paldepind]

If these could potentially be useful in other queries we could add them as member predicates to BinaryExpr?

[geoffw0]

Yes I have plans to do that, but I think we should add a ComparisonOperation class and possibly clean up the existing BinaryExpr hierarchy. So it deserves a PR of its own.

[paldepind]

What is the difference between alloc-size and alloc-layout?

[geoffw0]

alloc-size is an integer that controls the size of an allocation, whereas alloc-layout is a Layout that controls an allocation. A Layout is basically a glorified (size, alignment) pair, the result of a call such as std::alloc::Layout::array::<u32>(v).

The current design has taint flow through Layout constructors, with sinks where the allocation is actually made. The query doesn't care which kind of sink, but I figured it might be better to tag them separately in case some future query does care about the difference (perhaps because it cares about alignments rather than sizes???).

An alternative and slightly simpler design would be to have an alloc-size sink directly in the Layout constructors, rather than a summary, and drop the alloc-layout sinks (effectively assuming that you would only create a Layout because you intend to do an allocation).

Another alternative design would be to have taint/data flow to the size field of the Layout and have alloc-size sinks on the size field of the allocations that use a layout.

[geoffw0]

I wonder if it would also make sense to model Vec::with_capacity as a sink?

Yes I think it does make sense, and there are a bunch of other container class methods that might have similar properties. My (optimistic) hope is that auto-modelling can fill in the gaps, and the test cases for vec here will serve as a confirmation of that (not comprehensive coverage).

[geoffw0]

DCA LGTM, a bit uneventful, it looks like we're not finding many sinks for this initial version of the query.