Skip to content

Commit 7584434

Browse files
d10cd10c
authored
Merge pull request #10163 from d10c/missing-check-scanf-squashed
C++: New Query: missing return-value check for scanf-like functions
2 parents 2223bc3 + f956999 commit 7584434

File tree

8 files changed

+623
-0
lines changed

8 files changed

+623
-0
lines changed

‎cpp/ql/lib/semmle/code/cpp/commons/Scanf.qll‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,28 @@ class ScanfFunctionCall extends FunctionCall {
143143
* (rather than a `char*`).
144144
*/
145145
predicate isWideCharDefault() { this.getScanfFunction().isWideCharDefault() }
146+
147+
/**
148+
* Gets the output argument at position `n` in the vararg list of this call.
149+
*
150+
* The range of `n` is from `0` to `this.getNumberOfOutputArguments() - 1`.
151+
*/
152+
Expr getOutputArgument(int n) {
153+
result = this.getArgument(this.getTarget().getNumberOfParameters() + n) and
154+
n >= 0
155+
}
156+
157+
/**
158+
* Gets an output argument given to this call in vararg position.
159+
*/
160+
Expr getAnOutputArgument() { result = this.getOutputArgument(_) }
161+
162+
/**
163+
* Gets the number of output arguments present in this call.
164+
*/
165+
int getNumberOfOutputArguments() {
166+
result = this.getNumberOfArguments() - this.getTarget().getNumberOfParameters()
167+
}
146168
}
147169

148170
/**

‎cpp/ql/src/Critical/MissingCheckScanf.cpp‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
{
2+
int i, j, r;
3+
4+
r = scanf("%d %d", &i, &j);
5+
6+
use(i); // BAD: i is not guarded
7+
8+
if (r >= 1) {
9+
use(i); // GOOD: i is guarded correctly
10+
use(j); // BAD: j is guarded incorrectly
11+
}
12+
13+
if (r != 2)
14+
return;
15+
16+
use(j); // GOOD: j is guarded correctly
17+
}

‎cpp/ql/src/Critical/MissingCheckScanf.qhelp‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
7+
<overview>
8+
<p>
9+
This query finds calls of <tt>scanf</tt>-like functions with missing or
10+
improper return-value checking.
11+
</p>
12+
<p>
13+
Specifically, the query flags uses of variables that may have been modified by
14+
<tt>scanf</tt> and subsequently are used without being guarded by a correct
15+
return-value check. A proper check is one that ensures that the corresponding
16+
<tt>scanf</tt> has returned (at least) a certain minimum constant.
17+
</p>
18+
<p>
19+
Functions in the <tt>scanf</tt> family return either EOF (a negative value)
20+
in case of IO failure, or the number of items successfully read from the
21+
input. Consequently, a simple check that the return value is truthy (nonzero)
22+
is not enough.
23+
</p>
24+
<warning>
25+
This query has medium precision because, in the current implementation, it
26+
takes a strict stance on unguarded uses of output variables, and flags them
27+
as problematic even if they have already been initialized.
28+
</warning>
29+
</overview>
30+
31+
<recommendation>
32+
<p>
33+
Ensure that all subsequent uses of <tt>scanf</tt> output arguments occur in a
34+
branch of an <tt>if</tt> statement (or similar), in which it is known that the
35+
corresponding <tt>scanf</tt> call has in fact read all possible items from its
36+
input. This can be done by comparing the return value to a numerical constant.
37+
</p>
38+
</recommendation>
39+
40+
<example>
41+
<p>This example shows different ways of guarding a <tt>scanf</tt> output:
42+
</p>
43+
<sample src="MissingCheckScanf.cpp" />
44+
</example>
45+
46+
<references>
47+
<li>SEI CERT C++ Coding Standard: <a href="https://wiki.sei.cmu.edu/confluence/display/cplusplus/ERR62-CPP.+Detect+errors+when+converting+a+string+to+a+number">ERR62-CPP. Detect errors when converting a string to a number</a>.</li>
48+
<li>SEI CERT C Coding Standard: <a href="https://wiki.sei.cmu.edu/confluence/display/c/ERR33-C.+Detect+and+handle+standard+library+errors">ERR33-C. Detect and handle standard library errors</a>.</li>
49+
<li>cppreference.com: <a href="https://en.cppreference.com/w/c/io/fscanf">scanf, fscanf, sscanf, scanf_s, fscanf_s, sscanf_s</a>.</li>
50+
</references>
51+
</qhelp>

‎cpp/ql/src/Critical/MissingCheckScanf.ql‎

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
/**
2+
* @name Missing return-value check for a 'scanf'-like function
3+
* @description Failing to check that a call to 'scanf' actually writes to an
4+
* output variable can lead to unexpected behavior at reading time.
5+
* @kind problem
6+
* @problem.severity warning
7+
* @security-severity 7.5
8+
* @precision medium
9+
* @id cpp/missing-check-scanf
10+
* @tags security
11+
* correctness
12+
* external/cwe/cwe-252
13+
* external/cwe/cwe-253
14+
*/
15+
16+
import cpp
17+
import semmle.code.cpp.commons.Scanf
18+
import semmle.code.cpp.controlflow.Guards
19+
import semmle.code.cpp.dataflow.DataFlow
20+
import semmle.code.cpp.ir.IR
21+
import semmle.code.cpp.ir.ValueNumbering
22+
23+
/** An expression appearing as an output argument to a `scanf`-like call */
24+
class ScanfOutput extends Expr {
25+
ScanfFunctionCall call;
26+
int varargIndex;
27+
Instruction instr;
28+
ValueNumber valNum;
29+
30+
ScanfOutput() {
31+
this = call.getOutputArgument(varargIndex).getFullyConverted() and
32+
instr.getConvertedResultExpression() = this and
33+
valueNumber(instr) = valNum
34+
}
35+
36+
ScanfFunctionCall getCall() { result = call }
37+
38+
/**
39+
* Returns the smallest possible `scanf` return value that would indicate
40+
* success in writing this output argument.
41+
*/
42+
int getMinimumGuardConstant() {
43+
result =
44+
varargIndex + 1 -
45+
count(ScanfFormatLiteral f, int n |
46+
// Special case: %n writes to an argument without reading any input.
47+
// It does not increase the count returned by `scanf`.
48+
n <= varargIndex and f.getUse() = call and f.getConversionChar(n) = "n"
49+
)
50+
}
51+
52+
predicate hasGuardedAccess(Access e, boolean isGuarded) {
53+
e = this.getAnAccess() and
54+
if
55+
exists(int value, int minGuard | minGuard = this.getMinimumGuardConstant() |
56+
e.getBasicBlock() = blockGuardedBy(value, "==", call) and minGuard <= value
57+
or
58+
e.getBasicBlock() = blockGuardedBy(value, "<", call) and minGuard - 1 <= value
59+
or
60+
e.getBasicBlock() = blockGuardedBy(value, "<=", call) and minGuard <= value
61+
)
62+
then isGuarded = true
63+
else isGuarded = false
64+
}
65+
66+
/**
67+
* Get a subsequent access of the same underlying storage,
68+
* but before it gets reset or reused in another `scanf` call.
69+
*/
70+
Access getAnAccess() {
71+
exists(Instruction dst |
72+
this.bigStep() = dst and
73+
dst.getAst() = result and
74+
valueNumber(dst) = valNum
75+
)
76+
}
77+
78+
private Instruction bigStep() {
79+
result = this.smallStep(instr)
80+
or
81+
exists(Instruction i | i = this.bigStep() | result = this.smallStep(i))
82+
}
83+
84+
private Instruction smallStep(Instruction i) {
85+
instr.getASuccessor*() = i and
86+
i.getASuccessor() = result and
87+
not this.isBarrier(result)
88+
}
89+
90+
private predicate isBarrier(Instruction i) {
91+
valueNumber(i) = valNum and
92+
exists(Expr e | i.getAst() = e |
93+
i = any(StoreInstruction s).getDestinationAddress()
94+
or
95+
[e, e.getParent().(AddressOfExpr)] instanceof ScanfOutput
96+
)
97+
}
98+
}
99+
100+
/** Returns a block guarded by the assertion of `value op call` */
101+
BasicBlock blockGuardedBy(int value, string op, ScanfFunctionCall call) {
102+
exists(GuardCondition g, Expr left, Expr right |
103+
right = g.getAChild() and
104+
value = left.getValue().toInt() and
105+
DataFlow::localExprFlow(call, right)
106+
|
107+
g.ensuresEq(left, right, 0, result, true) and op = "=="
108+
or
109+
g.ensuresLt(left, right, 0, result, true) and op = "<"
110+
or
111+
g.ensuresLt(left, right, 1, result, true) and op = "<="
112+
)
113+
}
114+
115+
from ScanfOutput output, ScanfFunctionCall call, Access access
116+
where
117+
output.getCall() = call and
118+
output.hasGuardedAccess(access, false)
119+
select access,
120+
"$@ is read here, but may not have been written. " +
121+
"It should be guarded by a check that the $@ returns at least " +
122+
output.getMinimumGuardConstant() + ".", access, access.toString(), call, call.toString()

‎cpp/ql/src/change-notes/2022-08-24-missing-check-scanf.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* Added a new medium-precision query, `cpp/missing-check-scanf`, which detects `scanf` output variables that are used without a proper return-value check to see that they were actually written. A variation of this query was originally contributed as an [experimental query by @ihsinme](https://github.com/github/codeql/pull/8246).

‎cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
| test.cpp:30:7:30:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:30:7:30:7 | i | i | test.cpp:29:3:29:7 | call to scanf | call to scanf |
2+
| test.cpp:46:7:46:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:46:7:46:7 | i | i | test.cpp:45:3:45:7 | call to scanf | call to scanf |
3+
| test.cpp:63:7:63:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:63:7:63:7 | i | i | test.cpp:62:3:62:7 | call to scanf | call to scanf |
4+
| test.cpp:75:7:75:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:75:7:75:7 | i | i | test.cpp:74:3:74:7 | call to scanf | call to scanf |
5+
| test.cpp:87:7:87:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:87:7:87:7 | i | i | test.cpp:86:3:86:8 | call to fscanf | call to fscanf |
6+
| test.cpp:94:7:94:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:94:7:94:7 | i | i | test.cpp:93:3:93:8 | call to sscanf | call to sscanf |
7+
| test.cpp:143:8:143:8 | i | $@ is read here, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:143:8:143:8 | i | i | test.cpp:141:7:141:11 | call to scanf | call to scanf |
8+
| test.cpp:152:8:152:8 | i | $@ is read here, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:152:8:152:8 | i | i | test.cpp:150:7:150:11 | call to scanf | call to scanf |
9+
| test.cpp:184:8:184:8 | i | $@ is read here, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:184:8:184:8 | i | i | test.cpp:183:7:183:11 | call to scanf | call to scanf |
10+
| test.cpp:203:8:203:8 | j | $@ is read here, but may not have been written. It should be guarded by a check that the $@ returns at least 2. | test.cpp:203:8:203:8 | j | j | test.cpp:200:7:200:11 | call to scanf | call to scanf |
11+
| test.cpp:227:9:227:9 | d | $@ is read here, but may not have been written. It should be guarded by a check that the $@ returns at least 2. | test.cpp:227:9:227:9 | d | d | test.cpp:225:25:225:29 | call to scanf | call to scanf |
12+
| test.cpp:231:9:231:9 | d | $@ is read here, but may not have been written. It should be guarded by a check that the $@ returns at least 2. | test.cpp:231:9:231:9 | d | d | test.cpp:229:14:229:18 | call to scanf | call to scanf |
13+
| test.cpp:243:7:243:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:243:7:243:7 | i | i | test.cpp:242:3:242:7 | call to scanf | call to scanf |
14+
| test.cpp:251:7:251:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:251:7:251:7 | i | i | test.cpp:250:3:250:7 | call to scanf | call to scanf |
15+
| test.cpp:259:7:259:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:259:7:259:7 | i | i | test.cpp:258:3:258:7 | call to scanf | call to scanf |
16+
| test.cpp:271:7:271:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:271:7:271:7 | i | i | test.cpp:270:3:270:7 | call to scanf | call to scanf |
17+
| test.cpp:281:8:281:12 | ptr_i | $@ is read here, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:281:8:281:12 | ptr_i | ptr_i | test.cpp:280:3:280:7 | call to scanf | call to scanf |
18+
| test.cpp:289:7:289:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:289:7:289:7 | i | i | test.cpp:288:3:288:7 | call to scanf | call to scanf |
19+
| test.cpp:383:25:383:25 | u | $@ is read here, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:383:25:383:25 | u | u | test.cpp:382:6:382:11 | call to sscanf | call to sscanf |

‎cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.qlref‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Critical/MissingCheckScanf.ql

0 commit comments

Comments
 (0)