From 05d693e3bb68bbece3a966d5b6c3afce6ee5ead1 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 27 Apr 2021 09:41:13 +0200 Subject: [PATCH 1/3] C++: Also include the assignment versions in exprThatCanOverflow. --- .../src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll b/cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll index 2afe26bc3e0e..79dbe49611e0 100644 --- a/cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll +++ b/cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll @@ -1626,7 +1626,9 @@ private module SimpleRangeAnalysisCached { private predicate exprThatCanOverflow(Expr e) { e instanceof UnaryArithmeticOperation or e instanceof BinaryArithmeticOperation or - e instanceof LShiftExpr + e instanceof AssignArithmeticOperation or + e instanceof LShiftExpr or + e instanceof AssignLShiftExpr } /** From a41e9055c564088f7ba32357cc53e48beaf6d0a0 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 27 Apr 2021 09:43:02 +0200 Subject: [PATCH 2/3] C++: Delete the fix that was introduced in bb447d7174141dc518e6ce90367121280fec1d80. This is no longer needed after #5678. --- .../src/semmle/code/cpp/security/Overflow.qll | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/security/Overflow.qll b/cpp/ql/src/semmle/code/cpp/security/Overflow.qll index 465e64b9b6ca..6cf82791d526 100644 --- a/cpp/ql/src/semmle/code/cpp/security/Overflow.qll +++ b/cpp/ql/src/semmle/code/cpp/security/Overflow.qll @@ -99,12 +99,11 @@ VariableAccess varUse(LocalScopeVariable v) { result = v.getAnAccess() } * Holds if `e` potentially overflows and `use` is an operand of `e` that is not guarded. */ predicate missingGuardAgainstOverflow(Operation e, VariableAccess use) { - ( - convertedExprMightOverflowPositively(e) - or - // Ensure that the predicate holds when range analysis cannot determine an upper bound - upperBound(e.getFullyConverted()) = exprMaxVal(e.getFullyConverted()) - ) and + // Since `e` is guarenteed to be a `BinaryArithmeticOperation`, a `UnaryArithmeticOperation` or + // an `AssignArithmeticOperation` by the other constraints in this predicate, we know that + // `convertedExprMightOverflowPositively` will have a result even when `e` is not analyzable + // by `SimpleRangeAnalysis`. + convertedExprMightOverflowPositively(e) and use = e.getAnOperand() and exists(LocalScopeVariable v | use.getTarget() = v | // overflow possible if large @@ -126,12 +125,11 @@ predicate missingGuardAgainstOverflow(Operation e, VariableAccess use) { * Holds if `e` potentially underflows and `use` is an operand of `e` that is not guarded. */ predicate missingGuardAgainstUnderflow(Operation e, VariableAccess use) { - ( - convertedExprMightOverflowNegatively(e) - or - // Ensure that the predicate holds when range analysis cannot determine a lower bound - lowerBound(e.getFullyConverted()) = exprMinVal(e.getFullyConverted()) - ) and + // Since `e` is guarenteed to be a `BinaryArithmeticOperation`, a `UnaryArithmeticOperation` or + // an `AssignArithmeticOperation` by the other constraints in this predicate, we know that + // `convertedExprMightOverflowNegatively` will have a result even when `e` is not analyzable + // by `SimpleRangeAnalysis`. + convertedExprMightOverflowNegatively(e) and use = e.getAnOperand() and exists(LocalScopeVariable v | use.getTarget() = v | // underflow possible if use is left operand and small From 04a785b9fb438e61e2af3b4dc788930260b4f8df Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 27 Apr 2021 09:43:27 +0200 Subject: [PATCH 3/3] C++: Accept test changes. --- .../CWE-190/semmle/extreme/ArithmeticWithExtremeValues.expected | 1 - .../test/query-tests/Security/CWE/CWE-190/semmle/extreme/test.c | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/extreme/ArithmeticWithExtremeValues.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/extreme/ArithmeticWithExtremeValues.expected index 77b08e62d838..cbaf5d630795 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/extreme/ArithmeticWithExtremeValues.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/extreme/ArithmeticWithExtremeValues.expected @@ -3,5 +3,4 @@ | test.c:50:3:50:5 | sc3 | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:49:9:49:16 | 127 | Extreme value | | test.c:59:3:59:5 | sc6 | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:58:9:58:16 | 127 | Extreme value | | test.c:63:3:63:5 | sc8 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:62:9:62:16 | - ... | Extreme value | -| test.c:75:3:75:5 | sc1 | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:74:9:74:16 | 127 | Extreme value | | test.c:124:9:124:9 | x | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:118:17:118:23 | 2147483647 | Extreme value | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/extreme/test.c b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/extreme/test.c index ee2e041db3c3..8760641c8e2d 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/extreme/test.c +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/extreme/test.c @@ -72,7 +72,7 @@ void test_negatives() { signed char sc1, sc2, sc3, sc4, sc5, sc6, sc7, sc8; sc1 = CHAR_MAX; - sc1 += 0; // GOOD [FALSE POSITIVE] + sc1 += 0; // GOOD sc1 += -1; // GOOD sc2 = CHAR_MIN; sc2 += -1; // BAD [NOT DETECTED]