Skip to content

Commit ca162a4

Browse files
d10cd10c
committed
C++: complete initial implementation of cpp/missing-check-scanf
There are still some remaining FPs (haven't fully tested them) that should be ironed out in a follow-up to increase the precision, e.g.: * if scanf(&i) != 1 return if maybe() && scanf(&i) != 1 return use(i) // should be OK on both counts * The minimum guard constant for the *_s variants may not be right. * int i[2] scanf(i, i+1) // second i is flagged as a use of the first * Maybe loosen the "unguarded or badly guarded use() = bad" policy to "unguarded but already-initialized = good" and "badly guarded = bad", since a lot of FPs in MRVA fall into the "unguarded but already- initialized" bucket.
1 parent 69911d4 commit ca162a4

File tree

3 files changed

+355
-60
lines changed

3 files changed

+355
-60
lines changed

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

Lines changed: 109 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,122 @@
11
/**
2-
* @name TODO
2+
* @name Missing return-value check for a scanf-like function
33
* @description TODO
44
* @kind problem
55
* @problem.severity warning
66
* @security-severity TODO
77
* @precision TODO
88
* @id cpp/missing-check-scanf
9-
* @tags TODO
9+
* @tags security
1010
*/
1111

1212
import cpp
1313
import semmle.code.cpp.commons.Scanf
14+
import semmle.code.cpp.controlflow.Guards
15+
import semmle.code.cpp.dataflow.DataFlow
16+
import semmle.code.cpp.ir.IR
17+
import semmle.code.cpp.ir.ValueNumbering
1418

15-
from ScanfFunction scanf, FunctionCall fc
19+
/** An expression appearing as an output argument to a `scanf`-like call */
20+
class ScanfOutput extends Expr {
21+
ScanfFunctionCall call;
22+
int varargIndex;
23+
Instruction instr;
24+
ValueNumber valNum;
25+
26+
ScanfOutput() {
27+
this = call.getArgument(call.getTarget().getNumberOfParameters() + varargIndex) and
28+
varargIndex >= 0 and
29+
instr.getUnconvertedResultExpression() = this and
30+
valueNumber(instr) = valNum and
31+
// The following line is a kludge to prohibit more than one associated `instr` field,
32+
// as would occur, for example, when `this` is an access to an array variable.
33+
not instr instanceof ConvertInstruction
34+
}
35+
36+
ScanfFunctionCall getCall() { result = call }
37+
38+
/**
39+
* Any subsequent use of this argument should be surrounded by a
40+
* check ensuring that the `scanf`-like function has returned a value
41+
* equal to at least `getMinimumGuardConstant()`.
42+
*/
43+
int getMinimumGuardConstant() {
44+
result =
45+
varargIndex + 1 -
46+
count(ScanfFormatLiteral f, int n |
47+
n <= varargIndex and f.getUse() = call and f.parseConvSpec(n, _, _, _, "n")
48+
)
49+
}
50+
51+
predicate hasGuardedAccess(Access e, boolean isGuarded) {
52+
e = this.getAnAccess() and
53+
if
54+
exists(int value, int minGuard | minGuard = this.getMinimumGuardConstant() |
55+
e.getBasicBlock() = blockGuardedBy(value, "==", call) and minGuard <= value
56+
or
57+
e.getBasicBlock() = blockGuardedBy(value, "<", call) and minGuard - 1 <= value
58+
or
59+
e.getBasicBlock() = blockGuardedBy(value, "<=", call) and minGuard <= value
60+
)
61+
then isGuarded = true
62+
else isGuarded = false
63+
}
64+
65+
/**
66+
* Get a subsequent access of the same underlying storage,
67+
* but before it gets reset or reused in another `scanf` call.
68+
*/
69+
Access getAnAccess() {
70+
exists(Instruction dst |
71+
this.bigStep(instr) = dst and
72+
dst.getAst() = result and
73+
valueNumber(dst) = valNum
74+
)
75+
}
76+
77+
private Instruction bigStep(Instruction i) {
78+
result = this.smallStep(i)
79+
or
80+
exists(Instruction j | j = this.bigStep(i) | result = this.smallStep(j))
81+
}
82+
83+
private Instruction smallStep(Instruction i) {
84+
instr.getASuccessor*() = i and
85+
i.getASuccessor() = result and
86+
not this.isBarrier(result)
87+
}
88+
89+
private predicate isBarrier(Instruction i) {
90+
valueNumber(i) = valNum and
91+
exists(Expr e | i.getAst() = e |
92+
i = any(StoreInstruction s).getDestinationAddress()
93+
or
94+
[e, e.getParent().(AddressOfExpr)] instanceof ScanfOutput
95+
)
96+
}
97+
}
98+
99+
/** Returns a block guarded by the assertion of $value $op $call */
100+
BasicBlock blockGuardedBy(int value, string op, ScanfFunctionCall call) {
101+
exists(GuardCondition g, Expr left, Expr right |
102+
right = g.getAChild() and
103+
value = left.getValue().toInt() and
104+
DataFlow::localExprFlow(call, right)
105+
|
106+
g.ensuresEq(left, right, 0, result, true) and op = "=="
107+
or
108+
g.ensuresLt(left, right, 0, result, true) and op = "<"
109+
or
110+
g.ensuresLt(left, right, 1, result, true) and op = "<="
111+
)
112+
}
113+
114+
from ScanfOutput output, ScanfFunctionCall call, ScanfFunction fun, Access access
16115
where
17-
fc.getTarget() = scanf and
18-
fc instanceof ExprInVoidContext
19-
select fc, "This is a call to scanf."
116+
call.getTarget() = fun and
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 $@() returns at least " + output.getMinimumGuardConstant()
122+
+ ".", access, access.toString(), call, fun.getName()

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

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,19 @@
1-
| test.cpp:23:3:23:7 | call to scanf | This is a call to scanf. |
2-
| test.cpp:39:3:39:7 | call to scanf | This is a call to scanf. |
3-
| test.cpp:48:3:48:8 | call to fscanf | This is a call to scanf. |
4-
| test.cpp:55:3:55:8 | call to sscanf | This is a call to scanf. |
5-
| test.cpp:135:3:135:7 | call to scanf | This is a call to scanf. |
6-
| test.cpp:143:3:143:7 | call to scanf | This is a call to scanf. |
7-
| test.cpp:151:3:151:7 | call to scanf | This is a call to scanf. |
8-
| test.cpp:163:3:163:7 | call to scanf | This is a call to scanf. |
9-
| test.cpp:173:3:173:7 | call to scanf | This is a call to scanf. |
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 $@() returns at least 1. | test.cpp:30:7:30:7 | i | i | test.cpp:29:3:29:7 | call to scanf | 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 $@() returns at least 1. | test.cpp:46:7:46:7 | i | i | test.cpp:45:3:45:7 | call to scanf | 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 $@() returns at least 1. | test.cpp:63:7:63:7 | i | i | test.cpp:62:3:62:7 | call to scanf | 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 $@() returns at least 1. | test.cpp:75:7:75:7 | i | i | test.cpp:74:3:74:7 | call to scanf | 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 $@() returns at least 1. | test.cpp:87:7:87:7 | i | i | test.cpp:86:3:86:8 | call to fscanf | 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 $@() returns at least 1. | test.cpp:94:7:94:7 | i | i | test.cpp:93:3:93:8 | call to sscanf | 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 $@() returns at least 1. | test.cpp:143:8:143:8 | i | i | test.cpp:141:7:141:11 | call to scanf | 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 $@() returns at least 1. | test.cpp:152:8:152:8 | i | i | test.cpp:150:7:150:11 | call to scanf | 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 $@() returns at least 1. | test.cpp:184:8:184:8 | i | i | test.cpp:183:7:183:11 | call to scanf | 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 $@() returns at least 2. | test.cpp:203:8:203:8 | j | j | test.cpp:200:7:200:11 | call to scanf | 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 $@() returns at least 2. | test.cpp:227:9:227:9 | d | d | test.cpp:225:25:225:29 | call to scanf | 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 $@() returns at least 2. | test.cpp:231:9:231:9 | d | d | test.cpp:229:14:229:18 | call to scanf | 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 $@() returns at least 1. | test.cpp:243:7:243:7 | i | i | test.cpp:242:3:242:7 | call to scanf | 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 $@() returns at least 1. | test.cpp:251:7:251:7 | i | i | test.cpp:250:3:250:7 | call to scanf | 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 $@() returns at least 1. | test.cpp:259:7:259:7 | i | i | test.cpp:258:3:258:7 | call to scanf | 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 $@() returns at least 1. | test.cpp:271:7:271:7 | i | i | test.cpp:270:3:270:7 | call to scanf | 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 $@() returns at least 1. | test.cpp:281:8:281:12 | ptr_i | ptr_i | test.cpp:280:3:280:7 | call to scanf | 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 $@() returns at least 1. | test.cpp:289:7:289:7 | i | i | test.cpp:288:3:288:7 | call to scanf | 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 $@() returns at least 1. | test.cpp:383:25:383:25 | u | u | test.cpp:382:6:382:11 | call to sscanf | sscanf |

0 commit comments

Comments
 (0)