Merge pull request #1 from jketema/amammad-cpp-bombs

Cleanup cpp bombs

cpp/ql/src/experimental/query-tests/Security/CWE/CWE-409/Brotli.qll → cpp/ql/src/experimental/Security/CWE/CWE-409/Brotli.qll

@@ -3,23 +3,24 @@
  */
 
 import cpp
-import semmle.code.cpp.ir.dataflow.TaintTracking
 import DecompressionBomb
 
 /**
- * The `BrotliDecoderDecompress` function is used in flow sink. * Ref: https://www.brotli.org/decode.html#af68
+ * The `BrotliDecoderDecompress` function is used in flow sink.
+ * See https://www.brotli.org/decode.html.
  */
 class BrotliDecoderDecompressFunction extends DecompressionFunction {
-  BrotliDecoderDecompressFunction() { this.hasGlobalName(["BrotliDecoderDecompress"]) }
+  BrotliDecoderDecompressFunction() { this.hasGlobalName("BrotliDecoderDecompress") }
 
   override int getArchiveParameterIndex() { result = 1 }
 }
 
 /**
- * The `BrotliDecoderDecompressStream` function is used in flow sink. * Ref: https://www.brotli.org/decode.html#a234
+ * The `BrotliDecoderDecompressStream` function is used in flow sink.
+ * See https://www.brotli.org/decode.html.
  */
 class BrotliDecoderDecompressStreamFunction extends DecompressionFunction {
-  BrotliDecoderDecompressStreamFunction() { this.hasGlobalName(["BrotliDecoderDecompressStream"]) }
+  BrotliDecoderDecompressStreamFunction() { this.hasGlobalName("BrotliDecoderDecompressStream") }
 
   override int getArchiveParameterIndex() { result = 2 }
 }

cpp/ql/src/experimental/query-tests/Security/CWE/CWE-409/DecompressionBomb.qll → cpp/ql/src/experimental/Security/CWE/CWE-409/DecompressionBomb.qll

@@ -18,6 +18,9 @@ abstract class DecompressionFunction extends Function {
 /**
  * The Decompression Flow Steps, extend this class to define new decompression sinks.
  */
-abstract class DecompressionFlowStep extends Function {
+abstract class DecompressionFlowStep extends string {
+  bindingset[this]
+  DecompressionFlowStep() { any() }
+
   abstract predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2);
 }

cpp/ql/src/experimental/query-tests/Security/CWE/CWE-409/DecompressionBomb.qhelp → cpp/ql/src/experimental/Security/CWE/CWE-409/DecompressionBombs.qhelp

@@ -3,8 +3,8 @@
   "qhelp.dtd">
 <qhelp>
 <overview>
-<p>Extracting Compressed files with any compression algorithm like gzip can cause to denial of service attacks.</p>
-<p>Attackers can compress a huge file which created by repeated similiar byte and convert it to a small compressed file.</p>
+<p>Extracting Compressed files with any compression algorithm like gzip can cause denial of service attacks.</p>
+<p>Attackers can compress a huge file consisting of repeated similiar bytes into a small compressed file.</p>
 </overview>
 <recommendation>
 
@@ -14,12 +14,12 @@
 <example>
 
 <p>
-Reading uncompressed Gzip file within a loop and check for a threshold size in each cycle.
+Reading an uncompressed Gzip file within a loop and check for a threshold size in each cycle.
 </p>
 <sample src="example_good.cpp"/>
 
 <p>
-An Unsafe Approach can be this example which we don't check for uncompressed size.
+The following example is unsafe, as we do not check the uncompressed size.
 </p>
 <sample src="example_bad.cpp" />
 
@@ -28,11 +28,11 @@ An Unsafe Approach can be this example which we don't check for uncompressed siz
 <references>
 
 <li>
-<a href="https://zlib.net/manual.html">Zlib Documentation</a>
+<a href="https://zlib.net/manual.html">Zlib documentation</a>
 </li>
 
 <li>
-<a href="https://www.bamsoftware.com/hacks/zipbomb/">A great research to gain more impact by this kind of attacks</a>
+<a href="https://www.bamsoftware.com/hacks/zipbomb/">An explanation of the attack</a>
 </li>
 
 </references>

cpp/ql/src/experimental/query-tests/Security/CWE/CWE-409/DecompressionBombs.ql → cpp/ql/src/experimental/Security/CWE/CWE-409/DecompressionBombs.ql

@@ -3,39 +3,38 @@
  * @description User-controlled data that flows into decompression library APIs without checking the compression rate is dangerous
  * @kind path-problem
  * @problem.severity error
- * @security-severity 7.8
- * @precision high
- * @id cpp/data-decompression
+ * @precision low
+ * @id cpp/data-decompression-bomb
  * @tags security
  *       experimental
  *       external/cwe/cwe-409
  */
 
 import cpp
-import semmle.code.cpp.ir.dataflow.TaintTracking
 import semmle.code.cpp.security.FlowSources
 import DecompressionBomb
 
+predicate isSink(FunctionCall fc, DataFlow::Node sink) {
+  exists(DecompressionFunction f | fc.getTarget() = f |
+    fc.getArgument(f.getArchiveParameterIndex()) = [sink.asExpr(), sink.asIndirectExpr()]
+  )
+}
+
 module DecompressionTaintConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) { source instanceof FlowSource }
 
-  predicate isSink(DataFlow::Node sink) {
-    exists(FunctionCall fc, DecompressionFunction f | fc.getTarget() = f |
-      fc.getArgument(f.getArchiveParameterIndex()) = [sink.asExpr(), sink.asIndirectExpr()]
-    )
-  }
+  predicate isSink(DataFlow::Node sink) { isSink(_, sink) }
 
   predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
-    any(DecompressionFlowStep f).isAdditionalFlowStep(node1, node2) or
-    nextInAdditionalFlowStep(node1, node2)
+    any(DecompressionFlowStep s).isAdditionalFlowStep(node1, node2)
   }
 }
 
 module DecompressionTaint = TaintTracking::Global<DecompressionTaintConfig>;
 
 import DecompressionTaint::PathGraph
 
-from DecompressionTaint::PathNode source, DecompressionTaint::PathNode sink
-where DecompressionTaint::flowPath(source, sink)
-select sink.getNode(), source, sink, "This Decompression output $@.", source.getNode(),
-  "is not limited"
+from DecompressionTaint::PathNode source, DecompressionTaint::PathNode sink, FunctionCall fc
+where DecompressionTaint::flowPath(source, sink) and isSink(fc, sink.getNode())
+select sink.getNode(), source, sink, "The decompression output of $@ is not limited", fc,
+  fc.getTarget().getName()

cpp/ql/src/experimental/query-tests/Security/CWE/CWE-409/LibArchive.qll → cpp/ql/src/experimental/Security/CWE/CWE-409/LibArchive.qll

@@ -3,12 +3,11 @@
  */
 
 import cpp
-import semmle.code.cpp.ir.dataflow.TaintTracking
 import DecompressionBomb
 
 /**
  * The `archive_read_data*` functions are used in flow sink.
- * [Examples](https://github.com/libarchive/libarchive/wiki/Examples)
+ * See https://github.com/libarchive/libarchive/wiki/Examples.
  */
 class Archive_read_data_block extends DecompressionFunction {
   Archive_read_data_block() {
@@ -21,11 +20,11 @@ class Archive_read_data_block extends DecompressionFunction {
 /**
  * The `archive_read_open_filename` function as a flow step.
  */
-class ReadOpenFunction extends DecompressionFlowStep {
-  ReadOpenFunction() { this.hasGlobalName("archive_read_open_filename") }
+class ReadOpenFunctionStep extends DecompressionFlowStep {
+  ReadOpenFunctionStep() { this = "ReadOpenFunction" }
 
   override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
-    exists(FunctionCall fc | fc.getTarget() = this |
+    exists(FunctionCall fc | fc.getTarget().hasGlobalName("archive_read_open_filename") |
       node1.asIndirectExpr() = fc.getArgument(1) and
       node2.asIndirectExpr() = fc.getArgument(0)
     )

cpp/ql/src/experimental/query-tests/Security/CWE/CWE-409/MiniZip.qll → cpp/ql/src/experimental/Security/CWE/CWE-409/MiniZip.qll

@@ -3,12 +3,11 @@
  */
 
 import cpp
-import semmle.code.cpp.ir.dataflow.TaintTracking
 import DecompressionBomb
 
 /**
  * The `mz_zip_entry` function is used in flow sink.
- * [docuemnt](https://github.com/zlib-ng/minizip-ng/blob/master/doc/mz_zip.md)
+ * See https://github.com/zlib-ng/minizip-ng/blob/master/doc/mz_zip.md.
  */
 class Mz_zip_entry extends DecompressionFunction {
   Mz_zip_entry() { this.hasGlobalName("mz_zip_entry_read") }
@@ -18,7 +17,7 @@ class Mz_zip_entry extends DecompressionFunction {
 
 /**
  * The `mz_zip_reader_entry_*` and `mz_zip_reader_save_all` functions are used in flow sink.
- * [docuemnt](https://github.com/zlib-ng/minizip-ng/blob/master/doc/mz_zip_rw.md)
+ * See https://github.com/zlib-ng/minizip-ng/blob/master/doc/mz_zip_rw.md.
  */
 class Mz_zip_reader_entry extends DecompressionFunction {
   Mz_zip_reader_entry() {
@@ -43,13 +42,13 @@ class UnzOpenFunction extends DecompressionFunction {
 /**
  * The `mz_zip_reader_open_file` and `mz_zip_reader_open_file_in_memory` functions as a flow step.
  */
-class ReaderOpenFunction extends DecompressionFlowStep {
-  ReaderOpenFunction() {
-    this.hasGlobalName(["mz_zip_reader_open_file_in_memory", "mz_zip_reader_open_file"])
-  }
+class ReaderOpenFunctionStep extends DecompressionFlowStep {
+  ReaderOpenFunctionStep() { this = "ReaderOpenFunctionStep" }
 
   override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
-    exists(FunctionCall fc | fc.getTarget() = this |
+    exists(FunctionCall fc |
+      fc.getTarget().hasGlobalName(["mz_zip_reader_open_file_in_memory", "mz_zip_reader_open_file"])
+    |
       node1.asIndirectExpr() = fc.getArgument(1) and
       node2.asIndirectExpr() = fc.getArgument(0)
     )

cpp/ql/src/experimental/query-tests/Security/CWE/CWE-409/ZSTD.qll → cpp/ql/src/experimental/Security/CWE/CWE-409/ZSTD.qll

@@ -3,14 +3,13 @@
  */
 
 import cpp
-import semmle.code.cpp.ir.dataflow.TaintTracking
 import DecompressionBomb
 
 /**
  * The `ZSTD_decompress`  function is used in flow sink.
  */
 class ZstdDecompressFunction extends DecompressionFunction {
-  ZstdDecompressFunction() { this.hasGlobalName(["ZSTD_decompress"]) }
+  ZstdDecompressFunction() { this.hasGlobalName("ZSTD_decompress") }
 
   override int getArchiveParameterIndex() { result = 2 }
 }
@@ -19,7 +18,7 @@ class ZstdDecompressFunction extends DecompressionFunction {
  * The `ZSTD_decompressDCtx` function is used in flow sink.
  */
 class ZstdDecompressDctxFunction extends DecompressionFunction {
-  ZstdDecompressDctxFunction() { this.hasGlobalName(["ZSTD_decompressDCtx"]) }
+  ZstdDecompressDctxFunction() { this.hasGlobalName("ZSTD_decompressDCtx") }
 
   override int getArchiveParameterIndex() { result = 3 }
 }
@@ -28,7 +27,7 @@ class ZstdDecompressDctxFunction extends DecompressionFunction {
  * The `ZSTD_decompressStream` function is used in flow sink.
  */
 class ZstdDecompressStreamFunction extends DecompressionFunction {
-  ZstdDecompressStreamFunction() { this.hasGlobalName(["ZSTD_decompressStream"]) }
+  ZstdDecompressStreamFunction() { this.hasGlobalName("ZSTD_decompressStream") }
 
   override int getArchiveParameterIndex() { result = 2 }
 }
@@ -37,19 +36,19 @@ class ZstdDecompressStreamFunction extends DecompressionFunction {
  * The `ZSTD_decompress_usingDDict` function is used in flow sink.
  */
 class ZstdDecompressUsingDdictFunction extends DecompressionFunction {
-  ZstdDecompressUsingDdictFunction() { this.hasGlobalName(["ZSTD_decompress_usingDDict"]) }
+  ZstdDecompressUsingDdictFunction() { this.hasGlobalName("ZSTD_decompress_usingDDict") }
 
   override int getArchiveParameterIndex() { result = 3 }
 }
 
 /**
  * The `fopen_orDie` function as a flow step.
  */
-class FopenOrDieFunction extends DecompressionFlowStep {
-  FopenOrDieFunction() { this.hasGlobalName("fopen_orDie") }
+class FopenOrDieFunctionStep extends DecompressionFlowStep {
+  FopenOrDieFunctionStep() { this = "FopenOrDieFunctionStep" }
 
   override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
-    exists(FunctionCall fc | fc.getTarget() = this |
+    exists(FunctionCall fc | fc.getTarget().hasGlobalName("fopen_orDie") |
       node1.asIndirectExpr() = fc.getArgument(0) and
       node2.asExpr() = fc
     )
@@ -59,11 +58,11 @@ class FopenOrDieFunction extends DecompressionFlowStep {
 /**
  * The `fread_orDie` function as a flow step.
  */
-class FreadOrDieFunction extends DecompressionFlowStep {
-  FreadOrDieFunction() { this.hasGlobalName("fread_orDie") }
+class FreadOrDieFunctionStep extends DecompressionFlowStep {
+  FreadOrDieFunctionStep() { this = "FreadOrDieFunctionStep" }
 
   override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
-    exists(FunctionCall fc | fc.getTarget() = this |
+    exists(FunctionCall fc | fc.getTarget().hasGlobalName("fread_orDie") |
       node1.asIndirectExpr() = fc.getArgument(2) and
       node2.asIndirectExpr() = fc.getArgument(0)
     )

cpp/ql/src/experimental/query-tests/Security/CWE/CWE-409/ZlibGzopen.qll → cpp/ql/src/experimental/Security/CWE/CWE-409/ZlibGzopen.qll

@@ -3,7 +3,6 @@
  */
 
 import cpp
-import semmle.code.cpp.ir.dataflow.TaintTracking
 import DecompressionBomb
 
 /**
@@ -44,11 +43,11 @@ class GzReadFunction extends DecompressionFunction {
  *
  * `gzdopen(int fd, const char *mode)`
  */
-class GzdopenFunction extends DecompressionFlowStep {
-  GzdopenFunction() { this.hasGlobalName("gzdopen") }
+class GzdopenFunctionStep extends DecompressionFlowStep {
+  GzdopenFunctionStep() { this = "GzdopenFunctionStep" }
 
   override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
-    exists(FunctionCall fc | fc.getTarget() = this |
+    exists(FunctionCall fc | fc.getTarget().hasGlobalName("gzdopen") |
       node1.asExpr() = fc.getArgument(0) and
       node2.asExpr() = fc
     )
@@ -60,11 +59,11 @@ class GzdopenFunction extends DecompressionFlowStep {
  *
  * `gzopen(const char *path, const char *mode)`
  */
-class GzopenFunction extends DecompressionFlowStep {
-  GzopenFunction() { this.hasGlobalName("gzopen") }
+class GzopenFunctionStep extends DecompressionFlowStep {
+  GzopenFunctionStep() { this = "GzopenFunctionStep" }
 
   override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
-    exists(FunctionCall fc | fc.getTarget() = this |
+    exists(FunctionCall fc | fc.getTarget().hasGlobalName("gzopen") |
       node1.asIndirectExpr() = fc.getArgument(0) and
       node2.asExpr() = fc
     )

cpp/ql/src/experimental/Security/CWE/CWE-409/ZlibInflator.qll

@@ -0,0 +1,39 @@
+/**
+ * https://www.zlib.net/
+ */
+
+import cpp
+import DecompressionBomb
+
+/**
+ * The `inflate` and `inflateSync` functions are used in flow sink.
+ *
+ * `inflate(z_stream strm, int flush)`
+ *
+ * `inflateSync(z_stream strm)`
+ */
+class InflateFunction extends DecompressionFunction {
+  InflateFunction() { this.hasGlobalName(["inflate", "inflateSync"]) }
+
+  override int getArchiveParameterIndex() { result = 0 }
+}
+
+/**
+ * The `next_in` member of a `z_stream` variable is used in a flow steps.
+ */
+class NextInMemberStep extends DecompressionFlowStep {
+  NextInMemberStep() { this = "NextInMemberStep" }
+
+  override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+    exists(Variable nextInVar, VariableAccess zStreamAccess |
+      nextInVar.getDeclaringType().hasName("z_stream") and
+      nextInVar.hasName("next_in") and
+      zStreamAccess.getType().hasName("z_stream")
+    |
+      nextInVar.getAnAccess().getQualifier().(VariableAccess).getTarget() =
+        zStreamAccess.getTarget() and
+      node1.asIndirectExpr() = nextInVar.getAnAssignedValue() and
+      node2.asExpr() = zStreamAccess
+    )
+  }
+}

cpp/ql/src/experimental/query-tests/Security/CWE/CWE-409/ZlibUncompress.qll → cpp/ql/src/experimental/Security/CWE/CWE-409/ZlibUncompress.qll

@@ -3,7 +3,6 @@
  */
 
 import cpp
-import semmle.code.cpp.ir.dataflow.TaintTracking
 import DecompressionBomb
 
 /**

cpp/ql/src/experimental/Security/CWE/CWE-409/example_bad.cpp

@@ -0,0 +1,15 @@
+#include "zlib.h"
+
+void UnsafeGzread(gzFile inFileZ) {
+    const int BUFFER_SIZE = 8192;
+    unsigned char unzipBuffer[BUFFER_SIZE];
+    unsigned int unzippedBytes;
+    while (true) {
+        unzippedBytes = gzread(inFileZ, unzipBuffer, BUFFER_SIZE);
+        if (unzippedBytes <= 0) {
+            break;
+        }
+
+        // process buffer
+    }
+}