Skip to content

Commit c4322fd

Browse files
geoffw0geoffw0
authored
Merge pull request #6231 from ihsinme/ihsinme-patch-277
Add query for CWE-758: Reliance on Undefined, Unspecified, or Implementation-Defined Behavior
2 parents d282f6a + 4d36666 commit c4322fd

File tree

6 files changed

+229
-0
lines changed

6 files changed

+229
-0
lines changed

‎cpp/ql/src/experimental/Security/CWE/CWE-758/UndefinedOrImplementationDefinedBehavior.c

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
intA = ++intA + 1; // BAD: undefined behavior when changing variable `intA`
2+
...
3+
intA++;
4+
intA = intA + 1; // GOOD: correct design
5+
...
6+
char * buff;
7+
...
8+
if(funcAdd(buff)+fucDel(buff)>0) return 1; // BAD: undefined behavior when calling functions to change the `buff` variable
9+
...
10+
intA = funcAdd(buff);
11+
intB = funcDel(buff);
12+
if(intA+intB>0) return 1; // GOOD: correct design

‎cpp/ql/src/experimental/Security/CWE/CWE-758/UndefinedOrImplementationDefinedBehavior.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, the code constructs used may be executed in the wrong order in which the developer designed them. For example, if you call multiple functions as part of a single expression, and the functions have the ability to modify a shared resource, then the sequence in which the resource is changed can be unpredictable. 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 guaranteed, in terms of sequence of execution, coding techniques.</p>
13+
14+
</recommendation>
15+
<example>
16+
<p>The following example demonstrates sections of code with insufficient execution sequence definition.</p>
17+
<sample src="UndefinedOrImplementationDefinedBehavior.c" />
18+
19+
</example>
20+
<references>
21+
22+
<li>
23+
CWE Common Weakness Enumeration:
24+
<a href="https://wiki.sei.cmu.edu/confluence/display/c/EXP10-C.+Do+not+depend+on+the+order+of+evaluation+of+subexpressions+or+the+order+in+which+side+effects+take+place"> EXP10-C. Do not depend on the order of evaluation of subexpressions or the order in which side effects take place</a>.
25+
</li>
26+
27+
</references>
28+
</qhelp>

‎cpp/ql/src/experimental/Security/CWE/CWE-758/UndefinedOrImplementationDefinedBehavior.ql

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
/**
2+
* @name Errors Of Undefined Program Behavior
3+
* @description --In some situations, the code constructs used may be executed in the wrong order in which the developer designed them.
4+
* --For example, if you call multiple functions as part of a single expression, and the functions have the ability to modify a shared resource, then the sequence in which the resource is changed can be unpredictable.
5+
* --These code snippets look suspicious and require the developer's attention.
6+
* @kind problem
7+
* @id cpp/errors-of-undefined-program-behavior
8+
* @problem.severity warning
9+
* @precision medium
10+
* @tags security
11+
* external/cwe/cwe-758
12+
*/
13+
14+
import cpp
15+
import semmle.code.cpp.valuenumbering.HashCons
16+
import semmle.code.cpp.valuenumbering.GlobalValueNumbering
17+
18+
/**
19+
* Threatening expressions of undefined behavior.
20+
*/
21+
class ExpressionsOfTheSameLevel extends Expr {
22+
Expr exp2;
23+
24+
ExpressionsOfTheSameLevel() {
25+
this != exp2 and
26+
this.getParent() = exp2.getParent()
27+
}
28+
29+
/** Holds if the underlying expression is a function call. */
30+
predicate expressionCall() {
31+
this instanceof FunctionCall and
32+
exp2.getAChild*() instanceof FunctionCall and
33+
not this.getParent() instanceof Operator and
34+
not this.(FunctionCall).hasQualifier()
35+
}
36+
37+
/** Holds if the underlying expression is a call to a function to free resources. */
38+
predicate existsCloseOrFreeCall() {
39+
(
40+
globalValueNumber(this.(FunctionCall).getAnArgument()) =
41+
globalValueNumber(exp2.getAChild*().(FunctionCall).getAnArgument()) or
42+
hashCons(this.(FunctionCall).getAnArgument()) =
43+
hashCons(exp2.getAChild*().(FunctionCall).getAnArgument())
44+
) and
45+
(
46+
this.(FunctionCall).getTarget().hasGlobalOrStdName("close") or
47+
this.(FunctionCall).getTarget().hasGlobalOrStdName("free") or
48+
this.(FunctionCall).getTarget().hasGlobalOrStdName("fclose")
49+
)
50+
}
51+
52+
/** Holds if the arguments in the function can be changed. */
53+
predicate generalArgumentDerivedType() {
54+
exists(Parameter prt1, Parameter prt2, AssignExpr aet1, AssignExpr aet2, int i, int j |
55+
not this.(FunctionCall).getArgument(i).isConstant() and
56+
hashCons(this.(FunctionCall).getArgument(i)) =
57+
hashCons(exp2.getAChild*().(FunctionCall).getArgument(j)) and
58+
prt1 = this.(FunctionCall).getTarget().getParameter(i) and
59+
prt2 = exp2.getAChild*().(FunctionCall).getTarget().getParameter(j) and
60+
prt1.getType() instanceof DerivedType and
61+
(
62+
aet1 = this.(FunctionCall).getTarget().getEntryPoint().getASuccessor*() and
63+
(
64+
aet1.getLValue().(ArrayExpr).getArrayBase().(VariableAccess).getTarget() =
65+
prt1.getAnAccess().getTarget() or
66+
aet1.getLValue().(VariableAccess).getTarget() = prt1.getAnAccess().getTarget()
67+
)
68+
or
69+
exists(FunctionCall fc1 |
70+
fc1.getTarget().hasGlobalName("memcpy") and
71+
fc1.getArgument(0).(VariableAccess).getTarget() = prt1.getAnAccess().getTarget() and
72+
fc1 = this.(FunctionCall).getTarget().getEntryPoint().getASuccessor*()
73+
)
74+
) and
75+
(
76+
aet2 = exp2.getAChild*().(FunctionCall).getTarget().getEntryPoint().getASuccessor*() and
77+
(
78+
aet2.getLValue().(ArrayExpr).getArrayBase().(VariableAccess).getTarget() =
79+
prt2.getAnAccess().getTarget() or
80+
aet2.getLValue().(VariableAccess).getTarget() = prt2.getAnAccess().getTarget()
81+
)
82+
or
83+
exists(FunctionCall fc1 |
84+
fc1.getTarget().hasGlobalName("memcpy") and
85+
fc1.getArgument(0).(VariableAccess).getTarget() = prt2.getAnAccess().getTarget() and
86+
fc1 = exp2.(FunctionCall).getTarget().getEntryPoint().getASuccessor*()
87+
)
88+
)
89+
)
90+
}
91+
92+
/** Holds if functions have a common global argument. */
93+
predicate generalGlobalArgument() {
94+
exists(Declaration dl, AssignExpr aet1, AssignExpr aet2 |
95+
dl instanceof GlobalVariable and
96+
(
97+
(
98+
aet1.getLValue().(Access).getTarget() = dl or
99+
aet1.getLValue().(ArrayExpr).getArrayBase().(VariableAccess).getTarget() = dl
100+
) and
101+
aet1 = this.(FunctionCall).getTarget().getEntryPoint().getASuccessor*() and
102+
not aet1.getRValue().isConstant()
103+
or
104+
exists(FunctionCall fc1 |
105+
fc1.getTarget().hasGlobalName("memcpy") and
106+
fc1.getArgument(0).(VariableAccess).getTarget() = dl and
107+
fc1 = this.(FunctionCall).getTarget().getEntryPoint().getASuccessor*()
108+
)
109+
) and
110+
(
111+
(
112+
aet2.getLValue().(Access).getTarget() = dl or
113+
aet2.getLValue().(ArrayExpr).getArrayBase().(VariableAccess).getTarget() = dl
114+
) and
115+
aet2 = exp2.(FunctionCall).getTarget().getEntryPoint().getASuccessor*()
116+
or
117+
exists(FunctionCall fc1 |
118+
fc1.getTarget().hasGlobalName("memcpy") and
119+
fc1.getArgument(0).(VariableAccess).getTarget() = dl and
120+
fc1 = exp2.(FunctionCall).getTarget().getEntryPoint().getASuccessor*()
121+
)
122+
)
123+
)
124+
}
125+
126+
/** Holds if sequence point is not present in expression. */
127+
predicate orderOfActionExpressions() {
128+
not this.getParent() instanceof BinaryLogicalOperation and
129+
not this.getParent() instanceof ConditionalExpr and
130+
not this.getParent() instanceof Loop and
131+
not this.getParent() instanceof CommaExpr
132+
}
133+
134+
/** Holds if expression is crement. */
135+
predicate dangerousCrementChanges() {
136+
hashCons(this.(CrementOperation).getOperand()) = hashCons(exp2.(CrementOperation).getOperand())
137+
or
138+
hashCons(this.(CrementOperation).getOperand()) = hashCons(exp2)
139+
or
140+
hashCons(this.(CrementOperation).getOperand()) = hashCons(exp2.(ArrayExpr).getArrayOffset())
141+
or
142+
hashCons(this.(Assignment).getLValue()) = hashCons(exp2.(Assignment).getLValue())
143+
or
144+
not this.getAChild*() instanceof Call and
145+
(
146+
hashCons(this.getAChild*().(CrementOperation).getOperand()) = hashCons(exp2) or
147+
hashCons(this.getAChild*().(CrementOperation).getOperand()) =
148+
hashCons(exp2.(Assignment).getLValue())
149+
)
150+
}
151+
}
152+
153+
from ExpressionsOfTheSameLevel eots
154+
where
155+
eots.orderOfActionExpressions() and
156+
(
157+
eots.expressionCall() and
158+
(
159+
eots.generalArgumentDerivedType() or
160+
eots.generalGlobalArgument() or
161+
eots.existsCloseOrFreeCall()
162+
)
163+
or
164+
eots.dangerousCrementChanges()
165+
)
166+
select eots, "This expression may have undefined behavior."

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-758/semmle/tests/UndefinedOrImplementationDefinedBehavior.expected

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
| test.c:13:10:13:21 | call to tmpFunction1 | This expression may have undefined behavior. |
2+
| test.c:13:30:13:41 | call to tmpFunction2 | This expression may have undefined behavior. |
3+
| test.c:16:15:16:20 | ... ++ | This expression may have undefined behavior. |

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-758/semmle/tests/UndefinedOrImplementationDefinedBehavior.qlref

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

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
char tmpFunction1(char * buf)
2+
{
3+
buf[1]=buf[1] + buf[2] + buf[3];
4+
return buf[1];
5+
}
6+
char tmpFunction2(char * buf)
7+
{
8+
buf[2]=buf[1] + buf[2] + buf[3];
9+
return buf[2];
10+
}
11+
void workFunction_0(char *s, char * buf) {
12+
int intA;
13+
intA = tmpFunction1(buf) + tmpFunction2(buf); // BAD
14+
intA = tmpFunction1(buf); //GOOD
15+
intA += tmpFunction2(buf); // GOOD
16+
buf[intA] = intA++; // BAD
17+
intA++;
18+
buf[intA] = intA; // GOOD
19+
}

0 commit comments

Comments
 (0)