Skip to content

Commit c4714b5

Browse files
geoffw0geoffw0
authored
Merge pull request #6588 from ihsinme/ihsinme-patch-069
CPP: Add query for CWE-675: Duplicate Operations on Resource
2 parents 65f4ec4 + 8fa3cef commit c4714b5

File tree

6 files changed

+268
-0
lines changed

6 files changed

+268
-0
lines changed

‎cpp/ql/src/experimental/Security/CWE/CWE-675/DoubleRelease.c

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
...
2+
fs = socket(AF_UNIX, SOCK_STREAM, 0)
3+
...
4+
close(fs);
5+
fs = -1; // GOOD
6+
...
7+
8+
...
9+
fs = socket(AF_UNIX, SOCK_STREAM, 0)
10+
...
11+
close(fs);
12+
if(fs) close(fs); // BAD
13+
...

‎cpp/ql/src/experimental/Security/CWE/CWE-675/DoubleRelease.qhelp

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>Double release of the descriptor can lead to a crash of the program. Requires the attention of developers.</p>
7+
8+
</overview>
9+
<recommendation>
10+
<p>We recommend that you exclude situations of possible double release.</p>
11+
12+
</recommendation>
13+
<example>
14+
<p>The following example demonstrates an erroneous and corrected use of descriptor deallocation.</p>
15+
<sample src="DoubleRelease.c" />
16+
17+
</example>
18+
<references>
19+
20+
<li>
21+
CERT C Coding Standard:
22+
<a href="https://wiki.sei.cmu.edu/confluence/display/c/FIO46-C.+Do+not+access+a+closed+file">FIO46-C. Do not access a closed file</a>.
23+
</li>
24+
25+
</references>
26+
</qhelp>

‎cpp/ql/src/experimental/Security/CWE/CWE-675/DoubleRelease.ql

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
/**
2+
* @name Errors When Double Release
3+
* @description Double release of the descriptor can lead to a crash of the program.
4+
* @kind problem
5+
* @id cpp/double-release
6+
* @problem.severity warning
7+
* @precision medium
8+
* @tags security
9+
* external/cwe/cwe-675
10+
* external/cwe/cwe-666
11+
*/
12+
13+
import cpp
14+
import semmle.code.cpp.commons.File
15+
import semmle.code.cpp.valuenumbering.GlobalValueNumbering
16+
import semmle.code.cpp.valuenumbering.HashCons
17+
18+
/**
19+
* A function call that potentially does not return (such as `exit`).
20+
*/
21+
class CallMayNotReturn extends FunctionCall {
22+
CallMayNotReturn() {
23+
// call that is known to not return
24+
not exists(this.(ControlFlowNode).getASuccessor())
25+
or
26+
// call to another function that may not return
27+
exists(CallMayNotReturn exit | getTarget() = exit.getEnclosingFunction())
28+
or
29+
exists(ThrowExpr tex | tex = this.(ControlFlowNode).getASuccessor())
30+
}
31+
}
32+
33+
/** Holds if there are no assignment expressions to the function argument. */
34+
pragma[inline]
35+
predicate checkChangeVariable(FunctionCall fc0, ControlFlowNode fc1, ControlFlowNode fc2) {
36+
not exists(Expr exptmp |
37+
(
38+
exptmp = fc0.getArgument(0).(VariableAccess).getTarget().getAnAssignedValue() or
39+
exptmp.(AddressOfExpr).getOperand() =
40+
fc0.getArgument(0).(VariableAccess).getTarget().getAnAccess()
41+
) and
42+
exptmp = fc1.getASuccessor*() and
43+
exptmp = fc2.getAPredecessor*()
44+
) and
45+
(
46+
(
47+
not fc0.getArgument(0) instanceof PointerFieldAccess and
48+
not fc0.getArgument(0) instanceof ValueFieldAccess
49+
or
50+
fc0.getArgument(0).(VariableAccess).getQualifier() instanceof ThisExpr
51+
)
52+
or
53+
not exists(Expr exptmp |
54+
(
55+
exptmp =
56+
fc0.getArgument(0)
57+
.(VariableAccess)
58+
.getQualifier()
59+
.(VariableAccess)
60+
.getTarget()
61+
.getAnAssignedValue() or
62+
exptmp.(AddressOfExpr).getOperand() =
63+
fc0.getArgument(0)
64+
.(VariableAccess)
65+
.getQualifier()
66+
.(VariableAccess)
67+
.getTarget()
68+
.getAnAccess()
69+
) and
70+
exptmp = fc1.getASuccessor*() and
71+
exptmp = fc2.getAPredecessor*()
72+
)
73+
)
74+
}
75+
76+
/** Holds if the underlying expression is a call to the close function. Provided that the function parameter does not change after the call. */
77+
predicate closeReturn(FunctionCall fc) {
78+
fcloseCall(fc, _) and
79+
checkChangeVariable(fc, fc, fc.getEnclosingFunction())
80+
}
81+
82+
/** Holds if the underlying expression is a call to the close function. Provided that the function parameter does not change before the call. */
83+
predicate closeWithoutChangeBefore(FunctionCall fc) {
84+
fcloseCall(fc, _) and
85+
checkChangeVariable(fc, fc.getEnclosingFunction().getEntryPoint(), fc)
86+
}
87+
88+
/** Holds, if a sequential call of the specified functions is possible, via a higher-level function call. */
89+
predicate callInOtherFunctions(FunctionCall fc, FunctionCall fc1) {
90+
exists(FunctionCall fec1, FunctionCall fec2 |
91+
fc.getEnclosingFunction() != fc1.getEnclosingFunction() and
92+
fec1 = fc.getEnclosingFunction().getACallToThisFunction() and
93+
fec2 = fc1.getEnclosingFunction().getACallToThisFunction() and
94+
fec1.getASuccessor*() = fec2 and
95+
checkChangeVariable(fc, fec1, fec2)
96+
)
97+
}
98+
99+
/** Holds if successive calls to close functions are possible. */
100+
predicate interDoubleCloseFunctions(FunctionCall fc, FunctionCall fc1) {
101+
fcloseCall(fc, _) and
102+
fcloseCall(fc1, _) and
103+
fc != fc1 and
104+
fc.getASuccessor*() = fc1 and
105+
checkChangeVariable(fc, fc, fc1)
106+
}
107+
108+
/** Holds if the first arguments of the two functions are similar. */
109+
predicate similarArguments(FunctionCall fc, FunctionCall fc1) {
110+
globalValueNumber(fc.getArgument(0)) = globalValueNumber(fc1.getArgument(0))
111+
or
112+
fc.getArgument(0).(VariableAccess).getTarget() = fc1.getArgument(0).(VariableAccess).getTarget() and
113+
(
114+
not fc.getArgument(0) instanceof PointerFieldAccess and
115+
not fc.getArgument(0) instanceof ValueFieldAccess
116+
or
117+
fc.getArgument(0).(VariableAccess).getQualifier() instanceof ThisExpr
118+
)
119+
or
120+
fc.getArgument(0).(VariableAccess).getTarget() = fc1.getArgument(0).(VariableAccess).getTarget() and
121+
(
122+
fc.getArgument(0) instanceof PointerFieldAccess or
123+
fc.getArgument(0) instanceof ValueFieldAccess
124+
) and
125+
hashCons(fc.getArgument(0)) = hashCons(fc1.getArgument(0))
126+
}
127+
128+
from FunctionCall fc, FunctionCall fc1
129+
where
130+
not exists(CallMayNotReturn fctmp | fctmp = fc.getASuccessor*()) and
131+
not exists(IfStmt ifs | ifs.getCondition().getAChild*() = fc) and
132+
(
133+
// detecting a repeated call situation within one function
134+
closeReturn(fc) and
135+
closeWithoutChangeBefore(fc1) and
136+
callInOtherFunctions(fc, fc1)
137+
or
138+
// detection of repeated call in different functions
139+
interDoubleCloseFunctions(fc, fc1)
140+
) and
141+
similarArguments(fc, fc1)
142+
select fc, "Second call to the $@ function is possible.", fc1, fc1.getTarget().getName()

‎cpp/ql/test/query-tests/Security/CWE/CWE-675/semmle/tests/DoubleRelease.expected

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
| test.cpp:20:3:20:8 | call to fclose | Second call to the $@ function is possible. | test.cpp:21:3:21:8 | call to fclose | fclose |
2+
| test.cpp:31:3:31:8 | call to fclose | Second call to the $@ function is possible. | test.cpp:32:3:32:8 | call to fclose | fclose |
3+
| test.cpp:38:3:38:8 | call to fclose | Second call to the $@ function is possible. | test.cpp:44:3:44:8 | call to fclose | fclose |

‎cpp/ql/test/query-tests/Security/CWE/CWE-675/semmle/tests/DoubleRelease.qlref

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

‎cpp/ql/test/query-tests/Security/CWE/CWE-675/semmle/tests/test.cpp

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
#define NULL (0)
2+
typedef int FILE;
3+
FILE *fopen(const char *filename, const char *mode);
4+
int fclose(FILE *stream);
5+
extern FILE * fe;
6+
void test1()
7+
{
8+
FILE *f;
9+
10+
f = fopen("myFile.txt", "wt");
11+
fclose(f); // GOOD
12+
f = NULL;
13+
}
14+
15+
void test2()
16+
{
17+
FILE *f;
18+
19+
f = fopen("myFile.txt", "wt");
20+
fclose(f); // BAD
21+
fclose(f);
22+
}
23+
24+
void test3()
25+
{
26+
FILE *f;
27+
FILE *g;
28+
29+
f = fopen("myFile.txt", "wt");
30+
g = f;
31+
fclose(f); // BAD
32+
fclose(g);
33+
}
34+
35+
int fGtest4_1()
36+
{
37+
fe = fopen("myFile.txt", "wt");
38+
fclose(fe); // BAD
39+
return -1;
40+
}
41+
42+
int fGtest4_2()
43+
{
44+
fclose(fe);
45+
return -1;
46+
}
47+
48+
void Gtest4()
49+
{
50+
fGtest4_1();
51+
fGtest4_2();
52+
}
53+
54+
int fGtest5_1()
55+
{
56+
fe = fopen("myFile.txt", "wt");
57+
fclose(fe); // GOOD
58+
fe = NULL;
59+
return -1;
60+
}
61+
62+
int fGtest5_2()
63+
{
64+
fclose(fe);
65+
return -1;
66+
}
67+
68+
void Gtest5()
69+
{
70+
fGtest5_1();
71+
fGtest5_2();
72+
}
73+
74+
int main(int argc, char *argv[])
75+
{
76+
test1();
77+
test2();
78+
test3();
79+
80+
Gtest4();
81+
Gtest5();
82+
return 0;
83+
}

0 commit comments

Comments
 (0)