Java: Unbounded Allocation queries

java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.qhelp

@@ -0,0 +1,35 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+ When user input is used to control how much memory is allocated, such as in the size of an array, care should be taken to validate this number
+ and ensure it is within a sensible bound.
+ Otherwise, a malicous user may be able to cause a DoS (Denial of Service) attack by causing the application to consume a large amount of memory.
+</p>
+
+</overview>
+<recommendation>
+
+<p>Ensure that user-provided sizes are within some upper bound. 
+Alternatively, a structure such as <code>ArrayList</code> could be used to ensure that the application only allocates as much memory as is necassary.</p>
+
+</recommendation>
+<example>
+
+<p>
+The following example allocates an amount of memory based on a user provided value:
+</p>
+
+<sample src="UnboundedAllocationBad.java" />
+
+<p>The following example firt validates the uner input against an upper bound:</p>
+
+<sample src="UnboundedAllocationGood.java" />
+
+</example>
+<references>
+
+</references>
+</qhelp>

java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.ql

@@ -0,0 +1,28 @@
+/**
+ * @name Unbounded allocation
+ * @kind path-problem
+ * @problem.severity warning
+ * @precision medium
+ * @id java/unbounded-allocation
+ * @tags security
+ *       external/cwe/cwe-789
+ */
+
+import java
+import UnboundedAllocationCommon
+import semmle.code.java.dataflow.FlowSources
+import DataFlow::PathGraph
+
+class UnboundedAllocationConfig extends TaintTracking::Configuration {
+  UnboundedAllocationConfig() { this = "UnboundedAllocationConfig" }
+
+  override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof AllocationSink }
+
+  override predicate isSanitizer(DataFlow::Node node) { hasUpperBound(node.asExpr()) }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, UnboundedAllocationConfig config
+where config.hasFlowPath(source, sink)
+select sink, source, sink, "Unbounded memory allocation"

java/ql/src/Security/CWE/CWE-789/UnboundedAllocationBad.java

@@ -0,0 +1,9 @@
+public void processData(Socket sock) {
+    ObjectInputStream stream = new ObjectInputStream(sock.getInputStream());
+
+    int size = stream.readInt();
+    // BAD: A user-controlled amount of memory is alocated
+    byte[] buffer = new byte[size];
+
+    // ...
+}

java/ql/src/Security/CWE/CWE-789/UnboundedAllocationCommon.qll

@@ -0,0 +1,82 @@
+/**
+ * Common definitions for the unbounded allocation queries.
+ */
+
+import semmle.code.java.dataflow.RangeAnalysis
+import semmle.code.java.dataflow.FlowSteps
+import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.dataflow.TaintTracking
+
+/** A sink where memory is allocated. */
+class AllocationSink extends DataFlow::Node {
+  AllocationSink() {
+    this.asExpr() = any(ArrayCreationExpr a).getADimension()
+    or
+    exists(Call c, int i |
+      c.getArgument(i) = this.asExpr() and
+      c.getCallee().(AllocatingCallable).getParam() = i
+    )
+  }
+}
+
+/** A callable that allocates memory. */
+abstract class AllocatingCallable extends Callable {
+  /** Returns the parameter index controlling the size of the allocated memory. */
+  abstract int getParam();
+}
+
+private class AtomicArrayConstructor extends AllocatingCallable, Constructor {
+  AtomicArrayConstructor() {
+    this
+        .getDeclaringType()
+        .hasQualifiedName("java.util.concurrent.atomic",
+          ["AtomicIntArray", "AtomicLongArray", "AtomicReferenceArray"]) and
+    this.getParameterType(0) instanceof IntegralType
+  }
+
+  override int getParam() { result = 0 }
+}
+
+private class ListConstructor extends AllocatingCallable, Constructor {
+  ListConstructor() {
+    this.getDeclaringType().hasQualifiedName("java.util", ["ArrayList", "Vector"]) and
+    this.getParameterType(0) instanceof IntegralType
+  }
+
+  override int getParam() { result = 0 }
+}
+
+private class ReadMethod extends TaintPreservingCallable {
+  ReadMethod() {
+    this.getDeclaringType().hasQualifiedName("java.io", "ObjectInputStream") and
+    this.getName().matches("read%")
+  }
+
+  override predicate returnsTaintFrom(int arg) { arg = -1 }
+}
+
+private class ArithmeticStep extends TaintTracking::AdditionalTaintStep {
+  override predicate step(DataFlow::Node src, DataFlow::Node sink) {
+    exists(BinaryExpr binex | sink.asExpr() = binex and src.asExpr() = binex.getAnOperand() |
+      binex instanceof AddExpr
+      or
+      binex instanceof MulExpr
+      or
+      binex instanceof SubExpr
+      or
+      src.asExpr() = binex.getLeftOperand() and
+      (
+        binex instanceof DivExpr
+        or
+        binex instanceof LShiftExpr
+        or
+        binex instanceof RShiftExpr
+        or
+        binex instanceof URShiftExpr
+      )
+    )
+  }
+}
+
+/** Holds if `e` has a known upper bound. */
+predicate hasUpperBound(Expr e) { bounded(e, any(ZeroBound z), _, true, _) }

java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserialization.ql

@@ -0,0 +1,41 @@
+/**
+ * @name Unbounded allocation during deserialization
+ * @kind path-problem
+ * @problem.severity warning
+ * @precision medium
+ * @id java/unbounded-allocation-deserialization
+ * @tags security
+ *       external/cwe/cwe-789
+ */
+
+import java
+import UnboundedAllocationCommon
+import DataFlow::PathGraph
+
+class DeserializeMethod extends Method {
+  DeserializeMethod() {
+    this.getDeclaringType().getASourceSupertype*() instanceof TypeSerializable and
+    this.hasName("readObject") and
+    this.getParameterType(0).(RefType).hasQualifiedName("java.io", "ObjectInputStream")
+  }
+
+  Parameter getInputStreamParameter() { result = getParameter(0) }
+}
+
+class DeserializeParameter extends DataFlow::Node {
+  DeserializeParameter() { this.asParameter() = any(DeserializeMethod m).getInputStreamParameter() }
+}
+
+class UnboundedDeserializationConfig extends TaintTracking::Configuration {
+  UnboundedDeserializationConfig() { this = "UnboundedDeserializationConfig" }
+
+  override predicate isSource(DataFlow::Node src) { src instanceof DeserializeParameter }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof AllocationSink }
+
+  override predicate isSanitizer(DataFlow::Node node) { hasUpperBound(node.asExpr()) }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, UnboundedDeserializationConfig config
+where config.hasFlowPath(source, sink)
+select sink, source, sink, "Unbounded memory allocation from deserialization"

java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserializationBad.java

@@ -0,0 +1,22 @@
+class A implements Serializable{
+    transient Object[] data;
+
+    private void writeObject(java.io.ObjectOutputStream out) throws IOException {
+        out.writeInt(data.length);
+
+        for (int i = 0; i < data.length; i++) {
+            out.writeObject(data[i]);
+        }
+    }
+
+    private void readObject(java.io.ObjectInputStream in) throws IOException, ClassNotFoundException {
+        int length = in.readInt();
+
+        // BAD: An array is allocated whose size could be controlled by an attacker.
+        data = new Object[length];
+
+        for (int i = 0; i < length; i++) {
+            data[i] = in.readObject();
+        }
+    }
+}

java/ql/src/Security/CWE/CWE-789/UnboundedAllocationGood.java

@@ -0,0 +1,14 @@
+public void processData(Socket sock) {
+    ObjectInputStream stream = new ObjectInputStream(sock.getInputStream());
+
+    int size = stream.readInt();
+
+    if (size > 4096) {
+        throw new IOException("Size too large");
+    }
+
+    // GOOD: There is an upper bound on memory consumption.
+    byte[] buffer = new byte[size];
+
+    // ...
+}

java/ql/src/Security/CWE/CWE-789/UnbounedAllocationDeserializationGood.java

@@ -0,0 +1,24 @@
+class A implements Serializable{
+    transient Object[] data;
+
+    private void writeObject(java.io.ObjectOutputStream out) throws IOException {
+        out.writeInt(data.length);
+
+        for (int i = 0; i < data.length; i++) {
+            out.writeObject(data[i]);
+        }
+    }
+
+    private void readObject(java.io.ObjectInputStream in) throws IOException, ClassNotFoundException {
+        int length = in.readInt();
+
+        // GOOD: An ArrayList is used, which dynamically allocates memory as needed.
+        ArrayList<Object> dataList = new ArrayList<Object>();
+
+        for (int i = 0; i < length; i++) {
+            dataList.add(in.readObject());
+        }
+
+        data = dataList.toArray();
+    }
+}

java/ql/src/Security/CWE/CWE-789/UndoundedAllocationDeserialization.qhelp

@@ -0,0 +1,42 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+  A custom deserialization method may be expected to be run on an untrusted data stream. 
+  If so, care should be taken when allocating resources such as memory based on a number within that stream. 
+  Otherwise, an an attacker may be able to cause a DoS (Denial of Service) attack by providing the application with a malicously crafted
+  data stream causing it to consume an unbounded amount of memory.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+ If a deserialization method for a class is intended to be used on untrusted data, avoid allocating memory up front during deserialization, 
+ instead consider using a structure such as <code>ArrayList</code> that grows proportionally to the size of the input stream. 
+ Alternatively, a maximum size may be enforced.
+</p>
+
+</recommendation>
+<example>
+
+<p>
+The following example allocates an array of a size provided by the input stream, making it vulnerable to a DoS attack:
+</p>
+
+<sample src="UnboundedAllocationDeserializationBad.java" />
+
+<p>
+The following example uses an <code>ArrayList</code> to only allocate as much memory as needed, within a constant factor:
+</p>
+
+<sample src="UnboundedAllocationDeserializationGood.java" />
+
+</example>
+<references>
+
+<li>Example in the Guava framework: <a href="https://github.com/google/guava/wiki/CVE-2018-10237">CVE-2018-10237</a>, patched by <a href="https://github.com/google/guava/commit/f89ece5721b2f637fe754937ff1f3c86d80bb196">this commit</a>.</li>
+</references>
+</qhelp>

java/ql/test/query-tests/security/CWE-789/Test.java

@@ -0,0 +1,81 @@
+import java.io.*;
+import java.lang.ClassNotFoundException;
+import java.net.Socket;
+import java.util.ArrayList;
+
+class Test {
+
+    class A implements Serializable{
+        transient Object[] data;
+
+        private void writeObject(java.io.ObjectOutputStream out) throws IOException {
+            out.writeInt(data.length);
+
+            for (int i = 0; i < data.length; i++) {
+                out.writeObject(data[i]);
+            }
+        }
+
+        private void readObject(java.io.ObjectInputStream in) throws IOException, ClassNotFoundException {
+            int length = in.readInt();
+
+            // BAD: An array is allocated whose size could be controlled by an attacker.
+            data = new Object[length];
+
+            for (int i = 0; i < length; i++) {
+                data[i] = in.readObject();
+            }
+        }
+    }
+
+    class B implements Serializable{
+        transient Object[] data;
+
+        private void writeObject(java.io.ObjectOutputStream out) throws IOException {
+            out.writeInt(data.length);
+
+            for (int i = 0; i < data.length; i++) {
+                out.writeObject(data[i]);
+            }
+        }
+
+        private void readObject(java.io.ObjectInputStream in) throws IOException, ClassNotFoundException {
+            int length = in.readInt();
+
+            // GOOD: An ArrayList is used, which dynamically allocates memory as needed.
+            ArrayList<Object> dataList = new ArrayList<Object>();
+
+            for (int i = 0; i < length; i++) {
+                dataList.add(in.readObject());
+            }
+
+            data = dataList.toArray();
+        }
+    }
+
+
+    public void processData1(Socket sock) throws IOException {
+        ObjectInputStream stream = new ObjectInputStream(sock.getInputStream());
+
+        int size = stream.readInt();
+        // BAD: A user-controlled amount of memory is alocated
+        byte[] buffer = new byte[size];
+
+        // ...
+    }
+
+    public void processData2(Socket sock) throws IOException {
+        ObjectInputStream stream = new ObjectInputStream(sock.getInputStream());
+
+        int size = stream.readInt();
+
+        if (size > 4096) {
+            throw new IOException("Size too large");
+        }
+
+        // GOOD: There is an upper bound on memory consumption.
+        byte[] buffer = new byte[size];
+
+        // ...
+    }
+}