Skip to content

Commit a8b2805

Browse files
jketemajketema
authored
Merge pull request #8246 from ihsinme/ihsinme-patch-82
CPP: Add query for CWE-754: Improper Check for Unusual or Exceptional Conditions when using functions scanf
2 parents b1a4281 + ac8adea commit a8b2805

File tree

6 files changed

+278
-0
lines changed

6 files changed

+278
-0
lines changed

‎cpp/ql/src/experimental/Security/CWE/CWE-754/ImproperCheckReturnValueScanf.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
...
2+
r = scanf("%i", &i);
3+
if (r == 1) // GOOD
4+
return i;
5+
else
6+
return -1;
7+
...
8+
scanf("%i", &i); // BAD
9+
return i;
10+
...

‎cpp/ql/src/experimental/Security/CWE/CWE-754/ImproperCheckReturnValueScanf.qhelp

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>The `scanf` family functions does not require the memory pointed to by its additional pointer arguments to be initialized before calling. The user is required to check the return value of `scanf` and similar functions to establish how many of the additional arguments were assigned values. Not checking the return value and reading one of the arguments not assigned a value is undefined behavior and may have unexpected consequences.</p>
7+
</overview>
8+
9+
<recommendation>
10+
<p>
11+
The user should check the return value of `scanf` and related functions and check that any additional argument was assigned a value before reading the additional argument.
12+
</p>
13+
</recommendation>
14+
<example>
15+
<p>The first first example below is correct, as value of `i` is only read once it is checked that `scanf` has read one item. The second example is incorrect, as the return value of `scanf` is not checked, and as `scanf` might have failed to read any item before returning.</p>
16+
<sample src="ImproperCheckReturnValueScanf.cpp" />
17+
18+
</example>
19+
<references>
20+
21+
<li>
22+
CERT C Coding Standard:
23+
<a href="https://wiki.sei.cmu.edu/confluence/display/c/EXP12-C.+Do+not+ignore+values+returned+by+functions">EXP12-C. Do not ignore values returned by functions</a>.
24+
</li>
25+
26+
</references>
27+
</qhelp>

‎cpp/ql/src/experimental/Security/CWE/CWE-754/ImproperCheckReturnValueScanf.ql

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
/**
2+
* @name Improper check of return value of scanf
3+
* @description Not checking the return value of scanf and related functions may lead to undefined behavior.
4+
* @kind problem
5+
* @id cpp/improper-check-return-value-scanf
6+
* @problem.severity warning
7+
* @precision medium
8+
* @tags correctness
9+
* security
10+
* external/cwe/cwe-754
11+
* external/cwe/cwe-908
12+
*/
13+
14+
import cpp
15+
import semmle.code.cpp.commons.Exclusions
16+
17+
/** Returns the position of the first argument being filled. */
18+
int posArgumentInFunctionCall(FunctionCall fc) {
19+
(
20+
fc.getTarget().hasGlobalOrStdName(["scanf", "scanf_s"]) and
21+
result = 1
22+
or
23+
fc.getTarget().hasGlobalOrStdName(["fscanf", "sscanf", "fscanf_s", "sscanf_s"]) and
24+
result = 2
25+
)
26+
}
27+
28+
/** Holds if a function argument was not initialized but used after the call. */
29+
predicate argumentIsNotInitializedAndIsUsed(Variable vt, FunctionCall fc) {
30+
// Fillable argument was not initialized.
31+
vt instanceof LocalScopeVariable and
32+
not vt.getAnAssignment().getASuccessor+() = fc and
33+
(
34+
not vt.hasInitializer()
35+
or
36+
exists(Expr e, Variable v |
37+
e = vt.getInitializer().getExpr() and
38+
v = e.(AddressOfExpr).getOperand().(VariableAccess).getTarget() and
39+
(
40+
not v.hasInitializer() and
41+
not v.getAnAssignment().getASuccessor+() = fc
42+
)
43+
)
44+
) and
45+
not exists(AssignExpr ae |
46+
ae.getLValue() = vt.getAnAccess().getParent() and
47+
ae.getASuccessor+() = fc
48+
) and
49+
not exists(FunctionCall f0 |
50+
f0.getAnArgument().getAChild() = vt.getAnAccess() and
51+
f0.getASuccessor+() = fc
52+
) and
53+
exists(Expr e0 |
54+
// After the call, the completed arguments are assigned or returned as the result of the operation of the upper function.
55+
fc.getASuccessor+() = e0 and
56+
(
57+
(
58+
e0.(Assignment).getRValue().(VariableAccess).getTarget() = vt or
59+
e0.(Assignment).getRValue().(ArrayExpr).getArrayBase().(VariableAccess).getTarget() = vt
60+
)
61+
or
62+
e0.getEnclosingStmt() instanceof ReturnStmt and
63+
e0.(VariableAccess).getTarget() = vt
64+
or
65+
not exists(Expr e1 |
66+
fc.getASuccessor+() = e1 and
67+
e1.(VariableAccess).getTarget() = vt
68+
)
69+
)
70+
)
71+
}
72+
73+
from FunctionCall fc, int i
74+
where
75+
// Function return value is not evaluated.
76+
fc instanceof ExprInVoidContext and
77+
not isFromMacroDefinition(fc) and
78+
i in [posArgumentInFunctionCall(fc) .. fc.getNumberOfArguments() - 1] and
79+
(
80+
argumentIsNotInitializedAndIsUsed(fc.getArgument(i).(VariableAccess).getTarget(), fc) or
81+
argumentIsNotInitializedAndIsUsed(fc.getArgument(i)
82+
.(AddressOfExpr)
83+
.getOperand()
84+
.(VariableAccess)
85+
.getTarget(), fc) or
86+
argumentIsNotInitializedAndIsUsed(fc.getArgument(i)
87+
.(ArrayExpr)
88+
.getArrayBase()
89+
.(VariableAccess)
90+
.getTarget(), fc)
91+
) and
92+
// After the call, filled arguments are not evaluated.
93+
not exists(Expr e0, int i1 |
94+
i1 in [posArgumentInFunctionCall(fc) .. fc.getNumberOfArguments() - 1] and
95+
fc.getASuccessor+() = e0 and
96+
e0.getEnclosingElement() instanceof ComparisonOperation and
97+
(
98+
e0.(VariableAccess).getTarget() = fc.getArgument(i1).(VariableAccess).getTarget() or
99+
e0.(VariableAccess).getTarget() =
100+
fc.getArgument(i1).(AddressOfExpr).getOperand().(VariableAccess).getTarget()
101+
)
102+
)
103+
select fc, "Unchecked return value for call to '" + fc.getTarget().getName() + "'."

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-754/semmle/tests/ImproperCheckReturnValueScanf.expected

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
| test.cpp:52:3:52:7 | call to scanf | Unchecked return value for call to 'scanf'. |
2+
| test.cpp:53:3:53:7 | call to scanf | Unchecked return value for call to 'scanf'. |
3+
| test.cpp:54:3:54:7 | call to scanf | Unchecked return value for call to 'scanf'. |
4+
| test.cpp:105:3:105:7 | call to scanf | Unchecked return value for call to 'scanf'. |
5+
| test.cpp:106:3:106:7 | call to scanf | Unchecked return value for call to 'scanf'. |
6+
| test.cpp:107:3:107:7 | call to scanf | Unchecked return value for call to 'scanf'. |
7+
| test.cpp:115:3:115:7 | call to scanf | Unchecked return value for call to 'scanf'. |
8+
| test.cpp:120:3:120:7 | call to scanf | Unchecked return value for call to 'scanf'. |

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-754/semmle/tests/ImproperCheckReturnValueScanf.qlref

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

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

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
int scanf(const char *format, ...);
2+
int sscanf(const char *str, const char *format, ...);
3+
int globalVal;
4+
char * globalVala;
5+
int * globalValp;
6+
char globalVala2;
7+
int functionWork1(int retIndex) {
8+
int i;
9+
char a[10];
10+
int b;
11+
int *p = &b;
12+
if (scanf("%i", &i) != 1) // GOOD
13+
return -1;
14+
if (scanf("%s", a) != 1) // GOOD
15+
return -1;
16+
if (scanf("%i", p) != 1) // GOOD
17+
return -1;
18+
if(retIndex == 0)
19+
return (int)*a;
20+
if(retIndex == 1)
21+
return *p;
22+
return i;
23+
}
24+
25+
int functionWork1_(int retIndex) {
26+
int i;
27+
char a[10];
28+
int b;
29+
int *p = &b;
30+
int r;
31+
r = scanf("%i", &i);
32+
if (r != 1) // GOOD
33+
return -1;
34+
r = scanf("%s", a);
35+
if (r == 1) // GOOD
36+
return -1;
37+
r = scanf("%i", p);
38+
if (r != 1) // GOOD
39+
return -1;
40+
if(retIndex == 0)
41+
return (int)*a;
42+
if(retIndex == 1)
43+
return *p;
44+
return i;
45+
}
46+
47+
int functionWork1b(int retIndex) {
48+
int i;
49+
char a[10];
50+
int b;
51+
int *p = &b;
52+
scanf("%i", &i); // BAD
53+
scanf("%s", a); // BAD
54+
scanf("%i", p); // BAD
55+
if(retIndex == 0)
56+
return (int)*a;
57+
if(retIndex == 1)
58+
return *p;
59+
return i;
60+
}
61+
int functionWork1_() {
62+
int i;
63+
scanf("%i",&i); // BAD [NOT DETECTED]
64+
if(i<10)
65+
return -1;
66+
return i;
67+
}
68+
int functionWork2(int retIndex) {
69+
int i = 0;
70+
char a[10] = "";
71+
int b = 1;
72+
int *p = &b;
73+
scanf("%i", &i); // GOOD:Argument initialized even when scanf fails.
74+
scanf("%s", a); // GOOD:Argument initialized even when scanf fails.
75+
scanf("%i", p); // GOOD:Argument initialized even when scanf fails.
76+
if(retIndex == 0)
77+
return (int)*a;
78+
if(retIndex == 1)
79+
return *p;
80+
return i;
81+
}
82+
83+
int functionWork2_(int retIndex) {
84+
int i;
85+
i = 0;
86+
char a[10];
87+
a[0] = '\0';
88+
int b;
89+
b=1;
90+
int *p = &b;
91+
scanf("%i", &i); // GOOD:Argument initialized even when scanf fails.
92+
scanf("%s", a); // GOOD:Argument initialized even when scanf fails.
93+
scanf("%i", p); // GOOD:Argument initialized even when scanf fails.
94+
if(retIndex == 0)
95+
return (int)*a;
96+
if(retIndex == 1)
97+
return *p;
98+
return i;
99+
}
100+
int functionWork2b() {
101+
int i;
102+
char a[10];
103+
int b;
104+
int *p = &b;
105+
scanf("%i", &i); // BAD
106+
scanf("%s", a); // BAD
107+
scanf("%i", p); // BAD
108+
globalVal = i;
109+
globalVala = a;
110+
globalValp = p;
111+
return 0;
112+
}
113+
int functionWork2b_() {
114+
char a[10];
115+
scanf("%s", a); // BAD
116+
globalVala2 = a[0];
117+
return 0;
118+
}
119+
int functionWork3b(int * i) {
120+
scanf("%i", i); // BAD
121+
return 0;
122+
}
123+
int functionWork3() {
124+
char number[] = "42";
125+
int d;
126+
sscanf(number, "%d", &d); // GOOD: sscanf always succeeds
127+
if (d < 16)
128+
return -1;
129+
}

0 commit comments

Comments
 (0)