Skip to content

Commit d706d7b

Browse files
rdmarsh2rdmarsh2
authored
Merge pull request github#5887 from MathiasVP/fewer-rand-sources-in-uncontrolled-arithmetic
C++: Add more sanitizers to `cpp/uncontrolled-arithmetic`
2 parents 17b7431 + 7d26aca commit d706d7b

File tree

4 files changed

+85
-78
lines changed

4 files changed

+85
-78
lines changed

‎cpp/change-notes/2021-12-05-uncontrolled-arithmetic.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* The query "Uncontrolled arithmetic" (`cpp/uncontrolled-arithmetic`) has been improved to produce fewer false positives.

‎cpp/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql

Lines changed: 80 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -15,34 +15,99 @@ import cpp
1515
import semmle.code.cpp.security.Overflow
1616
import semmle.code.cpp.security.Security
1717
import semmle.code.cpp.security.TaintTracking
18+
import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
1819
import TaintedWithPath
1920

20-
predicate isRandCall(FunctionCall fc) { fc.getTarget().getName() = "rand" }
21+
predicate isUnboundedRandCall(FunctionCall fc) {
22+
fc.getTarget().getName() = "rand" and not bounded(fc)
23+
}
24+
25+
/**
26+
* An operand `e` of a division expression (i.e., `e` is an operand of either a `DivExpr` or
27+
* a `AssignDivExpr`) is bounded when `e` is the left-hand side of the division.
28+
*/
29+
pragma[inline]
30+
predicate boundedDiv(Expr e, Expr left) { e = left }
31+
32+
/**
33+
* An operand `e` of a remainder expression `rem` (i.e., `rem` is either a `RemExpr` or
34+
* an `AssignRemExpr`) with left-hand side `left` and right-ahnd side `right` is bounded
35+
* when `e` is `left` and `right` is upper bounded by some number that is less than the maximum integer
36+
* allowed by the result type of `rem`.
37+
*/
38+
pragma[inline]
39+
predicate boundedRem(Expr e, Expr rem, Expr left, Expr right) {
40+
e = left and
41+
upperBound(right.getFullyConverted()) < exprMaxVal(rem.getFullyConverted())
42+
}
2143

22-
predicate isRandCallOrParent(Expr e) {
23-
isRandCall(e) or
24-
isRandCallOrParent(e.getAChild())
44+
/**
45+
* An operand `e` of a bitwise and expression `andExpr` (i.e., `andExpr` is either an `BitwiseAndExpr`
46+
* or an `AssignAndExpr`) with operands `operand1` and `operand2` is the operand that is not `e` is upper
47+
* bounded by some number that is less than the maximum integer allowed by the result type of `andExpr`.
48+
*/
49+
pragma[inline]
50+
predicate boundedBitwiseAnd(Expr e, Expr andExpr, Expr operand1, Expr operand2) {
51+
operand1 != operand2 and
52+
e = operand1 and
53+
upperBound(operand2.getFullyConverted()) < exprMaxVal(andExpr.getFullyConverted())
2554
}
2655

27-
predicate isRandValue(Expr e) {
28-
isRandCall(e)
56+
/**
57+
* Holds if `fc` is a part of the left operand of a binary operation that greatly reduces the range
58+
* of possible values.
59+
*/
60+
predicate bounded(Expr e) {
61+
// For `%` and `&` we require that `e` is bounded by a value that is strictly smaller than the
62+
// maximum possible value of the result type of the operation.
63+
// For example, the function call `rand()` is considered bounded in the following program:
64+
// ```
65+
// int i = rand() % (UINT8_MAX + 1);
66+
// ```
67+
// but not in:
68+
// ```
69+
// unsigned char uc = rand() % (UINT8_MAX + 1);
70+
// ```
71+
exists(RemExpr rem | boundedRem(e, rem, rem.getLeftOperand(), rem.getRightOperand()))
72+
or
73+
exists(AssignRemExpr rem | boundedRem(e, rem, rem.getLValue(), rem.getRValue()))
74+
or
75+
exists(BitwiseAndExpr andExpr |
76+
boundedBitwiseAnd(e, andExpr, andExpr.getAnOperand(), andExpr.getAnOperand())
77+
)
78+
or
79+
exists(AssignAndExpr andExpr |
80+
boundedBitwiseAnd(e, andExpr, andExpr.getAnOperand(), andExpr.getAnOperand())
81+
)
82+
or
83+
// Optimitically assume that a division always yields a much smaller value.
84+
boundedDiv(e, any(DivExpr div).getLeftOperand())
85+
or
86+
boundedDiv(e, any(AssignDivExpr div).getLValue())
87+
}
88+
89+
predicate isUnboundedRandCallOrParent(Expr e) {
90+
isUnboundedRandCall(e)
91+
or
92+
isUnboundedRandCallOrParent(e.getAChild())
93+
}
94+
95+
predicate isUnboundedRandValue(Expr e) {
96+
isUnboundedRandCall(e)
2997
or
3098
exists(MacroInvocation mi |
3199
e = mi.getExpr() and
32-
isRandCallOrParent(e)
100+
isUnboundedRandCallOrParent(e)
33101
)
34102
}
35103

36104
class SecurityOptionsArith extends SecurityOptions {
37105
override predicate isUserInput(Expr expr, string cause) {
38-
isRandValue(expr) and
39-
cause = "rand" and
40-
not expr.getParent*() instanceof DivExpr
106+
isUnboundedRandValue(expr) and
107+
cause = "rand"
41108
}
42109
}
43110

44-
predicate isDiv(VariableAccess va) { exists(AssignDivExpr div | div.getLValue() = va) }
45-
46111
predicate missingGuard(VariableAccess va, string effect) {
47112
exists(Operation op | op.getAnOperand() = va |
48113
missingGuardAgainstUnderflow(op, va) and effect = "underflow"
@@ -52,29 +117,15 @@ predicate missingGuard(VariableAccess va, string effect) {
52117
}
53118

54119
class Configuration extends TaintTrackingConfiguration {
55-
override predicate isSink(Element e) {
56-
isDiv(e)
57-
or
58-
missingGuard(e, _)
59-
}
60-
}
120+
override predicate isSink(Element e) { missingGuard(e, _) }
61121

62-
/**
63-
* A value that undergoes division is likely to be bounded within a safe
64-
* range.
65-
*/
66-
predicate guardedByAssignDiv(Expr origin) {
67-
exists(VariableAccess va |
68-
taintedWithPath(origin, va, _, _) and
69-
isDiv(va)
70-
)
122+
override predicate isBarrier(Expr e) { super.isBarrier(e) or bounded(e) }
71123
}
72124

73125
from Expr origin, VariableAccess va, string effect, PathNode sourceNode, PathNode sinkNode
74126
where
75127
taintedWithPath(origin, va, sourceNode, sinkNode) and
76-
missingGuard(va, effect) and
77-
not guardedByAssignDiv(origin)
128+
missingGuard(va, effect)
78129
select va, sourceNode, sinkNode,
79130
"$@ flows to here and is used in arithmetic, potentially causing an " + effect + ".", origin,
80131
"Uncontrolled value"

‎cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/uncontrolled/ArithmeticUncontrolled.expected

Lines changed: 0 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -7,30 +7,10 @@ edges
77
| test.c:34:13:34:18 | call to rand | test.c:35:5:35:5 | r |
88
| test.c:34:13:34:18 | call to rand | test.c:35:5:35:5 | r |
99
| test.c:34:13:34:18 | call to rand | test.c:35:5:35:5 | r |
10-
| test.c:39:13:39:21 | ... % ... | test.c:40:5:40:5 | r |
11-
| test.c:39:13:39:21 | ... % ... | test.c:40:5:40:5 | r |
12-
| test.c:39:13:39:21 | ... % ... | test.c:40:5:40:5 | r |
13-
| test.c:39:13:39:21 | ... % ... | test.c:40:5:40:5 | r |
1410
| test.c:44:13:44:16 | call to rand | test.c:45:5:45:5 | r |
1511
| test.c:44:13:44:16 | call to rand | test.c:45:5:45:5 | r |
1612
| test.c:44:13:44:16 | call to rand | test.c:45:5:45:5 | r |
1713
| test.c:44:13:44:16 | call to rand | test.c:45:5:45:5 | r |
18-
| test.c:54:13:54:16 | call to rand | test.c:56:5:56:5 | r |
19-
| test.c:54:13:54:16 | call to rand | test.c:56:5:56:5 | r |
20-
| test.c:54:13:54:16 | call to rand | test.c:56:5:56:5 | r |
21-
| test.c:54:13:54:16 | call to rand | test.c:56:5:56:5 | r |
22-
| test.c:60:13:60:16 | call to rand | test.c:61:5:61:5 | r |
23-
| test.c:60:13:60:16 | call to rand | test.c:61:5:61:5 | r |
24-
| test.c:60:13:60:16 | call to rand | test.c:61:5:61:5 | r |
25-
| test.c:60:13:60:16 | call to rand | test.c:61:5:61:5 | r |
26-
| test.c:60:13:60:16 | call to rand | test.c:62:5:62:5 | r |
27-
| test.c:60:13:60:16 | call to rand | test.c:62:5:62:5 | r |
28-
| test.c:60:13:60:16 | call to rand | test.c:62:5:62:5 | r |
29-
| test.c:60:13:60:16 | call to rand | test.c:62:5:62:5 | r |
30-
| test.c:66:13:66:16 | call to rand | test.c:67:5:67:5 | r |
31-
| test.c:66:13:66:16 | call to rand | test.c:67:5:67:5 | r |
32-
| test.c:66:13:66:16 | call to rand | test.c:67:5:67:5 | r |
33-
| test.c:66:13:66:16 | call to rand | test.c:67:5:67:5 | r |
3414
| test.c:75:13:75:19 | ... ^ ... | test.c:77:9:77:9 | r |
3515
| test.c:75:13:75:19 | ... ^ ... | test.c:77:9:77:9 | r |
3616
| test.c:75:13:75:19 | ... ^ ... | test.c:77:9:77:9 | r |
@@ -67,34 +47,11 @@ nodes
6747
| test.c:35:5:35:5 | r | semmle.label | r |
6848
| test.c:35:5:35:5 | r | semmle.label | r |
6949
| test.c:35:5:35:5 | r | semmle.label | r |
70-
| test.c:39:13:39:21 | ... % ... | semmle.label | ... % ... |
71-
| test.c:39:13:39:21 | ... % ... | semmle.label | ... % ... |
72-
| test.c:40:5:40:5 | r | semmle.label | r |
73-
| test.c:40:5:40:5 | r | semmle.label | r |
74-
| test.c:40:5:40:5 | r | semmle.label | r |
7550
| test.c:44:13:44:16 | call to rand | semmle.label | call to rand |
7651
| test.c:44:13:44:16 | call to rand | semmle.label | call to rand |
7752
| test.c:45:5:45:5 | r | semmle.label | r |
7853
| test.c:45:5:45:5 | r | semmle.label | r |
7954
| test.c:45:5:45:5 | r | semmle.label | r |
80-
| test.c:54:13:54:16 | call to rand | semmle.label | call to rand |
81-
| test.c:54:13:54:16 | call to rand | semmle.label | call to rand |
82-
| test.c:56:5:56:5 | r | semmle.label | r |
83-
| test.c:56:5:56:5 | r | semmle.label | r |
84-
| test.c:56:5:56:5 | r | semmle.label | r |
85-
| test.c:60:13:60:16 | call to rand | semmle.label | call to rand |
86-
| test.c:60:13:60:16 | call to rand | semmle.label | call to rand |
87-
| test.c:61:5:61:5 | r | semmle.label | r |
88-
| test.c:61:5:61:5 | r | semmle.label | r |
89-
| test.c:61:5:61:5 | r | semmle.label | r |
90-
| test.c:62:5:62:5 | r | semmle.label | r |
91-
| test.c:62:5:62:5 | r | semmle.label | r |
92-
| test.c:62:5:62:5 | r | semmle.label | r |
93-
| test.c:66:13:66:16 | call to rand | semmle.label | call to rand |
94-
| test.c:66:13:66:16 | call to rand | semmle.label | call to rand |
95-
| test.c:67:5:67:5 | r | semmle.label | r |
96-
| test.c:67:5:67:5 | r | semmle.label | r |
97-
| test.c:67:5:67:5 | r | semmle.label | r |
9855
| test.c:75:13:75:19 | ... ^ ... | semmle.label | ... ^ ... |
9956
| test.c:75:13:75:19 | ... ^ ... | semmle.label | ... ^ ... |
10057
| test.c:77:9:77:9 | r | semmle.label | r |
@@ -133,10 +90,7 @@ nodes
13390
#select
13491
| test.c:21:17:21:17 | r | test.c:18:13:18:16 | call to rand | test.c:21:17:21:17 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:18:13:18:16 | call to rand | Uncontrolled value |
13592
| test.c:35:5:35:5 | r | test.c:34:13:34:18 | call to rand | test.c:35:5:35:5 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:34:13:34:18 | call to rand | Uncontrolled value |
136-
| test.c:40:5:40:5 | r | test.c:39:13:39:21 | ... % ... | test.c:40:5:40:5 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:39:13:39:21 | ... % ... | Uncontrolled value |
13793
| test.c:45:5:45:5 | r | test.c:44:13:44:16 | call to rand | test.c:45:5:45:5 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:44:13:44:16 | call to rand | Uncontrolled value |
138-
| test.c:56:5:56:5 | r | test.c:54:13:54:16 | call to rand | test.c:56:5:56:5 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:54:13:54:16 | call to rand | Uncontrolled value |
139-
| test.c:67:5:67:5 | r | test.c:66:13:66:16 | call to rand | test.c:67:5:67:5 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:66:13:66:16 | call to rand | Uncontrolled value |
14094
| test.c:77:9:77:9 | r | test.c:75:13:75:19 | ... ^ ... | test.c:77:9:77:9 | r | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:75:13:75:19 | ... ^ ... | Uncontrolled value |
14195
| test.c:100:5:100:5 | r | test.c:99:14:99:19 | call to rand | test.c:100:5:100:5 | r | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:99:14:99:19 | call to rand | Uncontrolled value |
14296
| test.cpp:25:7:25:7 | r | test.cpp:8:9:8:12 | call to rand | test.cpp:25:7:25:7 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:8:9:8:12 | call to rand | Uncontrolled value |

‎cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/uncontrolled/test.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ void randomTester() {
3737

3838
{
3939
int r = RANDN(100);
40-
r += 100; // GOOD: The return from RANDN is bounded [FALSE POSITIVE]
40+
r += 100; // GOOD: The return from RANDN is bounded
4141
}
4242

4343
{
@@ -53,7 +53,7 @@ void randomTester() {
5353
{
5454
int r = rand();
5555
r = r / 10;
56-
r += 100; // GOOD [FALSE POSITIVE]
56+
r += 100; // GOOD
5757
}
5858

5959
{
@@ -64,7 +64,7 @@ void randomTester() {
6464

6565
{
6666
int r = rand() & 0xFF;
67-
r += 100; // GOOD [FALSE POSITIVE]
67+
r += 100; // GOOD
6868
}
6969

7070
{

0 commit comments

Comments
 (0)