Rust: Query for uncontrolled allocation size #19171
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| extensions: | ||
| - addsTo: | ||
| pack: codeql/rust-all | ||
| extensible: sinkModel | ||
| data: | ||
| - ["repo:https://github.com/rust-lang/libc:libc", "::malloc", "Argument[0]", "alloc-size", "manual"] | ||
| - ["repo:https://github.com/rust-lang/libc:libc", "::aligned_alloc", "Argument[1]", "alloc-size", "manual"] | ||
| - ["repo:https://github.com/rust-lang/libc:libc", "::calloc", "Argument[0,1]", "alloc-size", "manual"] | ||
| - ["repo:https://github.com/rust-lang/libc:libc", "::realloc", "Argument[1]", "alloc-size", "manual"] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| extensions: | ||
| - addsTo: | ||
| pack: codeql/rust-all | ||
| extensible: sinkModel | ||
| data: | ||
| # Alloc | ||
| - ["lang:alloc", "crate::alloc::alloc", "Argument[0]", "alloc-layout", "manual"] | ||
| - ["lang:alloc", "crate::alloc::alloc_zeroed", "Argument[0]", "alloc-layout", "manual"] | ||
| - ["lang:alloc", "crate::alloc::realloc", "Argument[2]", "alloc-size", "manual"] | ||
| - ["lang:std", "<crate::alloc::System as crate::alloc::global::GlobalAlloc>::alloc", "Argument[0]", "alloc-layout", "manual"] | ||
| - ["lang:std", "<crate::alloc::System as crate::alloc::global::GlobalAlloc>::alloc_zeroed", "Argument[0]", "alloc-layout", "manual"] | ||
| - ["lang:std", "<crate::alloc::System as crate::alloc::Allocator>::allocate", "Argument[0]", "alloc-layout", "manual"] | ||
| - ["lang:std", "<crate::alloc::System as crate::alloc::Allocator>::allocate_zeroed", "Argument[0]", "alloc-layout", "manual"] | ||
| - ["lang:std", "<crate::alloc::System as crate::alloc::Allocator>::grow", "Argument[2]", "alloc-layout", "manual"] | ||
| - ["lang:std", "<crate::alloc::System as crate::alloc::Allocator>::grow_zeroed", "Argument[2]", "alloc-layout", "manual"] | ||
| - ["lang:alloc", "<crate::alloc::Global as crate::alloc::global::GlobalAlloc>::alloc", "Argument[0]", "alloc-layout", "manual"] | ||
| - ["lang:alloc", "<crate::alloc::Global as crate::alloc::global::GlobalAlloc>::alloc_zeroed", "Argument[0]", "alloc-layout", "manual"] | ||
| - ["lang:alloc", "<crate::alloc::Global as crate::alloc::Allocator>::allocate", "Argument[0]", "alloc-layout", "manual"] | ||
| - ["lang:alloc", "<crate::alloc::Global as crate::alloc::Allocator>::allocate_zeroed", "Argument[0]", "alloc-layout", "manual"] | ||
| - ["lang:alloc", "<crate::alloc::Global as crate::alloc::Allocator>::grow", "Argument[2]", "alloc-layout", "manual"] | ||
| - ["lang:alloc", "<crate::alloc::Global as crate::alloc::Allocator>::grow_zeroed", "Argument[2]", "alloc-layout", "manual"] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| /** | ||
| * Provides classes and predicates for reasoning about uncontrolled allocation | ||
| * size vulnerabilities. | ||
| */ | ||
|
|
||
| import rust | ||
| private import codeql.rust.Concepts | ||
| private import codeql.rust.dataflow.DataFlow | ||
| private import codeql.rust.dataflow.FlowSink | ||
| private import codeql.rust.controlflow.ControlFlowGraph as Cfg | ||
| private import codeql.rust.controlflow.CfgNodes as CfgNodes | ||
|
|
||
| /** | ||
| * Provides default sources, sinks and barriers for detecting uncontrolled | ||
| * allocation size vulnerabilities, as well as extension points for adding your own. | ||
| */ | ||
| module UncontrolledAllocationSize { | ||
| /** | ||
| * A data flow sink for uncontrolled allocation size vulnerabilities. | ||
| */ | ||
| abstract class Sink extends QuerySink::Range { | ||
| override string getSinkType() { result = "UncontrolledAllocationSize" } | ||
| } | ||
|
|
||
| /** | ||
| * A barrier for uncontrolled allocation size vulnerabilities. | ||
| */ | ||
| abstract class Barrier extends DataFlow::Node { } | ||
|
|
||
| /** | ||
| * A sink for uncontrolled allocation size from model data. | ||
| */ | ||
| private class ModelsAsDataSink extends Sink { | ||
| ModelsAsDataSink() { sinkNode(this, ["alloc-size", "alloc-layout"]) } | ||
| } | ||
|
|
||
| /** | ||
| * A barrier for uncontrolled allocation size that is an upper bound check / guard. | ||
| */ | ||
| private class UpperBoundCheckBarrier extends Barrier { | ||
|
There was a problem hiding this comment. Is it correct that if we had a guards library for Rust, then we could've used that here instead? There was a problem hiding this comment. This uses the barrier guard functionality in Which guards library are you thinking of? I wonder if you're thinking of range analysis actually - that would provide an actual value for the upper bound, which we don't really need here, but it would probably also improve accuracy just by covering less common cases better (like |
||
| UpperBoundCheckBarrier() { | ||
| this = DataFlow::BarrierGuard<isUpperBoundCheck/3>::getABarrierNode() | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Gets the operand on the "greater" (or "greater-or-equal") side | ||
| * of this relational expression, that is, the side that is larger | ||
| * if the overall expression evaluates to `true`; for example on | ||
| * `x <= 20` this is the `20`, and on `y > 0` it is `y`. | ||
| */ | ||
| private Expr getGreaterOperand(BinaryExpr op) { | ||
|
There was a problem hiding this comment. If these could potentially be useful in other queries we could add them as member predicates to There was a problem hiding this comment. Yes I have plans to do that, but I think we should add a There was a problem hiding this comment. That makes a lot of sense and I completely agree that There was a problem hiding this comment. Yes, though I think getting the class hierarchy right is the priority. I've created an internal issue tracking this. |
||
| op.getOperatorName() = ["<", "<="] and | ||
| result = op.getRhs() | ||
| or | ||
| op.getOperatorName() = [">", ">="] and | ||
| result = op.getLhs() | ||
| } | ||
|
|
||
| /** | ||
| * Gets the operand on the "lesser" (or "lesser-or-equal") side | ||
| * of this relational expression, that is, the side that is smaller | ||
| * if the overall expression evaluates to `true`; for example on | ||
| * `x <= 20` this is `x`, and on `y > 0` it is the `0`. | ||
| */ | ||
| private Expr getLesserOperand(BinaryExpr op) { | ||
| op.getOperatorName() = ["<", "<="] and | ||
| result = op.getLhs() | ||
| or | ||
| op.getOperatorName() = [">", ">="] and | ||
| result = op.getRhs() | ||
| } | ||
|
|
||
| /** | ||
| * Holds if comparison `g` having result `branch` indicates an upper bound for the sub-expression | ||
| * `node`. For example when the comparison `x < 10` is true, we have an upper bound for `x`. | ||
| */ | ||
| private predicate isUpperBoundCheck(CfgNodes::AstCfgNode g, Cfg::CfgNode node, boolean branch) { | ||
| exists(BinaryExpr cmp | g = cmp.getACfgNode() | | ||
| node = getLesserOperand(cmp).getACfgNode() and | ||
| branch = true | ||
| or | ||
| node = getGreaterOperand(cmp).getACfgNode() and | ||
| branch = false | ||
| or | ||
| cmp.getOperatorName() = "==" and | ||
| [cmp.getLhs(), cmp.getRhs()].getACfgNode() = node and | ||
| branch = true | ||
| or | ||
| cmp.getOperatorName() = "!=" and | ||
| [cmp.getLhs(), cmp.getRhs()].getACfgNode() = node and | ||
| branch = false | ||
| ) | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| <!DOCTYPE qhelp PUBLIC | ||
| "-//Semmle//qhelp//EN" | ||
| "qhelp.dtd"> | ||
| <qhelp> | ||
| <overview> | ||
|
|
||
| <p>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.</p> | ||
geoffw0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| <p>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 <code>--release</code> flag Rust performs two's complement wrapping, | ||
geoffw0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| with the result that less memory may be allocated than expected. This can lead to buffer overflow | ||
geoffw0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| incidents.</p> | ||
|
|
||
| </overview> | ||
| <recommendation> | ||
|
|
||
| <p>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 <code>checked_mul</code> that | ||
| does not wrap around.</p> | ||
|
|
||
| </recommendation> | ||
| <example> | ||
|
|
||
| <p>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 | ||
geoffw0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| provided, leading to less memory being allocated than other parts of the program expect.</p> | ||
geoffw0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| <sample src="UncontrolledAllocationSizeBad.rs" /> | ||
|
|
||
| <p>In the fixed example, the user input is checked against a maximum value. If the check fails an | ||
geoffw0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| error is returned, and both the multiplication and alloaction do not take place.</p> | ||
geoffw0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| <sample src="UncontrolledAllocationSizeGood.rs" /> | ||
|
|
||
| </example> | ||
| <references> | ||
|
|
||
| <li>The Rust Programming Language: <a href="https://doc.rust-lang.org/book/ch03-02-data-types.html#integer-overflow">Data Types - Integer Overflow</a>.</li> | ||
|
|
||
| </references> | ||
| </qhelp> | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| /** | ||
| * @name Uncontrolled allocation size | ||
| * @description Allocating memory with a size controlled by an external user can result in | ||
mchammer01 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * arbitrary amounts of memory being allocated. | ||
| * @kind path-problem | ||
| * @problem.severity recommendation | ||
| * @security-severity 7.5 | ||
| * @precision high | ||
| * @id rust/uncontrolled-allocation-size | ||
| * @tags reliability | ||
| * security | ||
| * external/cwe/cwe-770 | ||
| * external/cwe/cwe-789 | ||
| */ | ||
|
|
||
| import rust | ||
| import codeql.rust.Concepts | ||
| import codeql.rust.dataflow.DataFlow | ||
| import codeql.rust.dataflow.TaintTracking | ||
| import codeql.rust.dataflow.internal.DataFlowImpl | ||
| import codeql.rust.security.UncontrolledAllocationSizeExtensions | ||
|
|
||
| /** | ||
| * A taint-tracking configuration for uncontrolled allocation size vulnerabilities. | ||
| */ | ||
| module UncontrolledAllocationConfig implements DataFlow::ConfigSig { | ||
| import UncontrolledAllocationSize | ||
|
|
||
| predicate isSource(DataFlow::Node source) { source instanceof ActiveThreatModelSource } | ||
|
|
||
| predicate isSink(DataFlow::Node sink) { sink instanceof Sink } | ||
|
|
||
| predicate isBarrier(DataFlow::Node barrier) { barrier instanceof Barrier } | ||
| } | ||
|
|
||
| module UncontrolledAllocationFlow = TaintTracking::Global<UncontrolledAllocationConfig>; | ||
|
|
||
| import UncontrolledAllocationFlow::PathGraph | ||
|
|
||
| from UncontrolledAllocationFlow::PathNode source, UncontrolledAllocationFlow::PathNode sink | ||
| where UncontrolledAllocationFlow::flowPath(source, sink) | ||
| select sink.getNode(), source, sink, | ||
| "This allocation size is derived from a $@ and could allocate arbitrary amounts of memory.", | ||
| source.getNode(), "user-provided value" | ||
geoffw0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
|
|
||
| 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) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
|
|
||
| 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) { | ||
geoffw0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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) | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the difference between
alloc-sizeandalloc-layout?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alloc-sizeis an integer that controls the size of an allocation, whereasalloc-layoutis aLayoutthat controls an allocation. ALayoutis basically a glorified(size, alignment)pair, the result of a call such asstd::alloc::Layout::array::<u32>(v).The current design has taint flow through
Layoutconstructors, 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-sizesink directly in theLayoutconstructors, rather than a summary, and drop thealloc-layoutsinks (effectively assuming that you would only create aLayoutbecause you intend to do an allocation).Another alternative design would be to have taint/data flow to the
sizefield of theLayoutand havealloc-sizesinks on thesizefield of the allocations that use a layout.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! That clears things up for me. I think there is something to be said for the last approach, but for now we should probably just stick to the current approach 👍