diff --git a/java/ql/src/Security/CWE/CWE-789/UnboudnedAllocationDeserializationGood.java b/java/ql/src/Security/CWE/CWE-789/UnboudnedAllocationDeserializationGood.java new file mode 100644 index 000000000000..4bbc732b0c85 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-789/UnboudnedAllocationDeserializationGood.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 dataList = new ArrayList<>(); + + for (int i = 0; i < length; i++) { + dataList.add(in.readObject()); + } + + data = dataList.toArray(); + } +} diff --git a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.qhelp b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.qhelp new file mode 100644 index 000000000000..18bae01552fe --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.qhelp @@ -0,0 +1,35 @@ + + + +

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

+ +
+ + +

Ensure that user-provided sizes are within some upper bound. +Alternatively, a structure such as an ArrayList can be used to ensure that the application only allocates as much memory as is necessary.

+ +
+ + +

+The following example allocates an amount of memory directly from a user-provided value: +

+ + + +

The following example first validates the user input against an upper bound:

+ + + +
+ + + +
diff --git a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.ql b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.ql new file mode 100644 index 000000000000..2af2ed148851 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.ql @@ -0,0 +1,29 @@ +/** + * @name Unbounded allocation + * @description Allocating an unbounded amount of memory from a user-provided value can lead to a Denial of Service attack. + * @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 from $@.", source, "this user input" diff --git a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationBad.java b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationBad.java new file mode 100644 index 000000000000..1d990c1237d3 --- /dev/null +++ b/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 allocated + byte[] buffer = new byte[size]; + + // ... +} diff --git a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationCommon.qll b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationCommon.qll new file mode 100644 index 000000000000..56ee21ef753f --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationCommon.qll @@ -0,0 +1,72 @@ +/** + * Common definitions for the unbounded allocation queries. + */ + +import semmle.code.java.dataflow.RangeAnalysis +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", + ["AtomicIntegerArray", "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 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 + binex instanceof LShiftExpr + or + src.asExpr() = binex.getLeftOperand() and + ( + binex instanceof DivExpr + 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, _) } diff --git a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserialization.qhelp b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserialization.qhelp new file mode 100644 index 000000000000..c19abe97af9f --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserialization.qhelp @@ -0,0 +1,41 @@ + + + +

+ 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 attacker may be able to cause a DoS (Denial of Service) attack by providing the application with a maliciously crafted + data stream causing it to consume an unbounded amount of memory. +

+ +
+ + +

+ 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 ArrayList that grows proportionally to the size of the input stream. + Alternatively, a maximum size may be enforced. +

+ +
+ + +

+The following example allocates an array of a size provided by the input stream, making it vulnerable to a DoS attack: +

+ + + +

+The following example uses an ArrayList to only allocate as much memory as needed, within a constant factor: +

+ + +
+ + +
  • Example in the Guava framework: CVE-2018-10237, patched by this commit.
  • +
    +
    diff --git a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserialization.ql b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserialization.ql new file mode 100644 index 000000000000..de06880d820a --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserialization.ql @@ -0,0 +1,43 @@ +/** + * @name Unbounded allocation during deserialization + * @description Allocating an unbounded amount of memory during a deserialization method can lead to a Denial of Service attack. + * @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 during deserialization of $@.", source, + "this input stream" diff --git a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserializationBad.java b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserializationBad.java new file mode 100644 index 000000000000..b665dfc510c4 --- /dev/null +++ b/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(); + } + } +} \ No newline at end of file diff --git a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationGood.java b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationGood.java new file mode 100644 index 000000000000..1400fca3a003 --- /dev/null +++ b/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]; + + // ... +} \ No newline at end of file diff --git a/java/ql/test/query-tests/security/CWE-789/Test.java b/java/ql/test/query-tests/security/CWE-789/Test.java new file mode 100644 index 000000000000..2d059cc34a7c --- /dev/null +++ b/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 dataList = new ArrayList<>(); + + 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 allocated + 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]; + + // ... + } +} diff --git a/java/ql/test/query-tests/security/CWE-789/UnboundedAllocation.expected b/java/ql/test/query-tests/security/CWE-789/UnboundedAllocation.expected new file mode 100644 index 000000000000..920a0b7a03d6 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-789/UnboundedAllocation.expected @@ -0,0 +1,7 @@ +edges +| Test.java:58:58:58:78 | getInputStream(...) : InputStream | Test.java:62:34:62:37 | size | +nodes +| Test.java:58:58:58:78 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream | +| Test.java:62:34:62:37 | size | semmle.label | size | +#select +| Test.java:62:34:62:37 | size | Test.java:58:58:58:78 | getInputStream(...) : InputStream | Test.java:62:34:62:37 | size | Unbounded memory allocation from $@. | Test.java:58:58:58:78 | getInputStream(...) : InputStream | this user input | diff --git a/java/ql/test/query-tests/security/CWE-789/UnboundedAllocation.qlref b/java/ql/test/query-tests/security/CWE-789/UnboundedAllocation.qlref new file mode 100644 index 000000000000..1f1982520380 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-789/UnboundedAllocation.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-789/UnboundedAllocation.ql \ No newline at end of file diff --git a/java/ql/test/query-tests/security/CWE-789/UnboundedAllocationDeserialization.expected b/java/ql/test/query-tests/security/CWE-789/UnboundedAllocationDeserialization.expected new file mode 100644 index 000000000000..9ba3f245339a --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-789/UnboundedAllocationDeserialization.expected @@ -0,0 +1,7 @@ +edges +| Test.java:19:33:19:60 | in : ObjectInputStream | Test.java:23:31:23:36 | length | +nodes +| Test.java:19:33:19:60 | in : ObjectInputStream | semmle.label | in : ObjectInputStream | +| Test.java:23:31:23:36 | length | semmle.label | length | +#select +| Test.java:23:31:23:36 | length | Test.java:19:33:19:60 | in : ObjectInputStream | Test.java:23:31:23:36 | length | Unbounded memory allocation during deserialization of $@. | Test.java:19:33:19:60 | in : ObjectInputStream | this input stream | diff --git a/java/ql/test/query-tests/security/CWE-789/UnboundedAllocationDeserialization.qlref b/java/ql/test/query-tests/security/CWE-789/UnboundedAllocationDeserialization.qlref new file mode 100644 index 000000000000..05803d3fae8f --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-789/UnboundedAllocationDeserialization.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-789/UnboundedAllocationDeserialization.ql \ No newline at end of file