Skip to content

Commit ba33508

Browse files
geoffw0geoffw0
authored
Merge pull request #5601 from ihsinme/ihsinme-patch-259
CPP: Add query for CWE-691 Insufficient Control Flow Management After Refactoring The Code
2 parents 30d7f0d + c2d97b9 commit ba33508

6 files changed

+176
-3
lines changed

‎cpp/ql/src/experimental/Security/CWE/CWE-691/InsufficientControlFlowManagementAfterRefactoringTheCode.c

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
while(flagsLoop)
2+
{
3+
...
4+
if(flagsIf) break;
5+
...
6+
}while(flagsLoop); // BAD: when exiting through `break`, it is possible to get into an eternal loop.
7+
...
8+
while(flagsLoop)
9+
{
10+
...
11+
if(flagsIf) break;
12+
...
13+
} // GOOD: correct cycle
14+
...
15+
if(intA+intB) return 1; // BAD: possibly no comparison
16+
...
17+
if(intA+intB>intC) return 1; // GOOD: correct comparison

‎cpp/ql/src/experimental/Security/CWE/CWE-691/InsufficientControlFlowManagementAfterRefactoringTheCode.qhelp

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>In some situations, after code refactoring, parts of the old constructs may remain. They are correctly accepted by the compiler, but can critically affect program execution. For example, if you switch from `do {...} while ();` to `while () {...}` forgetting to remove the old construct completely, you get `while(){...}while();` which may be vulnerable. These code snippets look suspicious and require the developer's attention.</p>
7+
8+
9+
</overview>
10+
<recommendation>
11+
12+
<p>We recommend that you use more explicit code transformations.</p>
13+
14+
</recommendation>
15+
<example>
16+
<p>The following example demonstrates the erroneous and corrected sections of the code.</p>
17+
<sample src="InsufficientControlFlowManagementAfterRefactoringTheCode.c" />
18+
19+
</example>
20+
<references>
21+
22+
<li>
23+
CWE Common Weakness Enumeration:
24+
<a href="https://cwe.mitre.org/data/definitions/691.html"> CWE-691: Insufficient Control Flow Management</a>.
25+
</li>
26+
27+
</references>
28+
</qhelp>

‎cpp/ql/src/experimental/Security/CWE/CWE-691/InsufficientControlFlowManagementAfterRefactoringTheCode.ql

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
/**
2+
* @name Errors After Refactoring
3+
* @description --In some situations, after code refactoring, parts of the old constructs may remain.
4+
* --They are correctly accepted by the compiler, but can critically affect program execution.
5+
* --For example, if you switch from `do {...} while ();` to `while () {...}` with errors, you run the risk of running out of resources.
6+
* --These code snippets look suspicious and require the developer's attention.
7+
* @kind problem
8+
* @id cpp/errors-after-refactoring
9+
* @problem.severity warning
10+
* @precision medium
11+
* @tags correctness
12+
* security
13+
* external/cwe/cwe-691
14+
*/
15+
16+
import cpp
17+
import semmle.code.cpp.valuenumbering.HashCons
18+
import semmle.code.cpp.valuenumbering.GlobalValueNumbering
19+
20+
/**
21+
* Using `while` directly after the body of another` while`.
22+
*/
23+
class UsingWhileAfterWhile extends WhileStmt {
24+
/**
25+
* Using a loop call after another loop has finished running can result in an eternal loop.
26+
* For example, perhaps as a result of refactoring, the `do ... while ()` loop was incorrectly corrected.
27+
* Even in the case of deliberate use of such an expression, it is better to correct it.
28+
*/
29+
UsingWhileAfterWhile() {
30+
exists(WhileStmt wh1 |
31+
wh1.getStmt().getAChild*().(BreakStmt).(ControlFlowNode).getASuccessor().getASuccessor() =
32+
this and
33+
hashCons(wh1.getCondition()) = hashCons(this.getCondition()) and
34+
this.getStmt() instanceof EmptyStmt
35+
)
36+
or
37+
exists(ForStmt fr1 |
38+
fr1.getStmt().getAChild*().(BreakStmt).(ControlFlowNode).getASuccessor().getASuccessor() =
39+
this and
40+
hashCons(fr1.getCondition()) = hashCons(this.getCondition()) and
41+
this.getStmt() instanceof EmptyStmt
42+
)
43+
}
44+
}
45+
46+
/**
47+
* Using arithmetic in a condition.
48+
*/
49+
class UsingArithmeticInComparison extends BinaryArithmeticOperation {
50+
/**
51+
* Using arithmetic operations in a comparison operation can be dangerous.
52+
* For example, part of the comparison may have been lost as a result of refactoring.
53+
* Even if you deliberately use such an expression, it is better to add an explicit comparison.
54+
*/
55+
UsingArithmeticInComparison() {
56+
this.getParent*() instanceof IfStmt and
57+
not this.getAChild*().isConstant() and
58+
not this.getParent*() instanceof Call and
59+
not this.getParent*() instanceof AssignExpr and
60+
not this.getParent*() instanceof ArrayExpr and
61+
not this.getParent*() instanceof RemExpr and
62+
not this.getParent*() instanceof AssignBitwiseOperation and
63+
not this.getParent*() instanceof AssignArithmeticOperation and
64+
not this.getParent*() instanceof EqualityOperation and
65+
not this.getParent*() instanceof RelationalOperation
66+
}
67+
68+
/** Holds when the expression is inside the loop body. */
69+
predicate insideTheLoop() { exists(Loop lp | lp.getStmt().getAChild*() = this.getParent*()) }
70+
71+
/** Holds when the expression is used in binary operations. */
72+
predicate workingWithValue() {
73+
this.getParent*() instanceof BinaryBitwiseOperation or
74+
this.getParent*() instanceof NotExpr
75+
}
76+
77+
/** Holds when the expression contains a pointer. */
78+
predicate workingWithPointer() {
79+
this.getAChild*().getFullyConverted().getType() instanceof DerivedType
80+
}
81+
82+
/** Holds when a null comparison expression exists. */
83+
predicate compareWithZero() {
84+
exists(Expr exp |
85+
exp instanceof ComparisonOperation and
86+
(
87+
globalValueNumber(exp.getAChild*()) = globalValueNumber(this) or
88+
hashCons(exp.getAChild*()) = hashCons(this)
89+
) and
90+
(
91+
exp.(ComparisonOperation).getLeftOperand().getValue() = "0" or
92+
exp.(ComparisonOperation).getRightOperand().getValue() = "0"
93+
)
94+
)
95+
}
96+
97+
/** Holds when a comparison expression exists. */
98+
predicate compareWithOutZero() {
99+
exists(Expr exp |
100+
exp instanceof ComparisonOperation and
101+
(
102+
globalValueNumber(exp.getAChild*()) = globalValueNumber(this) or
103+
hashCons(exp.getAChild*()) = hashCons(this)
104+
)
105+
)
106+
}
107+
}
108+
109+
from Expr exp
110+
where
111+
exp instanceof UsingArithmeticInComparison and
112+
not exp.(UsingArithmeticInComparison).workingWithValue() and
113+
not exp.(UsingArithmeticInComparison).workingWithPointer() and
114+
not exp.(UsingArithmeticInComparison).insideTheLoop() and
115+
not exp.(UsingArithmeticInComparison).compareWithZero() and
116+
exp.(UsingArithmeticInComparison).compareWithOutZero()
117+
or
118+
exists(WhileStmt wst | wst instanceof UsingWhileAfterWhile and exp = wst.getCondition())
119+
select exp, "this expression needs your attention"

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-691/semmle/tests/InsufficientControlFlowManagementAfterRefactoringTheCode.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
| test.c:15:6:15:16 | ... + ... | this expression needs your attention |
2+
| test.c:17:17:17:27 | ... + ... | this expression needs your attention |
3+
| test.c:22:10:22:15 | ... > ... | this expression needs your attention |
4+
| test.c:26:10:26:15 | ... > ... | this expression needs your attention |

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-691/semmle/tests/InsufficientControlFlowManagementAfterRefactoringTheCode.qlref

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

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-691/semmle/tests/test.c

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,18 @@ void workFunction_0(char *s) {
1212
void workFunction_1(char *s) {
1313
int intA,intB;
1414

15-
if(intA + intB) return; // BAD [NOT DETECTED]
15+
if(intA + intB) return; // BAD
1616
if(intA + intB>4) return; // GOOD
17-
if(intA>0 && (intA + intB)) return; // BAD [NOT DETECTED]
17+
if(intA>0 && (intA + intB)) return; // BAD
1818
while(intA>0)
1919
{
2020
if(intB - intA<10) break;
2121
intA--;
22-
}while(intA>0); // BAD [NOT DETECTED]
22+
}while(intA>0); // BAD
23+
for(intA=100; intA>0; intA--)
24+
{
25+
if(intB - intA<10) break;
26+
}while(intA>0); // BAD
2327
while(intA>0)
2428
{
2529
if(intB - intA<10) break;

0 commit comments

Comments
 (0)