Skip to content

Commit 2328898

Browse files
MathiasVPMathiasVP
committed
C++: Use a 'TaintTracking::Configuration' for 'cpp/unclear-array-index-validation'.
1 parent d7652f9 commit 2328898

File tree

3 files changed

+95
-14
lines changed

3 files changed

+95
-14
lines changed

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

Lines changed: 61 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* @description Accessing an array without first checking
44
* that the index is within the bounds of the array can
55
* cause undefined behavior and can also be a security risk.
6-
* @kind problem
6+
* @kind path-problem
77
* @id cpp/unclear-array-index-validation
88
* @problem.severity warning
99
* @security-severity 8.8
@@ -12,9 +12,11 @@
1212
*/
1313

1414
import cpp
15-
import semmle.code.cpp.controlflow.Guards
16-
private import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
17-
import semmle.code.cpp.security.TaintTracking
15+
import semmle.code.cpp.controlflow.IRGuards
16+
import semmle.code.cpp.security.FlowSources
17+
import semmle.code.cpp.ir.dataflow.TaintTracking
18+
import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
19+
import DataFlow::PathGraph
1820

1921
predicate hasUpperBound(VariableAccess offsetExpr) {
2022
exists(BasicBlock controlled, StackVariable offsetVar, SsaDefinition def |
@@ -32,11 +34,60 @@ predicate linearBoundControls(BasicBlock controlled, SsaDefinition def, StackVar
3234
)
3335
}
3436

35-
from Expr origin, ArrayExpr arrayExpr, VariableAccess offsetExpr
36-
where
37-
tainted(origin, offsetExpr) and
38-
offsetExpr = arrayExpr.getArrayOffset() and
37+
predicate isUnboundedArrayIndex(DataFlow::Node sink, VariableAccess offsetExpr) {
38+
offsetExpr = sink.asExpr().(ArrayExpr).getArrayOffset() and
3939
not hasUpperBound(offsetExpr)
40-
select offsetExpr,
40+
}
41+
42+
predicate readsVariable(LoadInstruction load, Variable var) {
43+
load.getSourceAddress().(VariableAddressInstruction).getASTVariable() = var
44+
}
45+
46+
predicate hasUpperBoundsCheck(Variable var) {
47+
exists(RelationalOperation oper, VariableAccess access |
48+
oper.getAnOperand() = access and
49+
access.getTarget() = var and
50+
// Comparing to 0 is not an upper bound check
51+
not oper.getAnOperand().getValue() = "0"
52+
)
53+
}
54+
55+
predicate nodeIsBarrierEqualityCandidate(DataFlow::Node node, Operand access, Variable checkedVar) {
56+
readsVariable(node.asInstruction(), checkedVar) and
57+
any(IRGuardCondition guard).ensuresEq(access, _, _, node.asInstruction().getBlock(), true)
58+
}
59+
60+
predicate isFlowSource(FlowSource source, string sourceType) { sourceType = source.getSourceType() }
61+
62+
class ImproperArrayIndexValidationConfig extends TaintTracking::Configuration {
63+
ImproperArrayIndexValidationConfig() { this = "ImproperArrayIndexValidationConfig" }
64+
65+
override predicate isSource(DataFlow::Node source) { isFlowSource(source, _) }
66+
67+
override predicate isSanitizer(DataFlow::Node node) {
68+
hasUpperBound(node.asExpr())
69+
or
70+
exists(Variable checkedVar |
71+
readsVariable(node.asInstruction(), checkedVar) and
72+
hasUpperBoundsCheck(checkedVar)
73+
)
74+
or
75+
exists(Variable checkedVar, Operand access |
76+
readsVariable(access.getDef(), checkedVar) and
77+
nodeIsBarrierEqualityCandidate(node, access, checkedVar)
78+
)
79+
}
80+
81+
override predicate isSink(DataFlow::Node sink) { isUnboundedArrayIndex(sink, _) }
82+
}
83+
84+
from
85+
VariableAccess offsetExpr, ImproperArrayIndexValidationConfig conf, DataFlow::PathNode source,
86+
DataFlow::PathNode sink, string sourceType
87+
where
88+
conf.hasFlowPath(source, sink) and
89+
isFlowSource(source.getNode(), sourceType) and
90+
isUnboundedArrayIndex(sink.getNode(), offsetExpr)
91+
select sink.getNode(), source, sink,
4192
"$@ flows to here and is used in an array indexing expression, potentially causing an invalid access.",
42-
origin, "User-provided value"
93+
source.getNode(), sourceType

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,8 @@
1-
| 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 |
1+
edges
2+
| 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 |
3+
nodes
4+
| CWE122_Heap_Based_Buffer_Overflow__c_CWE129_fgets_01.c:30:19:30:29 | fgets output argument | semmle.label | fgets output argument |
5+
| CWE122_Heap_Based_Buffer_Overflow__c_CWE129_fgets_01.c:52:13:52:24 | access to array | semmle.label | access to array |
6+
subpaths
7+
#select
8+
| 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

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,26 @@
1-
| 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 |
2-
| 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 |
3-
| 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 |
1+
edges
2+
| test1.c:8:16:8:19 | argv | test1.c:9:9:9:9 | i |
3+
| test1.c:8:16:8:19 | argv | test1.c:11:9:11:9 | i |
4+
| test1.c:8:16:8:19 | argv | test1.c:13:9:13:9 | i |
5+
| test1.c:9:9:9:9 | i | test1.c:16:16:16:16 | i |
6+
| test1.c:11:9:11:9 | i | test1.c:32:16:32:16 | i |
7+
| test1.c:13:9:13:9 | i | test1.c:48:16:48:16 | i |
8+
| test1.c:16:16:16:16 | i | test1.c:18:12:18:17 | access to array |
9+
| test1.c:32:16:32:16 | i | test1.c:33:3:33:12 | access to array |
10+
| test1.c:48:16:48:16 | i | test1.c:53:7:53:16 | access to array |
11+
nodes
12+
| test1.c:8:16:8:19 | argv | semmle.label | argv |
13+
| test1.c:9:9:9:9 | i | semmle.label | i |
14+
| test1.c:11:9:11:9 | i | semmle.label | i |
15+
| test1.c:13:9:13:9 | i | semmle.label | i |
16+
| test1.c:16:16:16:16 | i | semmle.label | i |
17+
| test1.c:18:12:18:17 | access to array | semmle.label | access to array |
18+
| test1.c:32:16:32:16 | i | semmle.label | i |
19+
| test1.c:33:3:33:12 | access to array | semmle.label | access to array |
20+
| test1.c:48:16:48:16 | i | semmle.label | i |
21+
| test1.c:53:7:53:16 | access to array | semmle.label | access to array |
22+
subpaths
23+
#select
24+
| 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 |
25+
| 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 |
26+
| 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 |

0 commit comments

Comments
 (0)