Skip to content

Commit df8c399

Browse files
MathiasVPMathiasVP
authored
Merge pull request github#6710 from ihsinme/ihsinme-patch-70
CPP: Add query for CWE-1041 Use of Redundant Code
2 parents 0629ce0 + 4334acb commit df8c399

File tree

6 files changed

+211
-0
lines changed

6 files changed

+211
-0
lines changed

‎cpp/ql/src/experimental/Security/CWE/CWE-1041/FindWrapperFunctions.cpp

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
...
2+
int myFclose(FILE * fmy)
3+
{
4+
if(!fclose(fmy)) {
5+
fmy = NULL;
6+
return 0;
7+
}
8+
return -1;
9+
}
10+
...
11+
fe = fopen("myFile.txt", "wt");
12+
...
13+
fclose(fe); // BAD
14+
...
15+
fe = fopen("myFile.txt", "wt");
16+
...
17+
myFclose(fe); // GOOD
18+
...

‎cpp/ql/src/experimental/Security/CWE/CWE-1041/FindWrapperFunctions.qhelp

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>The presence of a shell function with additional check indicates the possible risks of the call. Use this check everywhere.</p>
7+
8+
</overview>
9+
10+
<example>
11+
<p>The following example demonstrates fallacious and fixed methods of using wrapper functions.</p>
12+
<sample src="FindWrapperFunctions.cpp" />
13+
14+
</example>
15+
<references>
16+
17+
<li>
18+
CERT C Coding Standard:
19+
<a href="https://wiki.sei.cmu.edu/confluence/display/java/JNI00-J.+Define+wrappers+around+native+methods">JNI00-J. Define wrappers around native methods</a>.
20+
</li>
21+
22+
</references>
23+
</qhelp>

‎cpp/ql/src/experimental/Security/CWE/CWE-1041/FindWrapperFunctions.ql

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
/**
2+
* @name Missed opportunity to call wrapper function
3+
* @description If a wrapper function is defined for a given function, any call to the given function should be via the wrapper function.
4+
* @kind problem
5+
* @id cpp/call-to-function-without-wrapper
6+
* @problem.severity warning
7+
* @precision medium
8+
* @tags correctness
9+
* maintainability
10+
* security
11+
* external/cwe/cwe-1041
12+
*/
13+
14+
import cpp
15+
import semmle.code.cpp.valuenumbering.GlobalValueNumbering
16+
import semmle.code.cpp.commons.Assertions
17+
18+
/**
19+
* A function call that is used in error situations (logging, throwing an exception, abnormal termination).
20+
*/
21+
class CallUsedToHandleErrors extends FunctionCall {
22+
CallUsedToHandleErrors() {
23+
// call that is known to not return
24+
not exists(this.(ControlFlowNode).getASuccessor())
25+
or
26+
// call throwing an exception
27+
exists(ThrowExpr tex | tex = this.(ControlFlowNode).getASuccessor())
28+
or
29+
// call logging a message, possibly an error
30+
exists(FormattingFunction ff | ff = this.(ControlFlowNode).getASuccessor())
31+
or
32+
// enabling recursive search
33+
exists(CallUsedToHandleErrors fr | getTarget() = fr.getEnclosingFunction())
34+
}
35+
}
36+
37+
/** Holds if the conditions for a call outside the wrapper function are met. */
38+
predicate conditionsOutsideWrapper(FunctionCall fcp) {
39+
fcp.getNumberOfArguments() > 0 and
40+
not exists(ConditionalStmt cdtmp | fcp.getEnclosingStmt().getParentStmt*() = cdtmp) and
41+
not exists(Loop lptmp | fcp.getEnclosingStmt().getParentStmt*() = lptmp) and
42+
not exists(ReturnStmt rttmp | fcp.getEnclosingStmt().getParentStmt*() = rttmp) and
43+
not exists(FunctionCall fctmp2 | fcp = fctmp2.getAnArgument().getAChild*()) and
44+
not exists(Assignment astmp | fcp = astmp.getRValue().getAChild*()) and
45+
not exists(Initializer intmp | fcp = intmp.getExpr().getAChild*()) and
46+
not exists(Assertion astmp | fcp = astmp.getAsserted().getAChild*()) and
47+
not exists(Operation optmp | fcp = optmp.getAChild*()) and
48+
not exists(ArrayExpr aetmp | fcp = aetmp.getAChild*()) and
49+
not exists(ExprCall ectmp | fcp = ectmp.getAnArgument().getAChild*())
50+
}
51+
52+
/** Holds if the conditions for calling `fcp` inside the `fnp` wrapper function are met. */
53+
pragma[inline]
54+
predicate conditionsInsideWrapper(FunctionCall fcp, Function fnp) {
55+
not exists(FunctionCall fctmp2 |
56+
fctmp2.getEnclosingFunction() = fnp and fcp = fctmp2.getAnArgument().getAChild*()
57+
) and
58+
not fcp instanceof CallUsedToHandleErrors and
59+
not fcp.getAnArgument().isConstant() and
60+
fcp.getEnclosingFunction() = fnp and
61+
fnp.getNumberOfParameters() > 0 and
62+
// the call arguments must be passed through the arguments of the wrapper function
63+
forall(int i | i in [0 .. fcp.getNumberOfArguments() - 1] |
64+
globalValueNumber(fcp.getArgument(i)) = globalValueNumber(fnp.getAParameter().getAnAccess())
65+
) and
66+
// there should be no more than one required call inside the wrapper function
67+
not exists(FunctionCall fctmp |
68+
fctmp.getTarget() = fcp.getTarget() and
69+
fctmp.getFile() = fcp.getFile() and
70+
fctmp != fcp and
71+
fctmp.getEnclosingFunction() = fnp
72+
) and
73+
// inside the wrapper function there should be no calls without paths to the desired function
74+
not exists(FunctionCall fctmp |
75+
fctmp.getEnclosingFunction() = fnp and
76+
fctmp.getFile() = fcp.getFile() and
77+
fctmp != fcp and
78+
(
79+
fctmp = fcp.getAPredecessor+()
80+
or
81+
not exists(FunctionCall fctmp1 |
82+
fctmp1 = fcp and
83+
(
84+
fctmp.getASuccessor+() = fctmp1 or
85+
fctmp.getAPredecessor+() = fctmp1
86+
)
87+
)
88+
)
89+
)
90+
}
91+
92+
/** Holds if the conditions for the wrapper function are met. */
93+
pragma[inline]
94+
predicate conditionsForWrapper(FunctionCall fcp, Function fnp) {
95+
not exists(ExprCall ectmp | fnp = ectmp.getEnclosingFunction()) and
96+
not exists(Loop lp | lp.getEnclosingFunction() = fnp) and
97+
not exists(SwitchStmt sw | sw.getEnclosingFunction() = fnp) and
98+
not fnp instanceof Operator and
99+
// inside the wrapper function there should be checks of arguments or the result,
100+
// perhaps by means of passing the latter as an argument to some function
101+
(
102+
exists(IfStmt ifs |
103+
ifs.getEnclosingFunction() = fnp and
104+
(
105+
globalValueNumber(ifs.getCondition().getAChild*()) = globalValueNumber(fcp.getAnArgument()) and
106+
ifs.getASuccessor*() = fcp
107+
or
108+
ifs.getCondition().getAChild() = fcp
109+
)
110+
)
111+
or
112+
exists(FunctionCall fctmp |
113+
fctmp.getEnclosingFunction() = fnp and
114+
globalValueNumber(fctmp.getAnArgument().getAChild*()) = globalValueNumber(fcp)
115+
)
116+
) and
117+
// inside the wrapper function there must be a function call to handle the error
118+
exists(CallUsedToHandleErrors fctmp |
119+
fctmp.getEnclosingFunction() = fnp and
120+
forall(int i | i in [0 .. fnp.getNumberOfParameters() - 1] |
121+
fnp.getParameter(i).getAnAccess().getTarget() =
122+
fcp.getAnArgument().(VariableAccess).getTarget() or
123+
fnp.getParameter(i).getUnspecifiedType() instanceof Class or
124+
fnp.getParameter(i).getUnspecifiedType().(ReferenceType).getBaseType() instanceof Class or
125+
fnp.getParameter(i).getAnAccess().getTarget() =
126+
fctmp.getAnArgument().(VariableAccess).getTarget()
127+
)
128+
)
129+
}
130+
131+
from FunctionCall fc, Function fn
132+
where
133+
exists(FunctionCall fctmp |
134+
conditionsInsideWrapper(fctmp, fn) and
135+
conditionsForWrapper(fctmp, fn) and
136+
conditionsOutsideWrapper(fc) and
137+
fctmp.getTarget() = fc.getTarget() and
138+
fc.getEnclosingFunction() != fn and
139+
fc.getEnclosingFunction().getMetrics().getNumberOfCalls() > fn.getMetrics().getNumberOfCalls()
140+
)
141+
select fc, "Consider changing the call to $@", fn, fn.getName()

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-1041/semmle/tests/FindWrapperFunctions.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
| test.cpp:23:3:23:8 | call to fclose | Consider changing the call to $@ | test.cpp:9:6:9:13 | myFclose | myFclose |

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-1041/semmle/tests/FindWrapperFunctions.qlref

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

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
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+
extern int printf(const char *fmt, ...);
7+
void exit(int status);
8+
9+
void myFclose(FILE * fmy)
10+
{
11+
int i;
12+
if(fmy) {
13+
i = fclose(fmy);
14+
fmy = NULL;
15+
printf("close end is code %d",i);
16+
if(i!=0) exit(1);
17+
}
18+
}
19+
20+
int main(int argc, char *argv[])
21+
{
22+
fe = fopen("myFile.txt", "wt");
23+
fclose(fe); // BAD
24+
fe = fopen("myFile.txt", "wt");
25+
myFclose(fe); // GOOD
26+
return 0;
27+
}

0 commit comments

Comments
 (0)