New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CPP: Add query for CVE-2022-37454: Integer addition may overflow inside if statement #12036
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
5a4a63f
Create IfStatementAdditionOverflow.ql
nmouha f577a04
Update IfStatementAdditionOverflow.ql
nmouha ed75172
Update IfStatementAdditionOverflow.ql
nmouha 08f04d5
Update IfStatementAdditionOverflow.ql
nmouha dc09c92
Update IfStatementAdditionOverflow.ql
nmouha 91a9a7e
Create test.cpp
nmouha 2477c3a
Update test.cpp
nmouha 59c1ae7
Update test.cpp
nmouha 66710ad
Create IfStatementAdditionOverflow.qlref
nmouha a2b5fbf
Create IfStatementAdditionOverflow.expected
nmouha 2de0e22
Update test.cpp
nmouha 5c6fc2f
Update IfStatementAdditionOverflow.ql
nmouha ef57861
Update IfStatementAdditionOverflow.expected
nmouha 187299f
Update test.cpp
nmouha 27519ce
Create IfStatementAdditionOverflow.qhelp
nmouha File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
33 changes: 33 additions & 0 deletions
33
cpp/ql/src/experimental/Security/CWE/CWE-190/IfStatementAdditionOverflow.qhelp
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| <!DOCTYPE qhelp PUBLIC | ||
| "-//Semmle//qhelp//EN" | ||
| "qhelp.dtd"> | ||
| <qhelp> | ||
|
|
||
| <overview> | ||
| <p> | ||
| Detects <code>if (a+b>c) a=c-b</code>, which incorrectly implements | ||
| <code>a = min(a,c-b)</code> if <code>a+b</code> overflows. | ||
| </p> | ||
| <p> | ||
| Also detects variants such as <code>if (b+a>c) a=c-b</code> (swapped | ||
| terms in addition), <code>if (a+b>c) { a=c-b }</code> (assignment | ||
| inside block), <code>c<a+b</code> (swapped operands), and | ||
| <code>>=</code>, <code><</code>, <code><=</code> instead of | ||
| <code>></code> (all operators). | ||
| </p> | ||
| <p> | ||
| This integer overflow is the root cause of the buffer overflow in | ||
| the SHA-3 reference implementation (CVE-2022-37454). | ||
| </p> | ||
| </overview> | ||
| <recommendation> | ||
| <p> | ||
| Replace by <code>if (a>c-b) a=c-b</code>. This avoids the overflow | ||
| and makes it easy to see that <code>a = min(a,c-b)</code>. | ||
| </p> | ||
| </recommendation> | ||
| <references> | ||
| <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> | ||
| <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> | ||
| </references> | ||
| </qhelp> |
42 changes: 42 additions & 0 deletions
42
cpp/ql/src/experimental/Security/CWE/CWE-190/IfStatementAdditionOverflow.ql
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| /** | ||
| * @name Integer addition may overflow inside if statement | ||
| * @description Writing 'if (a+b>c) a=c-b' incorrectly implements | ||
| * 'a = min(a,c-b)' if 'a+b' overflows. This integer | ||
| * overflow is the root cause of the buffer overflow | ||
| * in the SHA-3 reference implementation (CVE-2022-37454). | ||
| * @kind problem | ||
| * @problem.severity warning | ||
| * @id cpp/if-statement-addition-overflow | ||
| * @tags: experimental | ||
| * correctness | ||
| * security | ||
| * external/cwe/cwe-190 | ||
| */ | ||
|
|
||
| import cpp | ||
| import semmle.code.cpp.valuenumbering.GlobalValueNumbering | ||
| import semmle.code.cpp.valuenumbering.HashCons | ||
| import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis | ||
| import semmle.code.cpp.controlflow.Guards | ||
|
|
||
| from | ||
| GuardCondition guard, Expr expr, ExprStmt exprstmt, BasicBlock block, AssignExpr assignexpr, | ||
| AddExpr addexpr, SubExpr subexpr | ||
| where | ||
| (guard.ensuresLt(expr, addexpr, 0, block, _) or guard.ensuresLt(addexpr, expr, 0, block, _)) and | ||
| addexpr.getUnspecifiedType() instanceof IntegralType and | ||
| exprMightOverflowPositively(addexpr) and | ||
| block.getANode() = exprstmt and | ||
| exprstmt.getExpr() = assignexpr and | ||
| assignexpr.getRValue() = subexpr and | ||
| ( | ||
| hashCons(addexpr.getLeftOperand()) = hashCons(assignexpr.getLValue()) and | ||
| globalValueNumber(addexpr.getRightOperand()) = globalValueNumber(subexpr.getRightOperand()) | ||
| or | ||
| hashCons(addexpr.getRightOperand()) = hashCons(assignexpr.getLValue()) and | ||
| globalValueNumber(addexpr.getLeftOperand()) = globalValueNumber(subexpr.getRightOperand()) | ||
| ) and | ||
| globalValueNumber(expr) = globalValueNumber(subexpr.getLeftOperand()) | ||
| select guard, | ||
| "\"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.", | ||
| addexpr, "addition" | ||
35 changes: 35 additions & 0 deletions
35
...sts/Security/CWE/CWE-190/IfStatementAdditionOverflow/IfStatementAdditionOverflow.expected
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | | ||
| | 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 | |
1 change: 1 addition & 0 deletions
1
...-tests/Security/CWE/CWE-190/IfStatementAdditionOverflow/IfStatementAdditionOverflow.qlref
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| experimental/Security/CWE/CWE-190/IfStatementAdditionOverflow.ql |
63 changes: 63 additions & 0 deletions
63
...l/test/experimental/query-tests/Security/CWE/CWE-190/IfStatementAdditionOverflow/test.cpp
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
|
|
||
| int getAnInt(); | ||
| double getADouble(); | ||
| unsigned short getAnUnsignedShort(); | ||
|
|
||
| void test() | ||
| { | ||
| int a = getAnInt(); | ||
| int b = getAnInt(); | ||
| int c = getAnInt(); | ||
| int x = getAnInt(); | ||
| int y = getAnInt(); | ||
| double d = getADouble(); | ||
| unsigned short a1 = getAnUnsignedShort(); | ||
| unsigned short b1 = getAnUnsignedShort(); | ||
| unsigned short c1 = getAnUnsignedShort(); | ||
|
|
||
| if (a+b>c) a = c-b; // BAD | ||
| if (a+b>c) { a = c-b; } // BAD | ||
| if (b+a>c) a = c-b; // BAD | ||
| if (b+a>c) { a = c-b; } // BAD | ||
| if (c>a+b) a = c-b; // BAD | ||
| if (c>a+b) { a = c-b; } // BAD | ||
| if (c>b+a) a = c-b; // BAD | ||
| if (c>b+a) { a = c-b; } // BAD | ||
|
|
||
| if (a+b>=c) a = c-b; // BAD | ||
| if (a+b>=c) { a = c-b; } // BAD | ||
| if (b+a>=c) a = c-b; // BAD | ||
| if (b+a>=c) { a = c-b; } // BAD | ||
| if (c>=a+b) a = c-b; // BAD | ||
| if (c>=a+b) { a = c-b; } // BAD | ||
| if (c>=b+a) a = c-b; // BAD | ||
| if (c>=b+a) { a = c-b; } // BAD | ||
|
|
||
| if (a+b<c) a = c-b; // BAD | ||
| if (a+b<c) { a = c-b; } // BAD | ||
| if (b+a<c) a = c-b; // BAD | ||
| if (b+a<c) { a = c-b; } // BAD | ||
| if (c<a+b) a = c-b; // BAD | ||
| if (c<a+b) { a = c-b; } // BAD | ||
| if (c<b+a) a = c-b; // BAD | ||
| if (c<b+a) { a = c-b; } // BAD | ||
|
|
||
| if (a+b<=c) a = c-b; // BAD | ||
| if (a+b<=c) { a = c-b; } // BAD | ||
| if (b+a<=c) a = c-b; // BAD | ||
| if (b+a<=c) { a = c-b; } // BAD | ||
| if (c<=a+b) a = c-b; // BAD | ||
| if (c<=a+b) { a = c-b; } // BAD | ||
| if (c<=b+a) a = c-b; // BAD | ||
| if (c<=b+a) { a = c-b; } // BAD | ||
|
|
||
| if (a+b>d) a = d-b; // BAD | ||
| if (a+(double)b>c) a = c-b; // GOOD | ||
| if (a+(-x)>c) a = c-(-y); // GOOD | ||
| if (a+b>c) { b++; a = c-b; } // GOOD | ||
| if (a+d>c) a = c-d; // GOOD | ||
| if (a1+b1>c1) a1 = c1-b1; // GOOD | ||
|
|
||
| if (a+b<=c) { /* ... */ } else { a = c-b; } // BAD | ||
| if (a+b<=c) { return; } a = c-b; // BAD | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like you lost your
not isFromMacroDefinition(relop) andcondition in 5c6fc2f. Was that intentional?