Merge pull request #19171 from geoffw0/badalloc

Rust: Query for uncontrolled allocation size

Author: geoffw0

rust/ql/lib/codeql/rust/frameworks/libc.model.yml

@@ -3,5 +3,12 @@ extensions:
       pack: codeql/rust-all
       extensible: sourceModel
     data:
-      # Alloc
       - ["repo:https://github.com/rust-lang/libc:libc", "::free", "Argument[0]", "pointer-invalidate", "manual"]
+  - 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"]

rust/ql/lib/codeql/rust/frameworks/stdlib/lang-alloc.model.yml

@@ -1,4 +1,30 @@
 extensions:
+  - addsTo:
+      pack: codeql/rust-all
+      extensible: sourceModel
+    data:
+      # Alloc
+      - ["lang:alloc", "crate::alloc::dealloc", "Argument[0]", "pointer-invalidate", "manual"]
+  - 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"]
   - addsTo:
       pack: codeql/rust-all
       extensible: summaryModel
@@ -9,9 +35,3 @@ extensions:
       - ["lang:alloc", "<crate::string::String>::as_str", "Argument[self]", "ReturnValue", "taint", "manual"]
       - ["lang:alloc", "<crate::string::String>::as_bytes", "Argument[self]", "ReturnValue", "taint", "manual"]
       - ["lang:alloc", "<_ as crate::string::ToString>::to_string", "Argument[self]", "ReturnValue", "taint", "manual"]
-  - addsTo:
-      pack: codeql/rust-all
-      extensible: sourceModel
-    data:
-      # Alloc
-      - ["lang:alloc", "crate::alloc::dealloc", "Argument[0]", "pointer-invalidate", "manual"]

rust/ql/lib/codeql/rust/frameworks/stdlib/lang-core.model.yml

@@ -17,6 +17,21 @@ extensions:
       - ["lang:core", "<crate::slice::iter::Iter as crate::iter::traits::iterator::Iterator>::collect", "Argument[self].Element", "ReturnValue.Element", "value", "manual"]
       - ["lang:core", "<crate::slice::iter::Iter as crate::iter::traits::iterator::Iterator>::map", "Argument[self].Element", "Argument[0].Parameter[0]", "value", "manual"]
       - ["lang:core", "<crate::slice::iter::Iter as crate::iter::traits::iterator::Iterator>::for_each", "Argument[self].Element", "Argument[0].Parameter[0]", "value", "manual"]
+      # Layout
+      - ["lang:core", "<crate::alloc::layout::Layout>::from_size_align", "Argument[0]", "ReturnValue.Field[crate::result::Result::Ok(0)]", "taint", "manual"]
+      - ["lang:core", "<crate::alloc::layout::Layout>::from_size_align_unchecked", "Argument[0]", "ReturnValue", "taint", "manual"]
+      - ["lang:core", "<crate::alloc::layout::Layout>::array", "Argument[0]", "ReturnValue.Field[crate::result::Result::Ok(0)]", "taint", "manual"]
+      - ["lang:core", "<crate::alloc::layout::Layout>::repeat", "Argument[self]", "ReturnValue.Field[crate::result::Result::Ok(0)].Field[0]", "taint", "manual"]
+      - ["lang:core", "<crate::alloc::layout::Layout>::repeat", "Argument[0]", "ReturnValue.Field[crate::result::Result::Ok(0)].Field[0]", "taint", "manual"]
+      - ["lang:core", "<crate::alloc::layout::Layout>::repeat_packed", "Argument[self]", "ReturnValue.Field[crate::result::Result::Ok(0)]", "taint", "manual"]
+      - ["lang:core", "<crate::alloc::layout::Layout>::repeat_packed", "Argument[0]", "ReturnValue.Field[crate::result::Result::Ok(0)]", "taint", "manual"]
+      - ["lang:core", "<crate::alloc::layout::Layout>::extend", "Argument[self]", "ReturnValue.Field[crate::result::Result::Ok(0)].Field[0]", "taint", "manual"]
+      - ["lang:core", "<crate::alloc::layout::Layout>::extend", "Argument[0]", "ReturnValue.Field[crate::result::Result::Ok(0)].Field[0]", "taint", "manual"]
+      - ["lang:core", "<crate::alloc::layout::Layout>::extend_packed", "Argument[self]", "ReturnValue.Field[crate::result::Result::Ok(0)]", "taint", "manual"]
+      - ["lang:core", "<crate::alloc::layout::Layout>::extend_packed", "Argument[0]", "ReturnValue.Field[crate::result::Result::Ok(0)]", "taint", "manual"]
+      - ["lang:core", "<crate::alloc::layout::Layout>::align_to", "Argument[self]", "ReturnValue.Field[crate::result::Result::Ok(0)]", "taint", "manual"]
+      - ["lang:core", "<crate::alloc::layout::Layout>::pad_to_align", "Argument[self]", "ReturnValue", "taint", "manual"]
+      - ["lang:core", "<crate::alloc::layout::Layout>::size", "Argument[self]", "ReturnValue", "taint", "manual"]
       # Ptr
       - ["lang:core", "crate::ptr::read", "Argument[0].Reference", "ReturnValue", "value", "manual"]
       - ["lang:core", "crate::ptr::read_unaligned", "Argument[0].Reference", "ReturnValue", "value", "manual"]

rust/ql/lib/codeql/rust/security/UncontrolledAllocationSizeExtensions.qll

@@ -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 {
+    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) {
+    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
+    )
+  }
+}

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

@@ -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 a denial-of-service (DoS) attack.</p>
+
+<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,
+with the result that less memory than expected may be allocated. This can lead to buffer overflow
+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
+provided. This may lead to less memory being allocated than expected by other parts of the program.</p>
+<sample src="UncontrolledAllocationSizeBad.rs" />
+
+<p>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 allocation do not take place.</p>
+<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>

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

@@ -0,0 +1,45 @@
+/**
+ * @name Uncontrolled allocation size
+ * @description Allocating memory with a size controlled by an external user can result in
+ *              arbitrary amounts of memory being allocated, leading to a crash or a
+ *              denial-of-service (DoS) attack.
+ * @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"

rust/ql/src/queries/security/CWE-770/UncontrolledAllocationSizeBad.rs

@@ -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)
+    }
+}

rust/ql/src/queries/security/CWE-770/UncontrolledAllocationSizeGood.rs

@@ -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 {
+        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)
+    }
+}

rust/ql/src/queries/summary/Stats.qll

@@ -22,6 +22,7 @@ private import codeql.rust.security.CleartextLoggingExtensions
 private import codeql.rust.security.CleartextTransmissionExtensions
 private import codeql.rust.security.SqlInjectionExtensions
 private import codeql.rust.security.TaintedPathExtensions
+private import codeql.rust.security.UncontrolledAllocationSizeExtensions
 private import codeql.rust.security.WeakSensitiveDataHashingExtensions
 
 /**