Skip to content

Commit 8ce6335

Browse files
MathiasVPMathiasVP
authored
Merge pull request github#6372 from geoffw0/uncontrolledarith
2 parents eaf3d3c + 54253bc commit 8ce6335

File tree

5 files changed

+198
-12
lines changed

5 files changed

+198
-12
lines changed

‎cpp/change-notes/2021-07-27-uncontrolled-arithmetic.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm
2+
* Improvements made to the (`cpp/uncontrolled-arithmetic`) query, reducing the frequency of false positive results.

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

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,15 @@ private class RandS extends RandomFunction {
7474

7575
predicate missingGuard(VariableAccess va, string effect) {
7676
exists(Operation op | op.getAnOperand() = va |
77-
missingGuardAgainstUnderflow(op, va) and effect = "underflow"
77+
// underflow - random numbers are usually non-negative, so underflow is
78+
// only likely if the type is unsigned. Multiplication is also unlikely to
79+
// cause underflow of a non-negative number.
80+
missingGuardAgainstUnderflow(op, va) and
81+
effect = "underflow" and
82+
op.getUnspecifiedType().(IntegralType).isUnsigned() and
83+
not op instanceof MulExpr
7884
or
85+
// overflow
7986
missingGuardAgainstOverflow(op, va) and effect = "overflow"
8087
)
8188
}
@@ -108,6 +115,9 @@ class UncontrolledArithConfiguration extends TaintTracking::Configuration {
108115
op instanceof BitwiseAndExpr or
109116
op instanceof ComplementExpr
110117
).getAnOperand*()
118+
or
119+
// block unintended flow to pointers
120+
node.asExpr().getUnspecifiedType() instanceof PointerType
111121
}
112122
}
113123

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

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,9 @@ edges
77
| test.c:81:14:81:17 | call to rand | test.c:83:9:83:9 | r |
88
| test.c:81:23:81:26 | call to rand | test.c:83:9:83:9 | r |
99
| test.c:99:14:99:19 | call to rand | test.c:100:5:100:5 | r |
10+
| test.c:125:13:125:16 | call to rand | test.c:127:9:127:9 | r |
11+
| test.c:148:22:148:25 | call to rand | test.c:150:9:150:9 | r |
12+
| test.c:148:22:148:27 | (unsigned int)... | test.c:150:9:150:9 | r |
1013
| test.cpp:8:9:8:12 | Store | test.cpp:24:11:24:18 | call to get_rand |
1114
| test.cpp:8:9:8:12 | call to rand | test.cpp:8:9:8:12 | Store |
1215
| test.cpp:13:2:13:15 | Chi [[]] | test.cpp:30:13:30:14 | get_rand2 output argument [[]] |
@@ -18,6 +21,11 @@ edges
1821
| test.cpp:30:13:30:14 | get_rand2 output argument [[]] | test.cpp:30:13:30:14 | Chi |
1922
| test.cpp:36:13:36:13 | Chi | test.cpp:37:7:37:7 | r |
2023
| test.cpp:36:13:36:13 | get_rand3 output argument [[]] | test.cpp:36:13:36:13 | Chi |
24+
| test.cpp:78:10:78:13 | call to rand | test.cpp:82:10:82:10 | x |
25+
| test.cpp:90:10:90:13 | call to rand | test.cpp:94:10:94:10 | x |
26+
| test.cpp:129:10:129:13 | call to rand | test.cpp:132:10:132:10 | b |
27+
| test.cpp:147:11:147:14 | call to rand | test.cpp:149:11:149:16 | (int)... |
28+
| test.cpp:147:11:147:14 | call to rand | test.cpp:149:16:149:16 | y |
2129
nodes
2230
| test.c:18:13:18:16 | call to rand | semmle.label | call to rand |
2331
| test.c:21:17:21:17 | r | semmle.label | r |
@@ -33,6 +41,11 @@ nodes
3341
| test.c:83:9:83:9 | r | semmle.label | r |
3442
| test.c:99:14:99:19 | call to rand | semmle.label | call to rand |
3543
| test.c:100:5:100:5 | r | semmle.label | r |
44+
| test.c:125:13:125:16 | call to rand | semmle.label | call to rand |
45+
| test.c:127:9:127:9 | r | semmle.label | r |
46+
| test.c:148:22:148:25 | call to rand | semmle.label | call to rand |
47+
| test.c:148:22:148:27 | (unsigned int)... | semmle.label | (unsigned int)... |
48+
| test.c:150:9:150:9 | r | semmle.label | r |
3649
| test.cpp:8:9:8:12 | Store | semmle.label | Store |
3750
| test.cpp:8:9:8:12 | call to rand | semmle.label | call to rand |
3851
| test.cpp:13:2:13:15 | Chi [[]] | semmle.label | Chi [[]] |
@@ -47,15 +60,32 @@ nodes
4760
| test.cpp:36:13:36:13 | Chi | semmle.label | Chi |
4861
| test.cpp:36:13:36:13 | get_rand3 output argument [[]] | semmle.label | get_rand3 output argument [[]] |
4962
| test.cpp:37:7:37:7 | r | semmle.label | r |
63+
| test.cpp:78:10:78:13 | call to rand | semmle.label | call to rand |
64+
| test.cpp:82:10:82:10 | x | semmle.label | x |
65+
| test.cpp:90:10:90:13 | call to rand | semmle.label | call to rand |
66+
| test.cpp:94:10:94:10 | x | semmle.label | x |
67+
| test.cpp:129:10:129:13 | call to rand | semmle.label | call to rand |
68+
| test.cpp:132:10:132:10 | b | semmle.label | b |
69+
| test.cpp:147:11:147:14 | call to rand | semmle.label | call to rand |
70+
| test.cpp:149:11:149:16 | (int)... | semmle.label | (int)... |
71+
| test.cpp:149:16:149:16 | y | semmle.label | y |
5072
#select
5173
| 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 |
5274
| 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 |
5375
| 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 |
54-
| test.c:77:9:77:9 | r | test.c:75:13:75:19 | call to rand | 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 | call to rand | Uncontrolled value |
55-
| test.c:77:9:77:9 | r | test.c:75:13:75:19 | call to rand | 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 | call to rand | Uncontrolled value |
56-
| test.c:83:9:83:9 | r | test.c:81:14:81:17 | call to rand | test.c:83:9:83:9 | r | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:81:14:81:17 | call to rand | Uncontrolled value |
57-
| test.c:83:9:83:9 | r | test.c:81:23:81:26 | call to rand | test.c:83:9:83:9 | r | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:81:23:81:26 | call to rand | Uncontrolled value |
58-
| 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 |
76+
| test.c:77:9:77:9 | r | test.c:75:13:75:19 | call to rand | test.c:77:9:77:9 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:75:13:75:19 | call to rand | Uncontrolled value |
77+
| test.c:77:9:77:9 | r | test.c:75:13:75:19 | call to rand | test.c:77:9:77:9 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:75:13:75:19 | call to rand | Uncontrolled value |
78+
| test.c:83:9:83:9 | r | test.c:81:14:81:17 | call to rand | test.c:83:9:83:9 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:81:14:81:17 | call to rand | Uncontrolled value |
79+
| test.c:83:9:83:9 | r | test.c:81:23:81:26 | call to rand | test.c:83:9:83:9 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:81:23:81:26 | call to rand | Uncontrolled value |
80+
| 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 overflow. | test.c:99:14:99:19 | call to rand | Uncontrolled value |
81+
| test.c:127:9:127:9 | r | test.c:125:13:125:16 | call to rand | test.c:127:9:127:9 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:125:13:125:16 | call to rand | Uncontrolled value |
82+
| test.c:150:9:150:9 | r | test.c:148:22:148:25 | call to rand | test.c:150:9:150:9 | r | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:148:22:148:25 | call to rand | Uncontrolled value |
83+
| test.c:150:9:150:9 | r | test.c:148:22:148:27 | (unsigned int)... | test.c:150:9:150:9 | r | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:148:22:148:25 | call to rand | Uncontrolled value |
5984
| 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 |
6085
| test.cpp:31:7:31:7 | r | test.cpp:13:10:13:13 | call to rand | test.cpp:31:7:31:7 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:13:10:13:13 | call to rand | Uncontrolled value |
6186
| test.cpp:37:7:37:7 | r | test.cpp:18:9:18:12 | call to rand | test.cpp:37:7:37:7 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:18:9:18:12 | call to rand | Uncontrolled value |
87+
| test.cpp:82:10:82:10 | x | test.cpp:78:10:78:13 | call to rand | test.cpp:82:10:82:10 | x | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:78:10:78:13 | call to rand | Uncontrolled value |
88+
| test.cpp:94:10:94:10 | x | test.cpp:90:10:90:13 | call to rand | test.cpp:94:10:94:10 | x | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:90:10:90:13 | call to rand | Uncontrolled value |
89+
| test.cpp:132:10:132:10 | b | test.cpp:129:10:129:13 | call to rand | test.cpp:132:10:132:10 | b | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:129:10:129:13 | call to rand | Uncontrolled value |
90+
| test.cpp:149:11:149:16 | (int)... | test.cpp:147:11:147:14 | call to rand | test.cpp:149:11:149:16 | (int)... | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:147:11:147:14 | call to rand | Uncontrolled value |
91+
| test.cpp:149:16:149:16 | y | test.cpp:147:11:147:14 | call to rand | test.cpp:149:16:149:16 | y | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:147:11:147:14 | call to rand | Uncontrolled value |

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

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -74,30 +74,30 @@ void randomTester() {
7474
{
7575
int r = RAND2();
7676

77-
r = r - 100; // BAD
77+
r = r + 100; // BAD
7878
}
7979

8080
{
8181
int r = (rand() ^ rand());
8282

83-
r = r - 100; // BAD
83+
r = r + 100; // BAD
8484
}
8585

8686
{
87-
int r = RAND2() - 100; // BAD [NOT DETECTED]
87+
int r = RAND2() + 100; // BAD [NOT DETECTED]
8888
}
8989

9090
{
9191
int r = RAND();
9292
int *ptr_r = &r;
93-
*ptr_r -= 100; // BAD [NOT DETECTED]
93+
*ptr_r += 100; // BAD [NOT DETECTED]
9494
}
9595

9696
{
9797
int r = 0;
9898
int *ptr_r = &r;
9999
*ptr_r = RAND();
100-
r -= 100; // BAD
100+
r += 100; // BAD
101101
}
102102

103103
{
@@ -119,3 +119,34 @@ void randomTester2(int bound, int min, int max) {
119119
int r2 = (rand() % (max - min + 1)) + min;
120120
r2 += 100; // GOOD (This is a common way to clamp the random value between [min, max])
121121
}
122+
123+
void moreTests() {
124+
{
125+
int r = rand();
126+
127+
r = r * 100; // BAD
128+
}
129+
{
130+
int r = rand();
131+
132+
r *= 100; // BAD [NOT DETECTED]
133+
}
134+
135+
{
136+
int r = rand();
137+
138+
r <<= 8; // BAD [NOT DETECTED]
139+
}
140+
141+
{
142+
int r = rand();
143+
144+
r = r - 100; // GOOD
145+
}
146+
147+
{
148+
unsigned int r = rand();
149+
150+
r = r - 100; // BAD
151+
}
152+
}

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

Lines changed: 114 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,4 +47,117 @@ void test_with_bounded_randomness() {
4747

4848
unsigned unsigned_r = rand(10);
4949
unsigned_r++; // GOOD
50-
}
50+
}
51+
52+
int test_remainder_subtract()
53+
{
54+
int x = rand();
55+
int y = x % 100; // y <= x
56+
57+
return x - y; // GOOD (as y <= x)
58+
}
59+
60+
typedef unsigned long size_t;
61+
int snprintf(char *s, size_t n, const char *format, ...);
62+
63+
int test_buffer(char *buf_start, char *buf_end)
64+
{
65+
int len = buf_end - buf_start;
66+
67+
return len * 2; // GOOD
68+
}
69+
70+
int test_snprintf(char *buf, size_t buf_sz)
71+
{
72+
snprintf(buf, buf_sz, "my random number: %i\n", rand());
73+
test_buffer(buf, buf + buf_sz);
74+
}
75+
76+
int test_else_1()
77+
{
78+
int x = rand();
79+
80+
if (x > 100)
81+
{
82+
return x * 10; // BAD
83+
} else {
84+
return x * 10; // GOOD (as x <= 100)
85+
}
86+
}
87+
88+
int test_else_2()
89+
{
90+
int x = rand();
91+
92+
if (x > 100)
93+
{
94+
return x * 10; // BAD
95+
}
96+
97+
return x * 10; // GOOD (as x <= 100)
98+
}
99+
100+
int test_conditional_assignment_1()
101+
{
102+
int x = rand();
103+
int y = 100;
104+
105+
if (x < y)
106+
{
107+
y = x;
108+
return y * 10; // GOOD (as y <= 100)
109+
} else {
110+
return y * 10; // GOOD (as y = 100)
111+
}
112+
}
113+
114+
int test_conditional_assignment_2()
115+
{
116+
int x = rand();
117+
int y = 100;
118+
119+
if (x < y)
120+
{
121+
y = x;
122+
}
123+
124+
return y * 10; // GOOD (as y <= 100)
125+
}
126+
127+
int test_underflow()
128+
{
129+
int x = rand();
130+
int a = -x; // GOOD
131+
int b = 10 - x; // GOOD
132+
int c = b * 2; // BAD
133+
}
134+
135+
int test_cast()
136+
{
137+
int x = rand();
138+
short a = x; // BAD [NOT DETECTED]
139+
short b = -x; // BAD [NOT DETECTED]
140+
long long c = x; // GOOD
141+
long long d = -x; // GOOD
142+
}
143+
144+
void test_float()
145+
{
146+
{
147+
int x = rand();
148+
float y = x; // GOOD
149+
int z = (int)y * 5; // BAD
150+
}
151+
152+
{
153+
int x = rand();
154+
float y = x * 5.0f; // GOOD
155+
int z = y; // BAD [NOT DETECTED]
156+
}
157+
158+
{
159+
int x = rand();
160+
float y = x / 10.0f; // GOOD
161+
int z = (int)y * 5; // GOOD
162+
}
163+
}

0 commit comments

Comments
 (0)