C++: Use a 'TaintTracking::Configuration' for 'cpp/unclear-array-index-validation'.

cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql

@@ -3,7 +3,7 @@
  * @description Accessing an array without first checking
  *              that the index is within the bounds of the array can
  *              cause undefined behavior and can also be a security risk.
- * @kind problem
+ * @kind path-problem
  * @id cpp/unclear-array-index-validation
  * @problem.severity warning
  * @security-severity 8.8
@@ -12,9 +12,11 @@
  */
 
 import cpp
-import semmle.code.cpp.controlflow.Guards
-private import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
-import semmle.code.cpp.security.TaintTracking
+import semmle.code.cpp.controlflow.IRGuards
+import semmle.code.cpp.security.FlowSources
+import semmle.code.cpp.ir.dataflow.TaintTracking
+import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
+import DataFlow::PathGraph
 
 predicate hasUpperBound(VariableAccess offsetExpr) {
   exists(BasicBlock controlled, StackVariable offsetVar, SsaDefinition def |
@@ -32,11 +34,60 @@ predicate linearBoundControls(BasicBlock controlled, SsaDefinition def, StackVar
   )
 }
 
-from Expr origin, ArrayExpr arrayExpr, VariableAccess offsetExpr
-where
-  tainted(origin, offsetExpr) and
-  offsetExpr = arrayExpr.getArrayOffset() and
+predicate isUnboundedArrayIndex(DataFlow::Node sink, VariableAccess offsetExpr) {
+  offsetExpr = sink.asExpr().(ArrayExpr).getArrayOffset() and
   not hasUpperBound(offsetExpr)
-select offsetExpr,
+}
+
+predicate readsVariable(LoadInstruction load, Variable var) {
+  load.getSourceAddress().(VariableAddressInstruction).getASTVariable() = var
+}
+
+predicate hasUpperBoundsCheck(Variable var) {
+  exists(RelationalOperation oper, VariableAccess access |
+    oper.getAnOperand() = access and
+    access.getTarget() = var and
+    // Comparing to 0 is not an upper bound check
+    not oper.getAnOperand().getValue() = "0"
+  )
+}
+
+predicate nodeIsBarrierEqualityCandidate(DataFlow::Node node, Operand access, Variable checkedVar) {
+  readsVariable(node.asInstruction(), checkedVar) and
+  any(IRGuardCondition guard).ensuresEq(access, _, _, node.asInstruction().getBlock(), true)
+}
+
+predicate isFlowSource(FlowSource source, string sourceType) { sourceType = source.getSourceType() }
+
+class ImproperArrayIndexValidationConfig extends TaintTracking::Configuration {
+  ImproperArrayIndexValidationConfig() { this = "ImproperArrayIndexValidationConfig" }
+
+  override predicate isSource(DataFlow::Node source) { isFlowSource(source, _) }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    hasUpperBound(node.asExpr())
+    or
+    exists(Variable checkedVar |
+      readsVariable(node.asInstruction(), checkedVar) and
+      hasUpperBoundsCheck(checkedVar)
+    )
+    or
+    exists(Variable checkedVar, Operand access |
+      readsVariable(access.getDef(), checkedVar) and
+      nodeIsBarrierEqualityCandidate(node, access, checkedVar)
+    )
+  }
+
+  override predicate isSink(DataFlow::Node sink) { isUnboundedArrayIndex(sink, _) }
+}
+
+from
+  VariableAccess offsetExpr, ImproperArrayIndexValidationConfig conf, DataFlow::PathNode source,
+  DataFlow::PathNode sink, string sourceType
+where
+  conf.hasFlowPath(source, sink) and
+  isFlowSource(source.getNode(), sourceType) and
+  isUnboundedArrayIndex(sink.getNode(), offsetExpr)
+select sink.getNode(), source, sink,
   "$@ flows to here and is used in an array indexing expression, potentially causing an invalid access.",
-  origin, "User-provided value"
+  source.getNode(), sourceType

cpp/ql/test/query-tests/Security/CWE/CWE-129/SAMATE/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected

@@ -1 +1,8 @@
-| CWE122_Heap_Based_Buffer_Overflow__c_CWE129_fgets_01.c:52:20:52:23 | data | $@ flows to here and is used in an array indexing expression, potentially causing an invalid access. | CWE122_Heap_Based_Buffer_Overflow__c_CWE129_fgets_01.c:30:19:30:29 | inputBuffer | User-provided value |
+edges
+| CWE122_Heap_Based_Buffer_Overflow__c_CWE129_fgets_01.c:30:19:30:29 | fgets output argument | CWE122_Heap_Based_Buffer_Overflow__c_CWE129_fgets_01.c:52:13:52:24 | access to array |
+nodes
+| CWE122_Heap_Based_Buffer_Overflow__c_CWE129_fgets_01.c:30:19:30:29 | fgets output argument | semmle.label | fgets output argument |
+| CWE122_Heap_Based_Buffer_Overflow__c_CWE129_fgets_01.c:52:13:52:24 | access to array | semmle.label | access to array |
+subpaths
+#select
+| CWE122_Heap_Based_Buffer_Overflow__c_CWE129_fgets_01.c:52:13:52:24 | access to array | CWE122_Heap_Based_Buffer_Overflow__c_CWE129_fgets_01.c:30:19:30:29 | fgets output argument | CWE122_Heap_Based_Buffer_Overflow__c_CWE129_fgets_01.c:52:13:52:24 | access to array | $@ flows to here and is used in an array indexing expression, potentially causing an invalid access. | CWE122_Heap_Based_Buffer_Overflow__c_CWE129_fgets_01.c:30:19:30:29 | fgets output argument | String read by fgets |

cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected

@@ -1,3 +1,26 @@
-| test1.c:18:16:18:16 | i | $@ flows to here and is used in an array indexing expression, potentially causing an invalid access. | test1.c:8:16:8:19 | argv | User-provided value |
-| test1.c:33:11:33:11 | i | $@ flows to here and is used in an array indexing expression, potentially causing an invalid access. | test1.c:8:16:8:19 | argv | User-provided value |
-| test1.c:53:15:53:15 | j | $@ flows to here and is used in an array indexing expression, potentially causing an invalid access. | test1.c:8:16:8:19 | argv | User-provided value |
+edges
+| test1.c:8:16:8:19 | argv | test1.c:9:9:9:9 | i |
+| test1.c:8:16:8:19 | argv | test1.c:11:9:11:9 | i |
+| test1.c:8:16:8:19 | argv | test1.c:13:9:13:9 | i |
+| test1.c:9:9:9:9 | i | test1.c:16:16:16:16 | i |
+| test1.c:11:9:11:9 | i | test1.c:32:16:32:16 | i |
+| test1.c:13:9:13:9 | i | test1.c:48:16:48:16 | i |
+| test1.c:16:16:16:16 | i | test1.c:18:12:18:17 | access to array |
+| test1.c:32:16:32:16 | i | test1.c:33:3:33:12 | access to array |
+| test1.c:48:16:48:16 | i | test1.c:53:7:53:16 | access to array |
+nodes
+| test1.c:8:16:8:19 | argv | semmle.label | argv |
+| test1.c:9:9:9:9 | i | semmle.label | i |
+| test1.c:11:9:11:9 | i | semmle.label | i |
+| test1.c:13:9:13:9 | i | semmle.label | i |
+| test1.c:16:16:16:16 | i | semmle.label | i |
+| test1.c:18:12:18:17 | access to array | semmle.label | access to array |
+| test1.c:32:16:32:16 | i | semmle.label | i |
+| test1.c:33:3:33:12 | access to array | semmle.label | access to array |
+| test1.c:48:16:48:16 | i | semmle.label | i |
+| test1.c:53:7:53:16 | access to array | semmle.label | access to array |
+subpaths
+#select
+| test1.c:18:12:18:17 | access to array | test1.c:8:16:8:19 | argv | test1.c:18:12:18:17 | access to array | $@ flows to here and is used in an array indexing expression, potentially causing an invalid access. | test1.c:8:16:8:19 | argv | a command-line argument |
+| test1.c:33:3:33:12 | access to array | test1.c:8:16:8:19 | argv | test1.c:33:3:33:12 | access to array | $@ flows to here and is used in an array indexing expression, potentially causing an invalid access. | test1.c:8:16:8:19 | argv | a command-line argument |
+| test1.c:53:7:53:16 | access to array | test1.c:8:16:8:19 | argv | test1.c:53:7:53:16 | access to array | $@ flows to here and is used in an array indexing expression, potentially causing an invalid access. | test1.c:8:16:8:19 | argv | a command-line argument |