Skip to content

Commit 5a9e27c

Browse files
RasmusWLRasmusWL
committed
Merge branch 'main' into django-3.2
2 parents be9cbd7 + 94f0a15 commit 5a9e27c

File tree

95 files changed

+1193
-1399
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

95 files changed

+1193
-1399
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;

‎csharp/ql/src/Metrics/Summaries/LinesOfCode.ql

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
/**
2+
* @id cs/summary/lines-of-code
3+
* @name Total lines of code in the database
4+
* @description The total number of lines of code across all files. This is a useful metric of the size of a database. For all files that were seen during the build, this query counts the lines of code, excluding whitespace or comments.
5+
* @kind metric
6+
* @tags summary
7+
*/
8+
9+
import csharp
10+
11+
select sum(File f | f.fromSource() | f.getNumberOfLinesOfCode())

‎csharp/ql/src/semmle/code/cil/internal/SsaImplCommon.qll

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -316,6 +316,15 @@ private module SsaDefReaches {
316316
)
317317
}
318318

319+
/**
320+
* Holds if the reference to `def` at index `i` in basic block `bb` is the
321+
* last reference to `v` inside `bb`.
322+
*/
323+
pragma[noinline]
324+
predicate lastSsaRef(Definition def, SourceVariable v, BasicBlock bb, int i) {
325+
ssaDefRank(def, v, bb, i, _) = maxSsaRefRank(bb, v)
326+
}
327+
319328
predicate defOccursInBlock(Definition def, BasicBlock bb, SourceVariable v) {
320329
exists(ssaDefRank(def, v, bb, _, _))
321330
}
@@ -351,8 +360,7 @@ private module SsaDefReaches {
351360
*/
352361
predicate defAdjacentRead(Definition def, BasicBlock bb1, BasicBlock bb2, int i2) {
353362
varBlockReaches(def, bb1, bb2) and
354-
ssaRefRank(bb2, i2, def.getSourceVariable(), SsaRead()) = 1 and
355-
variableRead(bb2, i2, _, _)
363+
ssaRefRank(bb2, i2, def.getSourceVariable(), SsaRead()) = 1
356364
}
357365
}
358366

@@ -434,15 +442,22 @@ predicate adjacentDefRead(Definition def, BasicBlock bb1, int i1, BasicBlock bb2
434442
bb2 = bb1
435443
)
436444
or
437-
exists(SourceVariable v | ssaDefRank(def, v, bb1, i1, _) = maxSsaRefRank(bb1, v)) and
445+
lastSsaRef(def, _, bb1, i1) and
438446
defAdjacentRead(def, bb1, bb2, i2)
439447
}
440448

449+
pragma[noinline]
450+
private predicate adjacentDefRead(
451+
Definition def, BasicBlock bb1, int i1, BasicBlock bb2, int i2, SourceVariable v
452+
) {
453+
adjacentDefRead(def, bb1, i1, bb2, i2) and
454+
v = def.getSourceVariable()
455+
}
456+
441457
private predicate adjacentDefReachesRead(
442458
Definition def, BasicBlock bb1, int i1, BasicBlock bb2, int i2
443459
) {
444-
adjacentDefRead(def, bb1, i1, bb2, i2) and
445-
exists(SourceVariable v | v = def.getSourceVariable() |
460+
exists(SourceVariable v | adjacentDefRead(def, bb1, i1, bb2, i2, v) |
446461
ssaRef(bb1, i1, v, SsaDef())
447462
or
448463
variableRead(bb1, i1, v, true)
@@ -475,17 +490,19 @@ predicate adjacentDefNoUncertainReads(Definition def, BasicBlock bb1, int i1, Ba
475490
*/
476491
pragma[nomagic]
477492
predicate lastRefRedef(Definition def, BasicBlock bb, int i, Definition next) {
478-
exists(int rnk, SourceVariable v, int j | rnk = ssaDefRank(def, v, bb, i, _) |
493+
exists(SourceVariable v |
479494
// Next reference to `v` inside `bb` is a write
480-
next.definesAt(v, bb, j) and
481-
rnk + 1 = ssaRefRank(bb, j, v, SsaDef())
495+
exists(int rnk, int j |
496+
rnk = ssaDefRank(def, v, bb, i, _) and
497+
next.definesAt(v, bb, j) and
498+
rnk + 1 = ssaRefRank(bb, j, v, SsaDef())
499+
)
482500
or
483501
// Can reach a write using one or more steps
484-
rnk = maxSsaRefRank(bb, v) and
502+
lastSsaRef(def, v, bb, i) and
485503
exists(BasicBlock bb2 |
486504
varBlockReaches(def, bb, bb2) and
487-
next.definesAt(v, bb2, j) and
488-
1 = ssaRefRank(bb2, j, v, SsaDef())
505+
1 = ssaDefRank(next, v, bb2, _, SsaDef())
489506
)
490507
)
491508
}
@@ -539,7 +556,8 @@ pragma[nomagic]
539556
predicate lastRef(Definition def, BasicBlock bb, int i) {
540557
lastRefRedef(def, bb, i, _)
541558
or
542-
exists(SourceVariable v | ssaDefRank(def, v, bb, i, _) = maxSsaRefRank(bb, v) |
559+
lastSsaRef(def, _, bb, i) and
560+
(
543561
// Can reach exit directly
544562
bb instanceof ExitBasicBlock
545563
or

‎csharp/ql/src/semmle/code/csharp/controlflow/internal/pressa/SsaImplCommon.qll

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -316,6 +316,15 @@ private module SsaDefReaches {
316316
)
317317
}
318318

319+
/**
320+
* Holds if the reference to `def` at index `i` in basic block `bb` is the
321+
* last reference to `v` inside `bb`.
322+
*/
323+
pragma[noinline]
324+
predicate lastSsaRef(Definition def, SourceVariable v, BasicBlock bb, int i) {
325+
ssaDefRank(def, v, bb, i, _) = maxSsaRefRank(bb, v)
326+
}
327+
319328
predicate defOccursInBlock(Definition def, BasicBlock bb, SourceVariable v) {
320329
exists(ssaDefRank(def, v, bb, _, _))
321330
}
@@ -351,8 +360,7 @@ private module SsaDefReaches {
351360
*/
352361
predicate defAdjacentRead(Definition def, BasicBlock bb1, BasicBlock bb2, int i2) {
353362
varBlockReaches(def, bb1, bb2) and
354-
ssaRefRank(bb2, i2, def.getSourceVariable(), SsaRead()) = 1 and
355-
variableRead(bb2, i2, _, _)
363+
ssaRefRank(bb2, i2, def.getSourceVariable(), SsaRead()) = 1
356364
}
357365
}
358366

@@ -434,15 +442,22 @@ predicate adjacentDefRead(Definition def, BasicBlock bb1, int i1, BasicBlock bb2
434442
bb2 = bb1
435443
)
436444
or
437-
exists(SourceVariable v | ssaDefRank(def, v, bb1, i1, _) = maxSsaRefRank(bb1, v)) and
445+
lastSsaRef(def, _, bb1, i1) and
438446
defAdjacentRead(def, bb1, bb2, i2)
439447
}
440448

449+
pragma[noinline]
450+
private predicate adjacentDefRead(
451+
Definition def, BasicBlock bb1, int i1, BasicBlock bb2, int i2, SourceVariable v
452+
) {
453+
adjacentDefRead(def, bb1, i1, bb2, i2) and
454+
v = def.getSourceVariable()
455+
}
456+
441457
private predicate adjacentDefReachesRead(
442458
Definition def, BasicBlock bb1, int i1, BasicBlock bb2, int i2
443459
) {
444-
adjacentDefRead(def, bb1, i1, bb2, i2) and
445-
exists(SourceVariable v | v = def.getSourceVariable() |
460+
exists(SourceVariable v | adjacentDefRead(def, bb1, i1, bb2, i2, v) |
446461
ssaRef(bb1, i1, v, SsaDef())
447462
or
448463
variableRead(bb1, i1, v, true)
@@ -475,17 +490,19 @@ predicate adjacentDefNoUncertainReads(Definition def, BasicBlock bb1, int i1, Ba
475490
*/
476491
pragma[nomagic]
477492
predicate lastRefRedef(Definition def, BasicBlock bb, int i, Definition next) {
478-
exists(int rnk, SourceVariable v, int j | rnk = ssaDefRank(def, v, bb, i, _) |
493+
exists(SourceVariable v |
479494
// Next reference to `v` inside `bb` is a write
480-
next.definesAt(v, bb, j) and
481-
rnk + 1 = ssaRefRank(bb, j, v, SsaDef())
495+
exists(int rnk, int j |
496+
rnk = ssaDefRank(def, v, bb, i, _) and
497+
next.definesAt(v, bb, j) and
498+
rnk + 1 = ssaRefRank(bb, j, v, SsaDef())
499+
)
482500
or
483501
// Can reach a write using one or more steps
484-
rnk = maxSsaRefRank(bb, v) and
502+
lastSsaRef(def, v, bb, i) and
485503
exists(BasicBlock bb2 |
486504
varBlockReaches(def, bb, bb2) and
487-
next.definesAt(v, bb2, j) and
488-
1 = ssaRefRank(bb2, j, v, SsaDef())
505+
1 = ssaDefRank(next, v, bb2, _, SsaDef())
489506
)
490507
)
491508
}
@@ -539,7 +556,8 @@ pragma[nomagic]
539556
predicate lastRef(Definition def, BasicBlock bb, int i) {
540557
lastRefRedef(def, bb, i, _)
541558
or
542-
exists(SourceVariable v | ssaDefRank(def, v, bb, i, _) = maxSsaRefRank(bb, v) |
559+
lastSsaRef(def, _, bb, i) and
560+
(
543561
// Can reach exit directly
544562
bb instanceof ExitBasicBlock
545563
or

0 commit comments

Comments
 (0)