Skip to content

Commit 3d0229a

Browse files
owen-mcowen-mc
committed
Refactor #2 
1 parent 38c9c18 commit 3d0229a

File tree

3 files changed

+77
-16
lines changed

3 files changed

+77
-16
lines changed

‎go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll‎

Lines changed: 74 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ private module ConversionWithoutBoundsCheckConfig implements DataFlow::StateConf
233233
state.getConcreteBitsize() =
234234
min(int bitsize |
235235
bitsize = validBitsize() and
236-
f(effectiveBitSize, ip.isSigned(), 64) <= state.getConcreteBitsize()
236+
f(effectiveBitSize, ip.isSigned(), 64) <= bitsize
237237
|
238238
bitsize
239239
)
@@ -248,16 +248,25 @@ private module ConversionWithoutBoundsCheckConfig implements DataFlow::StateConf
248248
*/
249249
additional predicate isSink2(DataFlow::TypeCastNode sink, FlowState state) {
250250
sink.asExpr() instanceof ConversionExpr and
251-
exists(IntegerType integerType | sink.getResultType().getUnderlyingType() = integerType |
251+
exists(IntegerType integerType, int sinkBitsize, boolean sinkIsSigned |
252+
sink.getResultType().getUnderlyingType() = integerType and
252253
(
253-
state.getBitSize() = integerType.getSize()
254+
sinkBitsize = integerType.getSize()
254255
or
255256
not exists(integerType.getSize()) and
256-
state.getBitSize() = getIntTypeBitSize(sink.getFile(), 0)
257+
sinkBitsize = getIntTypeBitSize(sink.getFile(), 0)
257258
) and
258-
if integerType instanceof SignedIntegerType
259-
then state.getIsSigned() = true
260-
else state.getIsSigned() = false
259+
if integerType instanceof SignedIntegerType then sinkIsSigned = true else sinkIsSigned = false
260+
|
261+
// Flow state has a concrete value
262+
f(sinkBitsize, sinkIsSigned, 32) < state.getConcreteBitsize()
263+
or
264+
// Flow state has an architecture-independent value
265+
exists(boolean signed | signed = state.getArchictureDependentSignedness() |
266+
if sinkBitsize = 0
267+
then sinkIsSigned = true and signed = false
268+
else f(sinkBitsize, sinkIsSigned, 32) < f(0, signed, 64)
269+
)
261270
) and
262271
not exists(ShrExpr shrExpr |
263272
shrExpr.getLeftOperand().getGlobalValueNumber() =
@@ -276,16 +285,42 @@ private module ConversionWithoutBoundsCheckConfig implements DataFlow::StateConf
276285
}
277286

278287
predicate isBarrier(DataFlow::Node node, FlowState state) {
288+
isUpperBoundCheckBarrier(node, state, _)
289+
or
290+
isSink2(node, state)
291+
}
292+
293+
additional predicate isUpperBoundCheckBarrier(
294+
DataFlow::Node node, FlowState state, UpperBoundCheckGuard g
295+
) {
279296
// To catch flows that only happen on 32-bit architectures we consider an
280297
// architecture-dependent sink bit size to be 32.
281-
exists(UpperBoundCheckGuard g, int effectiveSinkBitSize |
282-
effectiveSinkBitSize = replaceZeroWith(state.getBitSize(), 32)
298+
exists(int effectiveSinkBitSize |
299+
effectiveSinkBitSize = state.getConcreteBitsize() or
300+
effectiveSinkBitSize = f(0, state.getArchictureDependentSignedness(), 32)
283301
|
284302
node = DataFlow::BarrierGuard<upperBoundCheckGuard/3>::getABarrierNodeForGuard(g) and
285-
g.isBoundFor(effectiveSinkBitSize, state.getIsSigned())
303+
// use `false` for `isSigned` as we have already subtracted 1 from
304+
// `effectiveSinkBitSize` if necessary
305+
g.isBoundFor2(effectiveSinkBitSize)
306+
)
307+
}
308+
309+
predicate isAdditionalFlowStep(
310+
DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2
311+
) {
312+
exists(UpperBoundCheckGuard g |
313+
isUpperBoundCheckBarrier(node1, state1, g) and
314+
node2 = node1.getASuccessor() and
315+
state2.getConcreteBitsize() =
316+
max(int bitsize |
317+
bitsize = validBitsize() and
318+
bitsize < state1.getConcreteBitsize() and
319+
not g.isBoundFor2(bitsize)
320+
|
321+
bitsize
322+
)
286323
)
287-
or
288-
isSink2(node, state)
289324
}
290325
}
291326

@@ -337,6 +372,33 @@ class UpperBoundCheckGuard extends DataFlow::RelationalComparisonNode {
337372
)
338373
}
339374

375+
predicate isBoundFor2(int bitSize) {
376+
bitSize = [7, 8, 15, 16, 31, 32, 63, 64] and
377+
exists(float bound, float strictnessOffset |
378+
// For `x < c` the bound is `c-1`. For `x >= c` we will be an upper bound
379+
// on the `branch` argument of `checks` is false, which is equivalent to
380+
// `x < c`.
381+
if expr instanceof LssExpr or expr instanceof GeqExpr
382+
then strictnessOffset = 1
383+
else strictnessOffset = 0
384+
|
385+
(
386+
bound = expr.getAnOperand().getExactValue().toFloat()
387+
or
388+
exists(DeclaredConstant maxint | maxint.hasQualifiedName("math", "MaxInt") |
389+
expr.getAnOperand() = maxint.getAReference() and
390+
bound = getMaxIntValue(32, true)
391+
)
392+
or
393+
exists(DeclaredConstant maxuint | maxuint.hasQualifiedName("math", "MaxUint") |
394+
expr.getAnOperand() = maxuint.getAReference() and
395+
bound = getMaxIntValue(32, false)
396+
)
397+
) and
398+
bound - strictnessOffset <= 2.pow(bitSize) - 1
399+
)
400+
}
401+
340402
/** Holds if this guard validates `e` upon evaluating to `branch`. */
341403
predicate checks(Expr e, boolean branch) {
342404
this.leq(branch, DataFlow::exprNode(e), _, _) and

‎go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.expected‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,2 @@
11
failures
22
testFailures
3-
| IncorrectIntegerConversion.go:339:23:339:67 | comment | Fixed spurious result:hasValueFlow="type conversion" |

‎go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -336,15 +336,15 @@ func testPathWithMoreThanOneSink(input string) {
336336
panic(err)
337337
}
338338
v1 := int16(parsed) // $ hasValueFlow="type conversion"
339-
_ = int16(v1) // $ SPURIOUS: hasValueFlow="type conversion"
339+
_ = int16(v1)
340340
}
341341
{
342342
parsed, err := strconv.ParseInt(input, 10, 32)
343343
if err != nil {
344344
panic(err)
345345
}
346346
v := int16(parsed) // $ hasValueFlow="type conversion"
347-
_ = int8(v) // $ SPURIOUS: hasValueFlow="type conversion"
347+
_ = int8(v)
348348
}
349349
{
350350
parsed, err := strconv.ParseInt(input, 10, 32)
@@ -353,7 +353,7 @@ func testPathWithMoreThanOneSink(input string) {
353353
}
354354
v1 := int32(parsed)
355355
v2 := int16(v1) // $ hasValueFlow="type conversion"
356-
_ = int8(v2) // $ SPURIOUS: hasValueFlow="type conversion"
356+
_ = int8(v2)
357357
}
358358
{
359359
parsed, err := strconv.ParseInt(input, 10, 16)

0 commit comments

Comments
 (0)