Skip to content

Commit cb28bc8

Browse files
tamasvajktamasvajk
authored
Merge branch 'main' into feature/java-sinks-csv
2 parents 7134eb9 + 2b8afe5 commit cb28bc8

File tree

521 files changed

+13562
-6365
lines changed

Some content is hidden

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

521 files changed

+13562
-6365
lines changed

‎.github/workflows/close-stale.yml

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
name: Mark stale issues
2+
3+
on:
4+
workflow_dispatch:
5+
schedule:
6+
- cron: "30 1 * * *"
7+
8+
jobs:
9+
stale:
10+
if: github.repository == 'github/codeql'
11+
12+
runs-on: ubuntu-latest
13+
14+
steps:
15+
- uses: actions/stale@v3
16+
with:
17+
repo-token: ${{ secrets.GITHUB_TOKEN }}
18+
stale-issue-message: 'This issue is stale because it has been open 14 days with no activity. Comment or remove the `Stale` label in order to avoid having this issue closed in 7 days.'
19+
close-issue-message: 'This issue was closed because it has been inactive for 7 days.'
20+
days-before-stale: 14
21+
days-before-close: 7
22+
only-labels: awaiting-response
23+
24+
# do not mark PRs as stale
25+
days-before-pr-stale: -1
26+
days-before-pr-close: -1
27+
28+
# Uncomment for dry-run
29+
# debug-only: true
30+
# operations-per-run: 1000

‎config/identical-files.json

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,10 @@
5656
"csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowImplConsistency.qll",
5757
"python/ql/src/semmle/python/dataflow/new/internal/DataFlowImplConsistency.qll"
5858
],
59+
"DataFlow Java/C# Flow Summaries": [
60+
"java/ql/src/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll",
61+
"csharp/ql/src/semmle/code/csharp/dataflow/internal/FlowSummaryImpl.qll"
62+
],
5963
"SsaReadPosition Java/C#": [
6064
"java/ql/src/semmle/code/java/dataflow/internal/rangeanalysis/SsaReadPositionCommon.qll",
6165
"csharp/ql/src/semmle/code/csharp/dataflow/internal/rangeanalysis/SsaReadPositionCommon.qll"

‎cpp/change-notes/2021-04-13-arithmetic-queries.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm
2+
* The queries cpp/tainted-arithmetic, cpp/uncontrolled-arithmetic, and cpp/arithmetic-with-extreme-values have been improved to produce fewer false positives.

‎cpp/change-notes/2021-04-21-return-stack-allocated-object.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
codescanning
2+
* The 'Pointer to stack object used as return value' (cpp/return-stack-allocated-object) query has been deprecated, and any uses should be replaced with `Returning stack-allocated memory` (cpp/return-stack-allocated-memory).

‎cpp/ql/src/Critical/ReturnStackAllocatedObject.ql

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
* @tags reliability
88
* security
99
* external/cwe/cwe-562
10+
* @deprecated This query is not suitable for production use and has been deprecated. Use
11+
* cpp/return-stack-allocated-memory instead.
1012
*/
1113

1214
import semmle.code.cpp.pointsto.PointsTo

‎cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
import cpp
1515
import semmle.code.cpp.dataflow.EscapesTree
16+
import semmle.code.cpp.models.interfaces.PointerWrapper
1617
import semmle.code.cpp.dataflow.DataFlow
1718

1819
/**
@@ -39,6 +40,10 @@ predicate hasNontrivialConversion(Expr e) {
3940
e instanceof ParenthesisExpr
4041
)
4142
or
43+
// A smart pointer can be stack-allocated while the data it points to is heap-allocated.
44+
// So we exclude such "conversions" from this predicate.
45+
e = any(PointerWrapper wrapper).getAnUnwrapperFunction().getACallToThisFunction()
46+
or
4247
hasNontrivialConversion(e.getConversion())
4348
}
4449

‎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/src/experimental/Security/CWE/CWE-691/InsufficientControlFlowManagementWhenUsingBitOperations.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
if(len>0 & memset(buf,0,len)) return 1; // BAD: `memset` will be called regardless of the value of the `len` variable. moreover, one cannot be sure that it will happen after verification
2+
...
3+
if(len>0 && memset(buf,0,len)) return 1; // GOOD: `memset` will be called after the `len` variable has been checked.
4+
...

0 commit comments

Comments
 (0)