Skip to content

Commit e241373

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #1711 from aschackmull/java/arithmetic-barriers
Approved by yh-semmle
2 parents 34106ec + de13d0c commit e241373

File tree

11 files changed

+312
-190
lines changed

11 files changed

+312
-190
lines changed

‎change-notes/1.22/analysis-java.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
| **Query** | **Expected impact** | **Change** |
66
|----------------------------|------------------------|------------------------------------------------------------------|
77
| Equals method does not inspect argument type (`java/unchecked-cast-in-equals`) | Fewer false positive and more true positive results | Precision has been improved by doing a bit of inter-procedural analysis and relying less on ad-hoc method names. |
8+
| Uncontrolled data in arithmetic expression (`java/uncontrolled-arithmetic`) | Fewer false positive results | Precision has been improved in several ways, in particular, by better detection of guards along the data-flow path. |
9+
| User-controlled data in arithmetic expression (`java/tainted-arithmetic`) | Fewer false positive results | Precision has been improved in several ways, in particular, by better detection of guards along the data-flow path. |
810

911
## Changes to QL libraries
1012

‎java/ql/src/Security/CWE/CWE-190/ArithmeticCommon.qll‎

Lines changed: 191 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,11 @@
11
import semmle.code.java.arithmetic.Overflow
2-
import semmle.code.java.controlflow.Dominance
3-
import semmle.code.java.dataflow.DefUse
42
import semmle.code.java.controlflow.Guards
3+
private import semmle.code.java.dataflow.SSA
4+
private import semmle.code.java.dataflow.DataFlow
5+
private import semmle.code.java.dataflow.RangeAnalysis
6+
private import semmle.code.java.dataflow.RangeUtils
7+
private import semmle.code.java.dataflow.SignAnalysis
8+
private import semmle.code.java.controlflow.internal.GuardsLogic
59

610
/**
711
* Holds if the type of `exp` is narrower than or equal to `numType`,
@@ -15,102 +19,138 @@ predicate narrowerThanOrEqualTo(ArithExpr exp, NumType numType) {
1519
)
1620
}
1721

18-
/** Holds if the size of this use is guarded using `Math.abs`. */
19-
predicate guardedAbs(ArithExpr e, Expr use) {
20-
exists(MethodAccess m | m.getMethod() instanceof MethodAbs |
21-
m.getArgument(0) = use and
22-
guardedLesser(e, m)
22+
private Guard sizeGuard(SsaVariable v, boolean branch, boolean upper) {
23+
exists(ComparisonExpr comp | comp = result |
24+
comp.getLesserOperand() = ssaRead(v, 0) and
25+
(
26+
branch = true and upper = true
27+
or
28+
branch = false and upper = false
29+
)
30+
or
31+
comp.getGreaterOperand() = ssaRead(v, 0) and
32+
(
33+
branch = true and upper = false
34+
or
35+
branch = false and upper = true
36+
)
37+
or
38+
exists(MethodAccess ma |
39+
ma.getMethod() instanceof MethodAbs and
40+
ma.getArgument(0) = ssaRead(v, 0) and
41+
(
42+
comp.getLesserOperand() = ma and branch = true
43+
or
44+
comp.getGreaterOperand() = ma and branch = false
45+
) and
46+
(upper = false or upper = true)
47+
)
48+
or
49+
// overflow test
50+
exists(AddExpr add, RValue use, Expr pos |
51+
use = ssaRead(v, 0) and
52+
add.hasOperands(use, pos) and
53+
positive(use) and
54+
positive(pos) and
55+
upper = true
56+
|
57+
comp.getLesserOperand().getProperExpr() = add and
58+
comp.getGreaterOperand().(IntegerLiteral).getIntValue() = 0 and
59+
branch = false
60+
or
61+
comp.getGreaterOperand().getProperExpr() = add and
62+
comp.getLesserOperand().(IntegerLiteral).getIntValue() = 0 and
63+
branch = true
64+
)
65+
)
66+
or
67+
result.isEquality(ssaRead(v, 0), _, branch) and
68+
(upper = true or upper = false)
69+
or
70+
exists(MethodAccess call, Method m, int ix |
71+
call = result and
72+
call.getArgument(ix) = ssaRead(v, 0) and
73+
call.getMethod().getSourceDeclaration() = m and
74+
m = customSizeGuard(ix, branch, upper)
2375
)
2476
}
2577

26-
/** Holds if the size of this use is guarded to be less than something. */
27-
predicate guardedLesser(ArithExpr e, Expr use) {
28-
exists(ConditionBlock c, ComparisonExpr guard |
29-
use = guard.getLesserOperand() and
30-
guard = c.getCondition() and
31-
c.controls(e.getBasicBlock(), true)
32-
)
33-
or
34-
guardedAbs(e, use)
78+
private Guard derivedSizeGuard(SsaVariable v, boolean branch, boolean upper) {
79+
result = sizeGuard(v, branch, upper) or
80+
exists(boolean branch0 | implies_v3(result, branch, derivedSizeGuard(v, branch0, upper), branch0))
3581
}
3682

37-
/** Holds if the size of this use is guarded to be greater than something. */
38-
predicate guardedGreater(ArithExpr e, Expr use) {
39-
exists(ConditionBlock c, ComparisonExpr guard |
40-
use = guard.getGreaterOperand() and
41-
guard = c.getCondition() and
42-
c.controls(e.getBasicBlock(), true)
83+
private Method customSizeGuard(int index, boolean retval, boolean upper) {
84+
exists(Parameter p, SsaImplicitInit v |
85+
result.getReturnType().(PrimitiveType).hasName("boolean") and
86+
not result.isOverridable() and
87+
p.getCallable() = result and
88+
not p.isVarargs() and
89+
p.getType() instanceof NumericOrCharType and
90+
p.getPosition() = index and
91+
v.isParameterDefinition(p) and
92+
forex(ReturnStmt ret |
93+
ret.getEnclosingCallable() = result and
94+
exists(Expr res | res = ret.getResult() |
95+
not res.(BooleanLiteral).getBooleanValue() = retval.booleanNot()
96+
)
97+
|
98+
ret.getResult() = derivedSizeGuard(v, retval, upper)
99+
)
43100
)
44-
or
45-
guardedAbs(e, use)
46101
}
47102

48-
/** Holds if this expression is (crudely) guarded by `use`. */
49-
predicate guarded(ArithExpr e, Expr use) {
50-
exists(ConditionBlock c, ComparisonExpr guard |
51-
use = guard.getAnOperand() and
52-
guard = c.getCondition() and
53-
c.controls(e.getBasicBlock(), true)
103+
/**
104+
* Holds if `e` is bounded in a way that is likely to prevent overflow.
105+
*/
106+
predicate guardedLessThanSomething(Expr e) {
107+
exists(SsaVariable v, Guard guard, boolean branch |
108+
e = v.getAUse() and
109+
guard = sizeGuard(v.getAPhiInputOrPriorDef*(), branch, true) and
110+
guard.controls(e.getBasicBlock(), branch)
54111
)
112+
or
113+
negative(e)
114+
or
115+
e.(MethodAccess).getMethod() instanceof MethodMathMin
55116
}
56117

57-
/** A prior use of the same variable that could see the same value. */
58-
VarAccess priorAccess(VarAccess access) { useUsePair(result, access) }
59-
60-
/** Holds if `e` is guarded against overflow by `use`. */
61-
predicate guardedAgainstOverflow(ArithExpr e, VarAccess use) {
62-
use = e.getAnOperand() and
63-
(
64-
// overflow possible if large
65-
e instanceof AddExpr and guardedLesser(e, priorAccess(use))
66-
or
67-
e instanceof PreIncExpr and guardedLesser(e, priorAccess(use))
68-
or
69-
e instanceof PostIncExpr and guardedLesser(e, priorAccess(use))
70-
or
71-
// overflow unlikely with subtraction
72-
e instanceof SubExpr
73-
or
74-
e instanceof PreDecExpr
75-
or
76-
e instanceof PostDecExpr
77-
or
78-
// overflow possible if large or small
79-
e instanceof MulExpr and
80-
guardedLesser(e, priorAccess(use)) and
81-
guardedGreater(e, priorAccess(use))
82-
or
83-
// overflow possible if MIN_VALUE
84-
e instanceof DivExpr and guardedGreater(e, priorAccess(use))
118+
/**
119+
* Holds if `e` is bounded in a way that is likely to prevent underflow.
120+
*/
121+
predicate guardedGreaterThanSomething(Expr e) {
122+
exists(SsaVariable v, Guard guard, boolean branch |
123+
e = v.getAUse() and
124+
guard = sizeGuard(v.getAPhiInputOrPriorDef*(), branch, false) and
125+
guard.controls(e.getBasicBlock(), branch)
85126
)
127+
or
128+
positive(e)
129+
or
130+
e.(MethodAccess).getMethod() instanceof MethodMathMax
86131
}
87132

88-
/** Holds if `e` is guarded against underflow by `use`. */
89-
predicate guardedAgainstUnderflow(ArithExpr e, VarAccess use) {
90-
use = e.getAnOperand() and
91-
(
92-
// underflow unlikely for addition
93-
e instanceof AddExpr
94-
or
95-
e instanceof PreIncExpr
133+
/** Holds if `e` occurs in a context where it will be upcast to a wider type. */
134+
predicate upcastToWiderType(Expr e) {
135+
exists(NumType t1, NumType t2 |
136+
t1 = e.getType() and
137+
(
138+
t2.widerThan(t1)
139+
or
140+
t1 instanceof CharacterType and t2.getWidthRank() >= 3
141+
)
142+
|
143+
exists(Variable v | v.getAnAssignedValue() = e and t2 = v.getType())
96144
or
97-
e instanceof PostIncExpr
145+
exists(CastExpr c | c.getExpr() = e and t2 = c.getType())
98146
or
99-
// underflow possible if use is left operand and small
100-
e instanceof SubExpr and
101-
(use = e.getRightOperand() or guardedGreater(e, priorAccess(use)))
147+
exists(ReturnStmt ret | ret.getResult() = e and t2 = ret.getEnclosingCallable().getReturnType())
102148
or
103-
e instanceof PreDecExpr and guardedGreater(e, priorAccess(use))
149+
exists(Parameter p | p.getAnArgument() = e and t2 = p.getType())
104150
or
105-
e instanceof PostDecExpr and guardedGreater(e, priorAccess(use))
106-
or
107-
// underflow possible if large or small
108-
e instanceof MulExpr and
109-
guardedLesser(e, priorAccess(use)) and
110-
guardedGreater(e, priorAccess(use))
111-
or
112-
// underflow possible if MAX_VALUE
113-
e instanceof DivExpr and guardedLesser(e, priorAccess(use))
151+
exists(ConditionalExpr cond | cond.getTrueExpr() = e or cond.getFalseExpr() = e |
152+
t2 = cond.getType()
153+
)
114154
)
115155
}
116156

@@ -124,7 +164,77 @@ private predicate inBitwiseAnd(Expr exp) {
124164
}
125165

126166
/** Holds if overflow/underflow is irrelevant for this expression. */
127-
predicate overflowIrrelevant(ArithExpr exp) {
167+
predicate overflowIrrelevant(Expr exp) {
128168
inBitwiseAnd(exp) or
129169
exp.getEnclosingCallable() instanceof HashCodeMethod
130170
}
171+
172+
/**
173+
* Holds if `n` is unlikely to be part in a path from some source containing
174+
* numeric data to some arithmetic expression that may overflow/underflow.
175+
*/
176+
private predicate unlikelyNode(DataFlow::Node n) {
177+
n.getTypeBound() instanceof TypeObject and
178+
not exists(CastExpr cast |
179+
DataFlow::localFlow(n, DataFlow::exprNode(cast.getExpr())) and
180+
cast.getType() instanceof NumericOrCharType
181+
)
182+
}
183+
184+
/** Holds if `n` is likely guarded against overflow. */
185+
predicate overflowBarrier(DataFlow::Node n) {
186+
n.getType() instanceof BooleanType or
187+
guardedLessThanSomething(n.asExpr()) or
188+
unlikelyNode(n) or
189+
upcastToWiderType(n.asExpr()) or
190+
overflowIrrelevant(n.asExpr())
191+
}
192+
193+
/** Holds if `n` is likely guarded against underflow. */
194+
predicate underflowBarrier(DataFlow::Node n) {
195+
n.getType() instanceof BooleanType or
196+
guardedGreaterThanSomething(n.asExpr()) or
197+
unlikelyNode(n) or
198+
upcastToWiderType(n.asExpr()) or
199+
overflowIrrelevant(n.asExpr())
200+
}
201+
202+
/**
203+
* Holds if `use` is an operand of `exp` that acts as a sink for
204+
* overflow-related dataflow.
205+
*/
206+
predicate overflowSink(ArithExpr exp, VarAccess use) {
207+
exp.getAnOperand() = use and
208+
(
209+
// overflow unlikely for subtraction and division
210+
exp instanceof AddExpr or
211+
exp instanceof PreIncExpr or
212+
exp instanceof PostIncExpr or
213+
exp instanceof MulExpr
214+
) and
215+
not guardedLessThanSomething(use) and
216+
// Exclude widening conversions of tainted values due to binary numeric promotion (JLS 5.6.2)
217+
// unless there is an enclosing cast down to a narrower type.
218+
narrowerThanOrEqualTo(exp, use.getType()) and
219+
not overflowIrrelevant(exp)
220+
}
221+
222+
/**
223+
* Holds if `use` is an operand of `exp` that acts as a sink for
224+
* underflow-related dataflow.
225+
*/
226+
predicate underflowSink(ArithExpr exp, VarAccess use) {
227+
exp.getAnOperand() = use and
228+
(
229+
// underflow unlikely for addition and division
230+
exp.(SubExpr).getLeftOperand() = use or
231+
exp instanceof PreDecExpr or
232+
exp instanceof PostDecExpr or
233+
exp instanceof MulExpr
234+
) and
235+
not guardedGreaterThanSomething(use) and
236+
// Exclude widening conversions of tainted values due to binary numeric promotion (JLS 5.6.2)
237+
// unless there is an enclosing cast down to a narrower type.
238+
narrowerThanOrEqualTo(exp, use.getType()) and
239+
not overflowIrrelevant(exp)
240+
}

‎java/ql/src/Security/CWE/CWE-190/ArithmeticTainted.ql‎

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -16,35 +16,35 @@ import semmle.code.java.dataflow.FlowSources
1616
import ArithmeticCommon
1717
import DataFlow::PathGraph
1818

19-
predicate sink(ArithExpr exp, VarAccess tainted, string effect) {
20-
exp.getAnOperand() = tainted and
21-
(
22-
not guardedAgainstUnderflow(exp, tainted) and effect = "underflow"
23-
or
24-
not guardedAgainstOverflow(exp, tainted) and effect = "overflow"
25-
) and
26-
// Exclude widening conversions of tainted values due to binary numeric promotion (JLS 5.6.2)
27-
// unless there is an enclosing cast down to a narrower type.
28-
narrowerThanOrEqualTo(exp, tainted.getType()) and
29-
not overflowIrrelevant(exp)
19+
class RemoteUserInputOverflowConfig extends TaintTracking::Configuration {
20+
RemoteUserInputOverflowConfig() { this = "ArithmeticTainted.ql:RemoteUserInputOverflowConfig" }
21+
22+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
23+
24+
override predicate isSink(DataFlow::Node sink) { overflowSink(_, sink.asExpr()) }
25+
26+
override predicate isSanitizer(DataFlow::Node n) { overflowBarrier(n) }
3027
}
3128

32-
class RemoteUserInputConfig extends TaintTracking::Configuration {
33-
RemoteUserInputConfig() { this = "ArithmeticTainted.ql:RemoteUserInputConfig" }
29+
class RemoteUserInputUnderflowConfig extends TaintTracking::Configuration {
30+
RemoteUserInputUnderflowConfig() { this = "ArithmeticTainted.ql:RemoteUserInputUnderflowConfig" }
3431

3532
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
3633

37-
override predicate isSink(DataFlow::Node sink) { sink(_, sink.asExpr(), _) }
34+
override predicate isSink(DataFlow::Node sink) { underflowSink(_, sink.asExpr()) }
3835

39-
override predicate isSanitizer(DataFlow::Node n) { n.getType() instanceof BooleanType }
36+
override predicate isSanitizer(DataFlow::Node n) { underflowBarrier(n) }
4037
}
4138

42-
from
43-
DataFlow::PathNode source, DataFlow::PathNode sink, ArithExpr exp, string effect,
44-
RemoteUserInputConfig conf
39+
from DataFlow::PathNode source, DataFlow::PathNode sink, ArithExpr exp, string effect
4540
where
46-
conf.hasFlowPath(source, sink) and
47-
sink(exp, sink.getNode().asExpr(), effect)
41+
any(RemoteUserInputOverflowConfig c).hasFlowPath(source, sink) and
42+
overflowSink(exp, sink.getNode().asExpr()) and
43+
effect = "overflow"
44+
or
45+
any(RemoteUserInputUnderflowConfig c).hasFlowPath(source, sink) and
46+
underflowSink(exp, sink.getNode().asExpr()) and
47+
effect = "underflow"
4848
select exp, source, sink,
4949
"$@ flows to here and is used in arithmetic, potentially causing an " + effect + ".",
5050
source.getNode(), "User-provided value"

0 commit comments

Comments
 (0)