From 090280749253bdac0384ac9958c2b8927095930a Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Tue, 27 Oct 2020 15:04:59 +0000 Subject: [PATCH 1/9] Java: Unbounded Allocation queries --- .../CWE/CWE-789/UnboundedAllocation.qhelp | 35 +++++++++ .../CWE/CWE-789/UnboundedAllocation.ql | 28 +++++++ .../CWE/CWE-789/UnboundedAllocationBad.java | 10 +++ .../CWE/CWE-789/UnboundedAllocationCommon.qll | 74 +++++++++++++++++++ .../UnboundedAllocationDeserialization.ql | 41 ++++++++++ ...UnboundedAllocationDeserializationBad.java | 22 ++++++ .../CWE/CWE-789/UnboundedAllocationGood.java | 15 ++++ ...UnbounedAllocationDeserializationGood.java | 24 ++++++ .../UndoundedAllocationDeserialization.qhelp | 42 +++++++++++ 9 files changed, 291 insertions(+) create mode 100644 java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.qhelp create mode 100644 java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.ql create mode 100644 java/ql/src/Security/CWE/CWE-789/UnboundedAllocationBad.java create mode 100644 java/ql/src/Security/CWE/CWE-789/UnboundedAllocationCommon.qll create mode 100644 java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserialization.ql create mode 100644 java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserializationBad.java create mode 100644 java/ql/src/Security/CWE/CWE-789/UnboundedAllocationGood.java create mode 100644 java/ql/src/Security/CWE/CWE-789/UnbounedAllocationDeserializationGood.java create mode 100644 java/ql/src/Security/CWE/CWE-789/UndoundedAllocationDeserialization.qhelp 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..05ec517b6d67 --- /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 ArrayList could be used to ensure that the application only allocates as much memory as is necassary.

+ + + + +

+The following example allocates an amount of memory based on a user provided value: +

+ + + +

The following example firt validates the uner 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..9f6ee8b9d9d6 --- /dev/null +++ b/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 UnboundedDeserializationConfig extends TaintTracking::Configuration { + UnboundedDeserializationConfig() { this = "UnboundedDeserializationConfig" } + + 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, UnboundedDeserializationConfig config +where config.hasFlowPath(source, sink) +select sink, source, sink, "Unbounded memory allocation" 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..c661a608d51d --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationBad.java @@ -0,0 +1,10 @@ +public void processData(Socket sock) { + InputStream iStream = sock.getInputStream(); + ObjectInpuStream oiStream = new ObjectInputStream(iStream); + + int size = oiStream.readInt(); + // BAD: A user-controlled amount of memory is alocated + byte[] buffer = new byte[size]; + + oiStream.readFully(buffer); +} \ No newline at end of file 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..8d42899b6d54 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationCommon.qll @@ -0,0 +1,74 @@ +import semmle.code.java.dataflow.RangeAnalysis +import semmle.code.java.dataflow.FlowSteps +import semmle.code.java.dataflow.DataFlow +import semmle.code.java.dataflow.TaintTracking + +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 + ) + } +} + +abstract class AllocatingCallable extends Callable { + abstract int getParam(); +} + +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 } +} + +class ListConstructor extends AllocatingCallable, Constructor { + ListConstructor() { + this.getDeclaringType().hasQualifiedName("java.util", ["ArrayList", "Vector"]) and + this.getParameterType(0) instanceof IntegralType + } + + override int getParam() { result = 0 } +} + +class ReadMethod extends TaintPreservingCallable { + ReadMethod() { + this.getDeclaringType().hasQualifiedName("java.io", "ObjectInputStream") and + this.getName().matches("read%") + } + + override predicate returnsTaintFrom(int arg) { arg = -1 } +} + +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 + ) + ) + } +} + +predicate hasUpperBound(Expr e) { bounded(e, any(ZeroBound z), _, true, _) } 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..5b6918f011dc --- /dev/null +++ b/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" 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..5fec524dcf8f --- /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..480448245dc4 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationGood.java @@ -0,0 +1,15 @@ +public void processData(Socket sock) { + InputStream iStream = sock.getInputStream(); + ObjectInpuStream oiStream = new ObjectInputStream(iStream); + + int size = oiStream.readInt(); + + if (size > 4096) { + throw new IllegalArgumentException("Size too large"); + } + + // GOOD: There is an upper bound on memory consumption. + byte[] buffer = new byte[size]; + + oiStream.readFully(buffer); +} \ No newline at end of file diff --git a/java/ql/src/Security/CWE/CWE-789/UnbounedAllocationDeserializationGood.java b/java/ql/src/Security/CWE/CWE-789/UnbounedAllocationDeserializationGood.java new file mode 100644 index 000000000000..7da36000365e --- /dev/null +++ b/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.writeLong(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(); + } +} \ No newline at end of file diff --git a/java/ql/src/Security/CWE/CWE-789/UndoundedAllocationDeserialization.qhelp b/java/ql/src/Security/CWE/CWE-789/UndoundedAllocationDeserialization.qhelp new file mode 100644 index 000000000000..471a15cf53e5 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-789/UndoundedAllocationDeserialization.qhelp @@ -0,0 +1,42 @@ + + + +

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

+ + + + +

+ 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 constanat factor: +

+ + + + + + +
  • Example in the Guava framework: CVE-2018-10237, patched by this commit.
    • +     +     \ No newline at end of file From 9495c5e800e68b23bf5f90633dc1efadc013b833 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Fri, 30 Oct 2020 12:15:39 +0000 Subject: [PATCH 2/9] Java: Add tests --- .../CWE/CWE-789/UnboundedAllocationBad.java | 7 +- ...UnboundedAllocationDeserializationBad.java | 2 +- .../CWE/CWE-789/UnboundedAllocationGood.java | 9 +-- ...UnbounedAllocationDeserializationGood.java | 2 +- .../UndoundedAllocationDeserialization.qhelp | 2 +- .../query-tests/security/CWE-789/Test.java | 81 +++++++++++++++++++ .../CWE-789/UnboundedAllocation.expected | 7 ++ .../CWE-789/UnboundedAllocation.qlref | 1 + ...nboundedAllocationDeserialization.expected | 7 ++ .../UnboundedAllocationDeserialization.qlref | 1 + 10 files changed, 107 insertions(+), 12 deletions(-) create mode 100644 java/ql/test/query-tests/security/CWE-789/Test.java create mode 100644 java/ql/test/query-tests/security/CWE-789/UnboundedAllocation.expected create mode 100644 java/ql/test/query-tests/security/CWE-789/UnboundedAllocation.qlref create mode 100644 java/ql/test/query-tests/security/CWE-789/UnboundedAllocationDeserialization.expected create mode 100644 java/ql/test/query-tests/security/CWE-789/UnboundedAllocationDeserialization.qlref diff --git a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationBad.java b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationBad.java index c661a608d51d..43649cc8aea2 100644 --- a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationBad.java +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationBad.java @@ -1,10 +1,9 @@ public void processData(Socket sock) { - InputStream iStream = sock.getInputStream(); - ObjectInpuStream oiStream = new ObjectInputStream(iStream); + ObjectInputStream stream = new ObjectInputStream(sock.getInputStream()); - int size = oiStream.readInt(); + int size = stream.readInt(); // BAD: A user-controlled amount of memory is alocated byte[] buffer = new byte[size]; - oiStream.readFully(buffer); + // ... } \ No newline at end of file diff --git a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserializationBad.java b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserializationBad.java index 5fec524dcf8f..b665dfc510c4 100644 --- a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserializationBad.java +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserializationBad.java @@ -16,7 +16,7 @@ private void readObject(java.io.ObjectInputStream in) throws IOException, ClassN data = new Object[length]; for (int i = 0; i < length; i++) { - data[i] = in.readObject() + 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 index 480448245dc4..1400fca3a003 100644 --- a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationGood.java +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationGood.java @@ -1,15 +1,14 @@ public void processData(Socket sock) { - InputStream iStream = sock.getInputStream(); - ObjectInpuStream oiStream = new ObjectInputStream(iStream); + ObjectInputStream stream = new ObjectInputStream(sock.getInputStream()); - int size = oiStream.readInt(); + int size = stream.readInt(); if (size > 4096) { - throw new IllegalArgumentException("Size too large"); + throw new IOException("Size too large"); } // GOOD: There is an upper bound on memory consumption. byte[] buffer = new byte[size]; - oiStream.readFully(buffer); + // ... } \ No newline at end of file diff --git a/java/ql/src/Security/CWE/CWE-789/UnbounedAllocationDeserializationGood.java b/java/ql/src/Security/CWE/CWE-789/UnbounedAllocationDeserializationGood.java index 7da36000365e..0cab2113f1ea 100644 --- a/java/ql/src/Security/CWE/CWE-789/UnbounedAllocationDeserializationGood.java +++ b/java/ql/src/Security/CWE/CWE-789/UnbounedAllocationDeserializationGood.java @@ -5,7 +5,7 @@ private void writeObject(java.io.ObjectOutputStream out) throws IOException { out.writeInt(data.length); for (int i = 0; i < data.length; i++) { - out.writeLong(data[i]); + out.writeObject(data[i]); } } diff --git a/java/ql/src/Security/CWE/CWE-789/UndoundedAllocationDeserialization.qhelp b/java/ql/src/Security/CWE/CWE-789/UndoundedAllocationDeserialization.qhelp index 471a15cf53e5..faf423c000f4 100644 --- a/java/ql/src/Security/CWE/CWE-789/UndoundedAllocationDeserialization.qhelp +++ b/java/ql/src/Security/CWE/CWE-789/UndoundedAllocationDeserialization.qhelp @@ -29,7 +29,7 @@ The following example allocates an array of a size provided by the input stream,

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

    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..571709ddaec2 --- /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 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]; + + // ... + } +} \ No newline at end of file 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..94d4d59044ac --- /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 | 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..23d07255e890 --- /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 from deserialization | 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 From e6260b3447d6afa8cb352e47e4528d47cb3eae49 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Fri, 30 Oct 2020 12:41:06 +0000 Subject: [PATCH 3/9] Java: Add qldoc; fix naming --- .../Security/CWE/CWE-789/UnboundedAllocation.ql | 6 +++--- .../CWE/CWE-789/UnboundedAllocationCommon.qll | 16 ++++++++++++---- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.ql b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.ql index 9f6ee8b9d9d6..0d4e8ff9206c 100644 --- a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.ql +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.ql @@ -13,8 +13,8 @@ import UnboundedAllocationCommon import semmle.code.java.dataflow.FlowSources import DataFlow::PathGraph -class UnboundedDeserializationConfig extends TaintTracking::Configuration { - UnboundedDeserializationConfig() { this = "UnboundedDeserializationConfig" } +class UnboundedAllocationConfig extends TaintTracking::Configuration { + UnboundedAllocationConfig() { this = "UnboundedAllocationConfig" } override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource } @@ -23,6 +23,6 @@ class UnboundedDeserializationConfig extends TaintTracking::Configuration { override predicate isSanitizer(DataFlow::Node node) { hasUpperBound(node.asExpr()) } } -from DataFlow::PathNode source, DataFlow::PathNode sink, UnboundedDeserializationConfig config +from DataFlow::PathNode source, DataFlow::PathNode sink, UnboundedAllocationConfig config where config.hasFlowPath(source, sink) select sink, source, sink, "Unbounded memory allocation" diff --git a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationCommon.qll b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationCommon.qll index 8d42899b6d54..bd1d7c3ff556 100644 --- a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationCommon.qll +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationCommon.qll @@ -1,8 +1,13 @@ +/** + * 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() @@ -14,11 +19,13 @@ class AllocationSink extends DataFlow::Node { } } +/** A callable that allocates memory. */ abstract class AllocatingCallable extends Callable { + /** Returns the parameter index controlling the size of the allocated memory. */ abstract int getParam(); } -class AtomicArrayConstructor extends AllocatingCallable, Constructor { +private class AtomicArrayConstructor extends AllocatingCallable, Constructor { AtomicArrayConstructor() { this .getDeclaringType() @@ -30,7 +37,7 @@ class AtomicArrayConstructor extends AllocatingCallable, Constructor { override int getParam() { result = 0 } } -class ListConstructor extends AllocatingCallable, Constructor { +private class ListConstructor extends AllocatingCallable, Constructor { ListConstructor() { this.getDeclaringType().hasQualifiedName("java.util", ["ArrayList", "Vector"]) and this.getParameterType(0) instanceof IntegralType @@ -39,7 +46,7 @@ class ListConstructor extends AllocatingCallable, Constructor { override int getParam() { result = 0 } } -class ReadMethod extends TaintPreservingCallable { +private class ReadMethod extends TaintPreservingCallable { ReadMethod() { this.getDeclaringType().hasQualifiedName("java.io", "ObjectInputStream") and this.getName().matches("read%") @@ -48,7 +55,7 @@ class ReadMethod extends TaintPreservingCallable { override predicate returnsTaintFrom(int arg) { arg = -1 } } -class ArithmeticStep extends TaintTracking::AdditionalTaintStep { +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 @@ -71,4 +78,5 @@ class ArithmeticStep extends TaintTracking::AdditionalTaintStep { } } +/** Holds if `e` has a known upper bound. */ predicate hasUpperBound(Expr e) { bounded(e, any(ZeroBound z), _, true, _) } From 94136a7b43133496f2b9deaafcf70d72649a8a15 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Mon, 2 Nov 2020 12:45:59 +0000 Subject: [PATCH 4/9] Java: Documentation corrections Co-authored-by: Marcono1234 Co-authored-by: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com> --- java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.qhelp | 4 ++-- .../ql/src/Security/CWE/CWE-789/UnboundedAllocationBad.java | 4 ++-- .../src/Security/CWE/CWE-789/UnboundedAllocationCommon.qll | 2 +- .../CWE/CWE-789/UnbounedAllocationDeserializationGood.java | 4 ++-- .../CWE/CWE-789/UndoundedAllocationDeserialization.qhelp | 4 ++-- java/ql/test/query-tests/security/CWE-789/Test.java | 6 +++--- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.qhelp b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.qhelp index 05ec517b6d67..df3d985b5304 100644 --- a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.qhelp +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.qhelp @@ -13,7 +13,7 @@

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

    +Alternatively, a structure such as ArrayList could be used to ensure that the application only allocates as much memory as is necessary.

     @@ -24,7 +24,7 @@ The following example allocates an amount of memory based on a user provided val -

    The following example firt validates the uner input against an upper bound:

    +

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

    diff --git a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationBad.java b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationBad.java index 43649cc8aea2..1d990c1237d3 100644 --- a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationBad.java +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationBad.java @@ -2,8 +2,8 @@ public void processData(Socket sock) { ObjectInputStream stream = new ObjectInputStream(sock.getInputStream()); int size = stream.readInt(); - // BAD: A user-controlled amount of memory is alocated + // BAD: A user-controlled amount of memory is allocated byte[] buffer = new byte[size]; // ... -} \ No newline at end of file +} diff --git a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationCommon.qll b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationCommon.qll index bd1d7c3ff556..6e67ef0b3182 100644 --- a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationCommon.qll +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationCommon.qll @@ -30,7 +30,7 @@ private class AtomicArrayConstructor extends AllocatingCallable, Constructor { this .getDeclaringType() .hasQualifiedName("java.util.concurrent.atomic", - ["AtomicIntArray", "AtomicLongArray", "AtomicReferenceArray"]) and + ["AtomicIntegerArray", "AtomicLongArray", "AtomicReferenceArray"]) and this.getParameterType(0) instanceof IntegralType } diff --git a/java/ql/src/Security/CWE/CWE-789/UnbounedAllocationDeserializationGood.java b/java/ql/src/Security/CWE/CWE-789/UnbounedAllocationDeserializationGood.java index 0cab2113f1ea..4bbc732b0c85 100644 --- a/java/ql/src/Security/CWE/CWE-789/UnbounedAllocationDeserializationGood.java +++ b/java/ql/src/Security/CWE/CWE-789/UnbounedAllocationDeserializationGood.java @@ -13,7 +13,7 @@ private void readObject(java.io.ObjectInputStream in) throws IOException, ClassN int length = in.readInt(); // GOOD: An ArrayList is used, which dynamically allocates memory as needed. - ArrayList dataList = new ArrayList(); + ArrayList dataList = new ArrayList<>(); for (int i = 0; i < length; i++) { dataList.add(in.readObject()); @@ -21,4 +21,4 @@ private void readObject(java.io.ObjectInputStream in) throws IOException, ClassN data = dataList.toArray(); } -} \ No newline at end of file +} diff --git a/java/ql/src/Security/CWE/CWE-789/UndoundedAllocationDeserialization.qhelp b/java/ql/src/Security/CWE/CWE-789/UndoundedAllocationDeserialization.qhelp index faf423c000f4..c47b8db5ba19 100644 --- a/java/ql/src/Security/CWE/CWE-789/UndoundedAllocationDeserialization.qhelp +++ b/java/ql/src/Security/CWE/CWE-789/UndoundedAllocationDeserialization.qhelp @@ -6,7 +6,7 @@

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

    @@ -39,4 +39,4 @@ The following example uses an ArrayList to only allocate as much me
  • Example in the Guava framework: CVE-2018-10237, patched by this commit.
    • - \ 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 index 571709ddaec2..2d059cc34a7c 100644 --- a/java/ql/test/query-tests/security/CWE-789/Test.java +++ b/java/ql/test/query-tests/security/CWE-789/Test.java @@ -43,7 +43,7 @@ private void readObject(java.io.ObjectInputStream in) throws IOException, ClassN int length = in.readInt(); // GOOD: An ArrayList is used, which dynamically allocates memory as needed. - ArrayList dataList = new ArrayList(); + ArrayList dataList = new ArrayList<>(); for (int i = 0; i < length; i++) { dataList.add(in.readObject()); @@ -58,7 +58,7 @@ 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 + // BAD: A user-controlled amount of memory is allocated byte[] buffer = new byte[size]; // ... @@ -78,4 +78,4 @@ public void processData2(Socket sock) throws IOException { // ... } -} \ No newline at end of file +} From 1e7593e432dcaf42f4ad257c3fc4e8d65401ba82 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Mon, 2 Nov 2020 13:01:44 +0000 Subject: [PATCH 5/9] Java: Remove ReadMethod; modify ArithmeticStep --- .../CWE/CWE-789/UnboundedAllocationCommon.qll | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationCommon.qll b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationCommon.qll index 6e67ef0b3182..56ee21ef753f 100644 --- a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationCommon.qll +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationCommon.qll @@ -3,7 +3,6 @@ */ import semmle.code.java.dataflow.RangeAnalysis -import semmle.code.java.dataflow.FlowSteps import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.TaintTracking @@ -46,15 +45,6 @@ private class ListConstructor extends AllocatingCallable, Constructor { 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() | @@ -64,12 +54,12 @@ private class ArithmeticStep extends TaintTracking::AdditionalTaintStep { or binex instanceof SubExpr or + binex instanceof LShiftExpr + or src.asExpr() = binex.getLeftOperand() and ( binex instanceof DivExpr or - binex instanceof LShiftExpr - or binex instanceof RShiftExpr or binex instanceof URShiftExpr From 751c6c641ce1a89bbe1df1e1fe3d1e02f3f570df Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Mon, 2 Nov 2020 16:20:47 +0000 Subject: [PATCH 6/9] Java: Documentation clean up Co-authored-by: Felicity Chapman --- java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.qhelp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.qhelp b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.qhelp index df3d985b5304..18bae01552fe 100644 --- a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.qhelp +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.qhelp @@ -13,13 +13,13 @@

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

    +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 based on a user provided value: +The following example allocates an amount of memory directly from a user-provided value:

    From 9096e20554755bd6365ae17476fa49242b2de2b0 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Mon, 2 Nov 2020 16:27:10 +0000 Subject: [PATCH 7/9] Java: Add query descriptions and alert placeholders. --- java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.ql | 3 ++- .../CWE/CWE-789/UnboundedAllocationDeserialization.ql | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.ql b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.ql index 0d4e8ff9206c..2af2ed148851 100644 --- a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.ql +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocation.ql @@ -1,5 +1,6 @@ /** * @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 @@ -25,4 +26,4 @@ class UnboundedAllocationConfig extends TaintTracking::Configuration { from DataFlow::PathNode source, DataFlow::PathNode sink, UnboundedAllocationConfig config where config.hasFlowPath(source, sink) -select sink, source, sink, "Unbounded memory allocation" +select sink, source, sink, "Unbounded memory allocation from $@.", source, "this user input" diff --git a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserialization.ql b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserialization.ql index 5b6918f011dc..de06880d820a 100644 --- a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserialization.ql +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserialization.ql @@ -1,5 +1,6 @@ /** * @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 @@ -38,4 +39,5 @@ class UnboundedDeserializationConfig extends TaintTracking::Configuration { from DataFlow::PathNode source, DataFlow::PathNode sink, UnboundedDeserializationConfig config where config.hasFlowPath(source, sink) -select sink, source, sink, "Unbounded memory allocation from deserialization" +select sink, source, sink, "Unbounded memory allocation during deserialization of $@.", source, + "this input stream" From 62cd902f6eaf48782a2082e5a6a75eac4ad6ab56 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Tue, 3 Nov 2020 10:31:13 +0000 Subject: [PATCH 8/9] Java: Fix docs and test output --- ...alization.qhelp => UnboundedAllocationDeserialization.qhelp} | 0 .../query-tests/security/CWE-789/UnboundedAllocation.expected | 2 +- .../CWE-789/UnboundedAllocationDeserialization.expected | 2 +- 3 files changed, 2 insertions(+), 2 deletions(-) rename java/ql/src/Security/CWE/CWE-789/{UndoundedAllocationDeserialization.qhelp => UnboundedAllocationDeserialization.qhelp} (100%) diff --git a/java/ql/src/Security/CWE/CWE-789/UndoundedAllocationDeserialization.qhelp b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserialization.qhelp similarity index 100% rename from java/ql/src/Security/CWE/CWE-789/UndoundedAllocationDeserialization.qhelp rename to java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserialization.qhelp diff --git a/java/ql/test/query-tests/security/CWE-789/UnboundedAllocation.expected b/java/ql/test/query-tests/security/CWE-789/UnboundedAllocation.expected index 94d4d59044ac..920a0b7a03d6 100644 --- a/java/ql/test/query-tests/security/CWE-789/UnboundedAllocation.expected +++ b/java/ql/test/query-tests/security/CWE-789/UnboundedAllocation.expected @@ -4,4 +4,4 @@ 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 | +| 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/UnboundedAllocationDeserialization.expected b/java/ql/test/query-tests/security/CWE-789/UnboundedAllocationDeserialization.expected index 23d07255e890..9ba3f245339a 100644 --- a/java/ql/test/query-tests/security/CWE-789/UnboundedAllocationDeserialization.expected +++ b/java/ql/test/query-tests/security/CWE-789/UnboundedAllocationDeserialization.expected @@ -4,4 +4,4 @@ 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 from deserialization | +| 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 | From 79d4b014a6e90f23b2c693795d1e070d24d4fcd6 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Tue, 3 Nov 2020 11:23:46 +0000 Subject: [PATCH 9/9] Java: Fix qhelp --- ...tionGood.java => UnboudnedAllocationDeserializationGood.java} | 0 .../CWE/CWE-789/UnboundedAllocationDeserialization.qhelp | 1 - 2 files changed, 1 deletion(-) rename java/ql/src/Security/CWE/CWE-789/{UnbounedAllocationDeserializationGood.java => UnboudnedAllocationDeserializationGood.java} (100%) diff --git a/java/ql/src/Security/CWE/CWE-789/UnbounedAllocationDeserializationGood.java b/java/ql/src/Security/CWE/CWE-789/UnboudnedAllocationDeserializationGood.java similarity index 100% rename from java/ql/src/Security/CWE/CWE-789/UnbounedAllocationDeserializationGood.java rename to java/ql/src/Security/CWE/CWE-789/UnboudnedAllocationDeserializationGood.java diff --git a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserialization.qhelp b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserialization.qhelp index c47b8db5ba19..c19abe97af9f 100644 --- a/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserialization.qhelp +++ b/java/ql/src/Security/CWE/CWE-789/UnboundedAllocationDeserialization.qhelp @@ -33,7 +33,6 @@ The following example uses an ArrayList to only allocate as much me

    -