Skip to content

Commit 52fb00c

Browse files
MathiasVPMathiasVP
authored
Merge pull request github#12036 from nmouha/patch-1
CPP: Add query for CVE-2022-37454: Integer addition may overflow inside if statement
2 parents 06d48dc + 27519ce commit 52fb00c

File tree

5 files changed

+174
-0
lines changed

5 files changed

+174
-0
lines changed

‎cpp/ql/src/experimental/Security/CWE/CWE-190/IfStatementAdditionOverflow.qhelp

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
Detects <code>if (a+b>c) a=c-b</code>, which incorrectly implements
9+
<code>a = min(a,c-b)</code> if <code>a+b</code> overflows.
10+
</p>
11+
<p>
12+
Also detects variants such as <code>if (b+a>c) a=c-b</code> (swapped
13+
terms in addition), <code>if (a+b>c) { a=c-b }</code> (assignment
14+
inside block), <code>c&lt;a+b</code> (swapped operands), and
15+
<code>&gt;=</code>, <code>&lt;</code>, <code>&lt;=</code> instead of
16+
<code>&gt;</code> (all operators).
17+
</p>
18+
<p>
19+
This integer overflow is the root cause of the buffer overflow in
20+
the SHA-3 reference implementation (CVE-2022-37454).
21+
</p>
22+
</overview>
23+
<recommendation>
24+
<p>
25+
Replace by <code>if (a>c-b) a=c-b</code>. This avoids the overflow
26+
and makes it easy to see that <code>a = min(a,c-b)</code>.
27+
</p>
28+
</recommendation>
29+
<references>
30+
<li>CVE-2022-37454: <a href="https://nvd.nist.gov/vuln/detail/CVE-2022-37454">The Keccak XKCP SHA-3 reference implementation before fdc6fef has an integer overflow and resultant buffer overflow that allows attackers to execute arbitrary code or eliminate expected cryptographic properties. This occurs in the sponge function interface.</a></li>
31+
<li>GitHub Advisory Database: <a href="https://github.com/advisories/GHSA-6w4m-2xhg-2658">CVE-2022-37454: Buffer overflow in sponge queue functions</a></li>
32+
</references>
33+
</qhelp>

‎cpp/ql/src/experimental/Security/CWE/CWE-190/IfStatementAdditionOverflow.ql

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
/**
2+
* @name Integer addition may overflow inside if statement
3+
* @description Writing 'if (a+b>c) a=c-b' incorrectly implements
4+
* 'a = min(a,c-b)' if 'a+b' overflows. This integer
5+
* overflow is the root cause of the buffer overflow
6+
* in the SHA-3 reference implementation (CVE-2022-37454).
7+
* @kind problem
8+
* @problem.severity warning
9+
* @id cpp/if-statement-addition-overflow
10+
* @tags: experimental
11+
* correctness
12+
* security
13+
* external/cwe/cwe-190
14+
*/
15+
16+
import cpp
17+
import semmle.code.cpp.valuenumbering.GlobalValueNumbering
18+
import semmle.code.cpp.valuenumbering.HashCons
19+
import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
20+
import semmle.code.cpp.controlflow.Guards
21+
22+
from
23+
GuardCondition guard, Expr expr, ExprStmt exprstmt, BasicBlock block, AssignExpr assignexpr,
24+
AddExpr addexpr, SubExpr subexpr
25+
where
26+
(guard.ensuresLt(expr, addexpr, 0, block, _) or guard.ensuresLt(addexpr, expr, 0, block, _)) and
27+
addexpr.getUnspecifiedType() instanceof IntegralType and
28+
exprMightOverflowPositively(addexpr) and
29+
block.getANode() = exprstmt and
30+
exprstmt.getExpr() = assignexpr and
31+
assignexpr.getRValue() = subexpr and
32+
(
33+
hashCons(addexpr.getLeftOperand()) = hashCons(assignexpr.getLValue()) and
34+
globalValueNumber(addexpr.getRightOperand()) = globalValueNumber(subexpr.getRightOperand())
35+
or
36+
hashCons(addexpr.getRightOperand()) = hashCons(assignexpr.getLValue()) and
37+
globalValueNumber(addexpr.getLeftOperand()) = globalValueNumber(subexpr.getRightOperand())
38+
) and
39+
globalValueNumber(expr) = globalValueNumber(subexpr.getLeftOperand())
40+
select guard,
41+
"\"if (a+b>c) a=c-b\" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as \"if (a>c-b) a=c-b\" which avoids the overflow.",
42+
addexpr, "addition"

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
| test.cpp:18:6:18:10 | ... > ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:18:6:18:8 | ... + ... | addition |
2+
| test.cpp:19:6:19:10 | ... > ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:19:6:19:8 | ... + ... | addition |
3+
| test.cpp:20:6:20:10 | ... > ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:20:6:20:8 | ... + ... | addition |
4+
| test.cpp:21:6:21:10 | ... > ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:21:6:21:8 | ... + ... | addition |
5+
| test.cpp:22:6:22:10 | ... > ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:22:8:22:10 | ... + ... | addition |
6+
| test.cpp:23:6:23:10 | ... > ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:23:8:23:10 | ... + ... | addition |
7+
| test.cpp:24:6:24:10 | ... > ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:24:8:24:10 | ... + ... | addition |
8+
| test.cpp:25:6:25:10 | ... > ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:25:8:25:10 | ... + ... | addition |
9+
| test.cpp:27:6:27:11 | ... >= ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:27:6:27:8 | ... + ... | addition |
10+
| test.cpp:28:6:28:11 | ... >= ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:28:6:28:8 | ... + ... | addition |
11+
| test.cpp:29:6:29:11 | ... >= ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:29:6:29:8 | ... + ... | addition |
12+
| test.cpp:30:6:30:11 | ... >= ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:30:6:30:8 | ... + ... | addition |
13+
| test.cpp:31:6:31:11 | ... >= ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:31:9:31:11 | ... + ... | addition |
14+
| test.cpp:32:6:32:11 | ... >= ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:32:9:32:11 | ... + ... | addition |
15+
| test.cpp:33:6:33:11 | ... >= ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:33:9:33:11 | ... + ... | addition |
16+
| test.cpp:34:6:34:11 | ... >= ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:34:9:34:11 | ... + ... | addition |
17+
| test.cpp:36:6:36:10 | ... < ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:36:6:36:8 | ... + ... | addition |
18+
| test.cpp:37:6:37:10 | ... < ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:37:6:37:8 | ... + ... | addition |
19+
| test.cpp:38:6:38:10 | ... < ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:38:6:38:8 | ... + ... | addition |
20+
| test.cpp:39:6:39:10 | ... < ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:39:6:39:8 | ... + ... | addition |
21+
| test.cpp:40:6:40:10 | ... < ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:40:8:40:10 | ... + ... | addition |
22+
| test.cpp:41:6:41:10 | ... < ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:41:8:41:10 | ... + ... | addition |
23+
| test.cpp:42:6:42:10 | ... < ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:42:8:42:10 | ... + ... | addition |
24+
| test.cpp:43:6:43:10 | ... < ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:43:8:43:10 | ... + ... | addition |
25+
| test.cpp:45:6:45:11 | ... <= ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:45:6:45:8 | ... + ... | addition |
26+
| test.cpp:46:6:46:11 | ... <= ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:46:6:46:8 | ... + ... | addition |
27+
| test.cpp:47:6:47:11 | ... <= ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:47:6:47:8 | ... + ... | addition |
28+
| test.cpp:48:6:48:11 | ... <= ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:48:6:48:8 | ... + ... | addition |
29+
| test.cpp:49:6:49:11 | ... <= ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:49:9:49:11 | ... + ... | addition |
30+
| test.cpp:50:6:50:11 | ... <= ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:50:9:50:11 | ... + ... | addition |
31+
| test.cpp:51:6:51:11 | ... <= ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:51:9:51:11 | ... + ... | addition |
32+
| test.cpp:52:6:52:11 | ... <= ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:52:9:52:11 | ... + ... | addition |
33+
| test.cpp:54:6:54:10 | ... > ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:54:6:54:8 | ... + ... | addition |
34+
| test.cpp:61:6:61:11 | ... <= ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:61:6:61:8 | ... + ... | addition |
35+
| test.cpp:62:6:62:11 | ... <= ... | "if (a+b>c) a=c-b" was detected where the $@ may potentially overflow/wraparound. The code can be rewritten as "if (a>c-b) a=c-b" which avoids the overflow. | test.cpp:62:6:62:8 | ... + ... | addition |

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
experimental/Security/CWE/CWE-190/IfStatementAdditionOverflow.ql

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

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
2+
int getAnInt();
3+
double getADouble();
4+
unsigned short getAnUnsignedShort();
5+
6+
void test()
7+
{
8+
int a = getAnInt();
9+
int b = getAnInt();
10+
int c = getAnInt();
11+
int x = getAnInt();
12+
int y = getAnInt();
13+
double d = getADouble();
14+
unsigned short a1 = getAnUnsignedShort();
15+
unsigned short b1 = getAnUnsignedShort();
16+
unsigned short c1 = getAnUnsignedShort();
17+
18+
if (a+b>c) a = c-b; // BAD
19+
if (a+b>c) { a = c-b; } // BAD
20+
if (b+a>c) a = c-b; // BAD
21+
if (b+a>c) { a = c-b; } // BAD
22+
if (c>a+b) a = c-b; // BAD
23+
if (c>a+b) { a = c-b; } // BAD
24+
if (c>b+a) a = c-b; // BAD
25+
if (c>b+a) { a = c-b; } // BAD
26+
27+
if (a+b>=c) a = c-b; // BAD
28+
if (a+b>=c) { a = c-b; } // BAD
29+
if (b+a>=c) a = c-b; // BAD
30+
if (b+a>=c) { a = c-b; } // BAD
31+
if (c>=a+b) a = c-b; // BAD
32+
if (c>=a+b) { a = c-b; } // BAD
33+
if (c>=b+a) a = c-b; // BAD
34+
if (c>=b+a) { a = c-b; } // BAD
35+
36+
if (a+b<c) a = c-b; // BAD
37+
if (a+b<c) { a = c-b; } // BAD
38+
if (b+a<c) a = c-b; // BAD
39+
if (b+a<c) { a = c-b; } // BAD
40+
if (c<a+b) a = c-b; // BAD
41+
if (c<a+b) { a = c-b; } // BAD
42+
if (c<b+a) a = c-b; // BAD
43+
if (c<b+a) { a = c-b; } // BAD
44+
45+
if (a+b<=c) a = c-b; // BAD
46+
if (a+b<=c) { a = c-b; } // BAD
47+
if (b+a<=c) a = c-b; // BAD
48+
if (b+a<=c) { a = c-b; } // BAD
49+
if (c<=a+b) a = c-b; // BAD
50+
if (c<=a+b) { a = c-b; } // BAD
51+
if (c<=b+a) a = c-b; // BAD
52+
if (c<=b+a) { a = c-b; } // BAD
53+
54+
if (a+b>d) a = d-b; // BAD
55+
if (a+(double)b>c) a = c-b; // GOOD
56+
if (a+(-x)>c) a = c-(-y); // GOOD
57+
if (a+b>c) { b++; a = c-b; } // GOOD
58+
if (a+d>c) a = c-d; // GOOD
59+
if (a1+b1>c1) a1 = c1-b1; // GOOD
60+
61+
if (a+b<=c) { /* ... */ } else { a = c-b; } // BAD
62+
if (a+b<=c) { return; } a = c-b; // BAD
63+
}

0 commit comments

Comments
 (0)