Skip to content

Commit 36de496

Browse files
ihsinmeihsinme
committed
Add files via upload
1 parent 15049ca commit 36de496

File tree

3 files changed

+163
-0
lines changed

3 files changed

+163
-0
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: coreten 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 () {...}` with errors, you run the risk of running out of resources. 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: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
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 comparison.
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 RemExpr and
61+
not this.getParent*() instanceof AssignBitwiseOperation and
62+
not this.getParent*() instanceof AssignArithmeticOperation and
63+
not this.getParent*() instanceof EqualityOperation and
64+
not this.getParent*() instanceof RelationalOperation
65+
}
66+
67+
/** Holds when the expression is inside the loop body. */
68+
predicate insideTheLoop() { exists(Loop lp | lp.getStmt().getAChild*() = this.getParent*()) }
69+
70+
/** Holds when the expression is used in binary operations. */
71+
predicate workingWithValue() {
72+
this.getParent*() instanceof BinaryBitwiseOperation or
73+
this.getParent*() instanceof NotExpr
74+
}
75+
76+
/** Holds when the expression contains a pointer. */
77+
predicate workingWithPointer() {
78+
this.getAChild*().getFullyConverted().getType() instanceof DerivedType
79+
}
80+
81+
/** Holds when a null comparison expression exists. */
82+
predicate compareWithZero() {
83+
exists(Expr exp |
84+
exp instanceof ComparisonOperation and
85+
(
86+
globalValueNumber(exp.getAChild*()) = globalValueNumber(this) or
87+
hashCons(exp.getAChild*()) = hashCons(this)
88+
) and
89+
(
90+
exp.(ComparisonOperation).getLeftOperand().getValue() = "0" or
91+
exp.(ComparisonOperation).getRightOperand().getValue() = "0"
92+
)
93+
)
94+
}
95+
96+
/** Holds when a comparison expression exists. */
97+
predicate compareWithOutZero() {
98+
exists(Expr exp |
99+
exp instanceof ComparisonOperation and
100+
(
101+
globalValueNumber(exp.getAChild*()) = globalValueNumber(this) or
102+
hashCons(exp.getAChild*()) = hashCons(this)
103+
)
104+
)
105+
}
106+
}
107+
108+
from Expr exp, WhileStmt wst
109+
where
110+
exp instanceof UsingArithmeticInComparison and
111+
not exp.(UsingArithmeticInComparison).workingWithValue() and
112+
not exp.(UsingArithmeticInComparison).workingWithPointer() and
113+
not exp.(UsingArithmeticInComparison).insideTheLoop() and
114+
not exp.(UsingArithmeticInComparison).compareWithZero() and
115+
exp.(UsingArithmeticInComparison).compareWithOutZero()
116+
or
117+
wst instanceof UsingWhileAfterWhile and exp = wst.getCondition()
118+
select exp, "this expression needs your attention"

0 commit comments

Comments
 (0)