Merge branch 'main' into mathiasvp/reverse-read-take-3

Author: MathiasVP

cpp/ql/src/Architecture/General Class-Level Information/ClassHierarchies.ql

@@ -4,9 +4,6 @@
  * @kind graph
  * @id cpp/architecture/class-hierarchies
  * @graph.layout organic
- * @workingset jhotdraw
- * @result succeed 48
- * @result_ondemand succeed 48
  * @tags maintainability
  */
 

cpp/ql/src/Architecture/General Class-Level Information/InheritanceDepthDistribution.ql

@@ -4,9 +4,6 @@
  * @kind chart
  * @id cpp/architecture/inheritance-depth-distribution
  * @chart.type line
- * @workingset jhotdraw
- * @result succeed 48
- * @result_ondemand succeed 48
  * @tags maintainability
  */
 

cpp/ql/src/Architecture/General Namespace-Level Information/GlobalNamespaceClasses.ql

@@ -1,7 +1,8 @@
 /**
  * @name Global namespace classes
  * @description Finds classes that belong to no namespace.
- * @kind table
+ * @kind problem
+ * @problem.severity recommendation
  * @id cpp/architecture/global-namespace-classes
  * @tags maintainability
  *       modularity

cpp/ql/src/Architecture/Refactoring Opportunities/ClassesWithManyDependencies.ql

@@ -4,9 +4,6 @@
  * @kind problem
  * @id cpp/architecture/classes-with-many-dependencies
  * @problem.severity recommendation
- * @workingset jhotdraw
- * @result succeed 20
- * @result_ondemand succeed 20
  * @tags maintainability
  *       statistical
  *       non-attributable

cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.cpp

@@ -0,0 +1,19 @@
+
+image::image(int width, int height)
+{
+	int x, y;
+
+	// allocate width * height pixels
+	pixels = new uint32_t[width * height];
+
+	// fill width * height pixels
+	for (y = 0; y < height; y++)
+	{
+		for (x = 0; x < width; x++)
+		{
+			pixels[(y * width) + height] = 0;
+		}
+	}
+
+	// ...
+}

cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.qhelp

@@ -0,0 +1,25 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>The result of a multiplication is used in the size of an allocation. If the multiplication can be made to overflow, a much smaller amount of memory may be allocated than the rest of the code expects. This may lead to overflowing writes when the buffer is accessed later.</p>
+</overview>
+
+<recommendation>
+<p>To fix this issue, ensure that the arithmetic used in the size of an allocation cannot overflow before memory is allocated.</p>
+</recommendation>
+
+<example>
+<p>In the following example, an array of size <code>width * height</code> is allocated and stored as <code>pixels</code>. If <code>width</code> and <code>height</code> are set such that the multiplication overflows and wraps to a small value (say, 4) then the initialization code that follows the allocation will write beyond the end of the array.</p>
+<sample src="AllocMultiplicationOverflow.cpp"/>
+</example>
+
+<references>
+<li>
+  Cplusplus.com: <a href="http://www.cplusplus.com/articles/DE18T05o/">Integer overflow</a>.
+</li>
+</references>
+
+</qhelp>

cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.ql

@@ -0,0 +1,40 @@
+/**
+ * @name Multiplication result may overflow and be used in allocation
+ * @description Using a multiplication result that may overflow in the size of an allocation may lead to buffer overflows when the allocated memory is used.
+ * @kind path-problem
+ * @problem.severity warning
+ * @precision low
+ * @tags security
+ *       correctness
+ *       external/cwe/cwe-190
+ *       external/cwe/cwe-128
+ * @id cpp/multiplication-overflow-in-alloc
+ */
+
+import cpp
+import semmle.code.cpp.models.interfaces.Allocation
+import semmle.code.cpp.dataflow.DataFlow
+import DataFlow::PathGraph
+
+class MultToAllocConfig extends DataFlow::Configuration {
+  MultToAllocConfig() { this = "MultToAllocConfig" }
+
+  override predicate isSource(DataFlow::Node node) {
+    // a multiplication of two non-constant expressions
+    exists(MulExpr me |
+      me = node.asExpr() and
+      forall(Expr e | e = me.getAnOperand() | not exists(e.getValue()))
+    )
+  }
+
+  override predicate isSink(DataFlow::Node node) {
+    // something that affects an allocation size
+    node.asExpr() = any(AllocationExpr ae).getSizeExpr().getAChild*()
+  }
+}
+
+from MultToAllocConfig config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink, source, sink,
+  "Potentially overflowing value from $@ is used in the size of this allocation.", source,
+  "multiplication"

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/AllocMultiplicationOverflow/AllocMultiplicationOverflow.expected

@@ -0,0 +1,17 @@
+edges
+| test.cpp:22:17:22:21 | ... * ... | test.cpp:23:33:23:37 | size1 |
+nodes
+| test.cpp:13:33:13:37 | ... * ... | semmle.label | ... * ... |
+| test.cpp:15:31:15:35 | ... * ... | semmle.label | ... * ... |
+| test.cpp:19:34:19:38 | ... * ... | semmle.label | ... * ... |
+| test.cpp:22:17:22:21 | ... * ... | semmle.label | ... * ... |
+| test.cpp:23:33:23:37 | size1 | semmle.label | size1 |
+| test.cpp:30:27:30:31 | ... * ... | semmle.label | ... * ... |
+| test.cpp:31:27:31:31 | ... * ... | semmle.label | ... * ... |
+#select
+| test.cpp:13:33:13:37 | ... * ... | test.cpp:13:33:13:37 | ... * ... | test.cpp:13:33:13:37 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:13:33:13:37 | ... * ... | multiplication |
+| test.cpp:15:31:15:35 | ... * ... | test.cpp:15:31:15:35 | ... * ... | test.cpp:15:31:15:35 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:15:31:15:35 | ... * ... | multiplication |
+| test.cpp:19:34:19:38 | ... * ... | test.cpp:19:34:19:38 | ... * ... | test.cpp:19:34:19:38 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:19:34:19:38 | ... * ... | multiplication |
+| test.cpp:23:33:23:37 | size1 | test.cpp:22:17:22:21 | ... * ... | test.cpp:23:33:23:37 | size1 | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:22:17:22:21 | ... * ... | multiplication |
+| test.cpp:30:27:30:31 | ... * ... | test.cpp:30:27:30:31 | ... * ... | test.cpp:30:27:30:31 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:30:27:30:31 | ... * ... | multiplication |
+| test.cpp:31:27:31:31 | ... * ... | test.cpp:31:27:31:31 | ... * ... | test.cpp:31:27:31:31 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:31:27:31:31 | ... * ... | multiplication |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/AllocMultiplicationOverflow/AllocMultiplicationOverflow.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.ql

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/AllocMultiplicationOverflow/test.cpp

@@ -0,0 +1,32 @@
+
+typedef unsigned long size_t;
+void *malloc(size_t size);
+
+int getAnInt();
+
+void test()
+{
+	int x = getAnInt();
+	int y = getAnInt();
+
+	char *buffer1 = (char *)malloc(x + y); // GOOD
+	char *buffer2 = (char *)malloc(x * y); // BAD
+	int *buffer3 = (int *)malloc(x * sizeof(int)); // GOOD
+	int *buffer4 = (int *)malloc(x * y * sizeof(int)); // BAD
+
+	if ((x <= 1000) && (y <= 1000))
+	{
+		char *buffer5 = (char *)malloc(x * y); // GOOD [FALSE POSITIVE]
+	}
+
+	size_t size1 = x * y;
+	char *buffer5 = (char *)malloc(size1); // BAD
+
+	size_t size2 = x;
+	size2 *= y;
+	char *buffer6 = (char *)malloc(size2); // BAD [NOT DETECTED]
+
+	char *buffer7 = new char[x * 10]; // GOOD
+	char *buffer8 = new char[x * y]; // BAD
+	char *buffer9 = new char[x * x]; // BAD
+}