From b7b6cd5f3eda546a4aa491af4f8d69b8760345e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= Date: Thu, 27 Jun 2024 12:44:42 +0200 Subject: [PATCH] SimpleRangeAnalysis src/: float -> BigInt --- cpp/ql/src/Critical/OverflowStatic.ql | 11 ++-- .../Likely Bugs/Arithmetic/IntMultToLong.ql | 32 ++++++----- .../Arithmetic/PointlessComparison.ql | 4 +- .../CWE-129/ImproperArrayIndexValidation.ql | 2 +- .../CWE/CWE-190/ComparisonWithWiderType.ql | 3 +- ...nsignedDifferenceExpressionComparedZero.ql | 18 +++---- .../Likely Bugs/DerefNullResult.cpp | 5 ++ .../Likely Bugs/RedundantNullCheckParam.cpp | 2 + .../CWE/CWE-561/FindIncorrectlyUsedSwitch.ql | 46 ++++++++-------- ...cErrorWhenUseBitwiseOrLogicalOperations.ql | 54 ++++++++++--------- 10 files changed, 98 insertions(+), 79 deletions(-) diff --git a/cpp/ql/src/Critical/OverflowStatic.ql b/cpp/ql/src/Critical/OverflowStatic.ql index 519c3f9b4015..9f471f3afc23 100644 --- a/cpp/ql/src/Critical/OverflowStatic.ql +++ b/cpp/ql/src/Critical/OverflowStatic.ql @@ -58,7 +58,8 @@ predicate overflowOffsetInLoop(BufferAccess bufaccess, string msg) { loop.limit() >= bufaccess.bufferSize() and loop.counter().getAnAccess() = bufaccess.getArrayOffset() and // Ensure that we don't have an upper bound on the array index that's less than the buffer size. - not upperBound(bufaccess.getArrayOffset().getFullyConverted()) < bufaccess.bufferSize() and + not upperBound(bufaccess.getArrayOffset().getFullyConverted()) < + bufaccess.bufferSize().toBigInt() and // The upper bounds analysis must not have been widended not upperBoundMayBeWidened(bufaccess.getArrayOffset().getFullyConverted()) and msg = @@ -103,7 +104,7 @@ class CallWithBufferSize extends FunctionCall { ) } - int statedSizeValue() { + QlBuiltins::BigInt statedSizeValue() { // `upperBound(e)` defaults to `exprMaxVal(e)` when `e` isn't analyzable. So to get a meaningful // result in this case we pick the minimum value obtainable from dataflow and range analysis. result = @@ -111,16 +112,16 @@ class CallWithBufferSize extends FunctionCall { .minimum(min(Expr statedSizeSrc | DataFlow::localExprFlow(statedSizeSrc, this.statedSizeExpr()) | - statedSizeSrc.getValue().toInt() + statedSizeSrc.getValue().toBigInt() )) } } predicate wrongBufferSize(Expr error, string msg) { - exists(CallWithBufferSize call, int bufsize, Variable buf, int statedSize | + exists(CallWithBufferSize call, int bufsize, Variable buf, QlBuiltins::BigInt statedSize | staticBuffer(call.buffer(), buf, bufsize) and statedSize = call.statedSizeValue() and - statedSize > bufsize and + statedSize > bufsize.toBigInt() and error = call.statedSizeExpr() and msg = "Potential buffer-overflow: '" + buf.getName() + "' has size " + bufsize.toString() + " not " + diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/IntMultToLong.ql b/cpp/ql/src/Likely Bugs/Arithmetic/IntMultToLong.ql index ba7a6b58aa01..29cd160cad4f 100644 --- a/cpp/ql/src/Likely Bugs/Arithmetic/IntMultToLong.ql +++ b/cpp/ql/src/Likely Bugs/Arithmetic/IntMultToLong.ql @@ -76,20 +76,22 @@ int getEffectiveMulOperands(MulExpr me) { * using SimpleRangeAnalysis. */ class AnalyzableExpr extends Expr { - float maxValue() { result = upperBound(this.getFullyConverted()) } + QlBuiltins::BigInt maxValue() { result = upperBound(this.getFullyConverted()) } - float minValue() { result = lowerBound(this.getFullyConverted()) } + QlBuiltins::BigInt minValue() { result = lowerBound(this.getFullyConverted()) } } class ParenAnalyzableExpr extends AnalyzableExpr, ParenthesisExpr { - override float maxValue() { result = this.getExpr().(AnalyzableExpr).maxValue() } + override QlBuiltins::BigInt maxValue() { result = this.getExpr().(AnalyzableExpr).maxValue() } - override float minValue() { result = this.getExpr().(AnalyzableExpr).minValue() } + override QlBuiltins::BigInt minValue() { result = this.getExpr().(AnalyzableExpr).minValue() } } class MulAnalyzableExpr extends AnalyzableExpr, MulExpr { - override float maxValue() { - exists(float x1, float y1, float x2, float y2 | + override QlBuiltins::BigInt maxValue() { + exists( + QlBuiltins::BigInt x1, QlBuiltins::BigInt y1, QlBuiltins::BigInt x2, QlBuiltins::BigInt y2 + | x1 = this.getLeftOperand().getFullyConverted().(AnalyzableExpr).minValue() and x2 = this.getLeftOperand().getFullyConverted().(AnalyzableExpr).maxValue() and y1 = this.getRightOperand().getFullyConverted().(AnalyzableExpr).minValue() and @@ -98,8 +100,10 @@ class MulAnalyzableExpr extends AnalyzableExpr, MulExpr { ) } - override float minValue() { - exists(float x1, float x2, float y1, float y2 | + override QlBuiltins::BigInt minValue() { + exists( + QlBuiltins::BigInt x1, QlBuiltins::BigInt x2, QlBuiltins::BigInt y1, QlBuiltins::BigInt y2 + | x1 = this.getLeftOperand().getFullyConverted().(AnalyzableExpr).minValue() and x2 = this.getLeftOperand().getFullyConverted().(AnalyzableExpr).maxValue() and y1 = this.getRightOperand().getFullyConverted().(AnalyzableExpr).minValue() and @@ -110,13 +114,13 @@ class MulAnalyzableExpr extends AnalyzableExpr, MulExpr { } class AddAnalyzableExpr extends AnalyzableExpr, AddExpr { - override float maxValue() { + override QlBuiltins::BigInt maxValue() { result = this.getLeftOperand().getFullyConverted().(AnalyzableExpr).maxValue() + this.getRightOperand().getFullyConverted().(AnalyzableExpr).maxValue() } - override float minValue() { + override QlBuiltins::BigInt minValue() { result = this.getLeftOperand().getFullyConverted().(AnalyzableExpr).minValue() + this.getRightOperand().getFullyConverted().(AnalyzableExpr).minValue() @@ -124,13 +128,13 @@ class AddAnalyzableExpr extends AnalyzableExpr, AddExpr { } class SubAnalyzableExpr extends AnalyzableExpr, SubExpr { - override float maxValue() { + override QlBuiltins::BigInt maxValue() { result = this.getLeftOperand().getFullyConverted().(AnalyzableExpr).maxValue() - this.getRightOperand().getFullyConverted().(AnalyzableExpr).minValue() } - override float minValue() { + override QlBuiltins::BigInt minValue() { result = this.getLeftOperand().getFullyConverted().(AnalyzableExpr).minValue() - this.getRightOperand().getFullyConverted().(AnalyzableExpr).maxValue() @@ -140,7 +144,7 @@ class SubAnalyzableExpr extends AnalyzableExpr, SubExpr { class VarAnalyzableExpr extends AnalyzableExpr, VariableAccess { VarAnalyzableExpr() { this.getTarget() instanceof StackVariable } - override float maxValue() { + override QlBuiltins::BigInt maxValue() { exists(SsaDefinition def, Variable v | def.getAUse(v) = this and // if there is a defining expression, use that for @@ -152,7 +156,7 @@ class VarAnalyzableExpr extends AnalyzableExpr, VariableAccess { ) } - override float minValue() { + override QlBuiltins::BigInt minValue() { exists(SsaDefinition def, Variable v | def.getAUse(v) = this and if exists(def.getDefiningValue(v)) diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/PointlessComparison.ql b/cpp/ql/src/Likely Bugs/Arithmetic/PointlessComparison.ql index e2fe02be867f..991fc258d30b 100644 --- a/cpp/ql/src/Likely Bugs/Arithmetic/PointlessComparison.ql +++ b/cpp/ql/src/Likely Bugs/Arithmetic/PointlessComparison.ql @@ -26,7 +26,9 @@ import UnsignedGEZero // So to reduce the number of false positives, we do not report a result if // the comparison is in a macro expansion. Similarly for template // instantiations. -from ComparisonOperation cmp, SmallSide ss, float left, float right, boolean value, string reason +from + ComparisonOperation cmp, SmallSide ss, QlBuiltins::BigInt left, QlBuiltins::BigInt right, + boolean value, string reason where not cmp.isInMacroExpansion() and not cmp.isFromTemplateInstantiation(_) and diff --git a/cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql b/cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql index b5dc4d893b21..255a55390793 100644 --- a/cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql +++ b/cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql @@ -46,7 +46,7 @@ predicate guardChecks(IRGuardCondition g, Expr e, boolean branch) { predicate offsetIsAlwaysInBounds(ArrayExpr arrayExpr, VariableAccess offsetExpr) { exists(ArrayType arrayType | arrayType = arrayExpr.getArrayBase().getUnspecifiedType() and - arrayType.getArraySize() > upperBound(offsetExpr.getFullyConverted()) + arrayType.getArraySize().toBigInt() > upperBound(offsetExpr.getFullyConverted()) ) } diff --git a/cpp/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql b/cpp/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql index 7d9ef88adea1..846a727c1145 100644 --- a/cpp/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql +++ b/cpp/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql @@ -58,7 +58,8 @@ where // We adjust the comparison size in the case of a signed integer type. // This is to exclude the sign bit from the comparison that determines if the small type's size is sufficient to hold // the value of the larger type determined with range analysis. - upperBound(conv).log2() > (getComparisonSize(small) * 8 - getComparisonSizeAdjustment(small)) + upperBound(conv).toString().toFloat().log() / 2.log() > + (getComparisonSize(small) * 8 - getComparisonSizeAdjustment(small)) ) and // Ignore cases where the smaller type is int or larger // These are still bugs, but you should need a very large string or array to diff --git a/cpp/ql/src/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.ql b/cpp/ql/src/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.ql index 02de4223c185..185eb8452780 100644 --- a/cpp/ql/src/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.ql +++ b/cpp/ql/src/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.ql @@ -62,7 +62,7 @@ Expr exprIsLeftOrLessBase(SubExpr sub) { // result = e - y <= e result = s and s.getLeftOperand() = e and - lowerBound(s.getRightOperand().getFullyConverted()) >= 0 + lowerBound(s.getRightOperand().getFullyConverted()) >= 0.toBigInt() ) or exists(Expr other | @@ -73,7 +73,7 @@ Expr exprIsLeftOrLessBase(SubExpr sub) { // so: // result = e + y <= e + 0 = e result.(AddExpr).hasOperands(e, other) and - upperBound(other.getFullyConverted()) <= 0 + upperBound(other.getFullyConverted()) <= 0.toBigInt() ) or exists(DivExpr d | @@ -85,7 +85,7 @@ Expr exprIsLeftOrLessBase(SubExpr sub) { // result = e / y <= e / 1 = e result = d and d.getLeftOperand() = e and - lowerBound(d.getRightOperand().getFullyConverted()) >= 1 + lowerBound(d.getRightOperand().getFullyConverted()) >= 1.toBigInt() ) or exists(RShiftExpr rs | @@ -122,20 +122,20 @@ predicate exprIsSubLeftOrLess(SubExpr sub, DataFlow::Node n) { isGuarded(sub, other.asExpr(), n.asExpr()) // other >= n ) or - exists(DataFlow::Node other, float p, float q | + exists(DataFlow::Node other, QlBuiltins::BigInt p, QlBuiltins::BigInt q | // linear access of `other` exprIsSubLeftOrLess(sub, other) and linearAccess(n.asExpr(), other.asExpr(), p, q) and // n = p * other + q - p <= 1 and - q <= 0 + p <= 1.toBigInt() and + q <= 0.toBigInt() ) or - exists(DataFlow::Node other, float p, float q | + exists(DataFlow::Node other, QlBuiltins::BigInt p, QlBuiltins::BigInt q | // linear access of `n` exprIsSubLeftOrLess(sub, other) and linearAccess(other.asExpr(), n.asExpr(), p, q) and // other = p * n + q - p >= 1 and - q >= 0 + p >= 1.toBigInt() and + q >= 0.toBigInt() ) } diff --git a/cpp/ql/src/experimental/Likely Bugs/DerefNullResult.cpp b/cpp/ql/src/experimental/Likely Bugs/DerefNullResult.cpp index f96d67517b75..787019bb3abf 100644 --- a/cpp/ql/src/experimental/Likely Bugs/DerefNullResult.cpp +++ b/cpp/ql/src/experimental/Likely Bugs/DerefNullResult.cpp @@ -1,3 +1,8 @@ +#define NULL nullptr +char *malloc(int); +void printf(const char *, ...); +int snprintf(char *, int, const char *); + char * create (int arg) { if (arg > 42) { // this function may return NULL diff --git a/cpp/ql/src/experimental/Likely Bugs/RedundantNullCheckParam.cpp b/cpp/ql/src/experimental/Likely Bugs/RedundantNullCheckParam.cpp index 3765d0b14d40..7b9aa815d464 100644 --- a/cpp/ql/src/experimental/Likely Bugs/RedundantNullCheckParam.cpp +++ b/cpp/ql/src/experimental/Likely Bugs/RedundantNullCheckParam.cpp @@ -1,3 +1,5 @@ +#define NULL nullptr + void test(char *arg1, int *arg2) { if (arg1[0] == 'A') { if (arg2 != NULL) { //maybe redundant diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-561/FindIncorrectlyUsedSwitch.ql b/cpp/ql/src/experimental/Security/CWE/CWE-561/FindIncorrectlyUsedSwitch.ql index a643c3e3bf08..b006d773a248 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-561/FindIncorrectlyUsedSwitch.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-561/FindIncorrectlyUsedSwitch.ql @@ -28,32 +28,32 @@ predicate isRealRange(Expr exp) { lowerBound(exp).toString() != "-4294967296" and lowerBound(exp).toString() != "-Infinity" and lowerBound(exp).toString() != "NaN" and - upperBound(exp) != 2147483647 and - upperBound(exp) != 268435455 and - upperBound(exp) != 33554431 and - upperBound(exp) != 8388607 and - upperBound(exp) != 65535 and - upperBound(exp) != 32767 and - upperBound(exp) != 255 and - upperBound(exp) != 127 and - upperBound(exp) != 63 and - upperBound(exp) != 31 and - upperBound(exp) != 15 and - upperBound(exp) != 7 and - lowerBound(exp) != -2147483648 and - lowerBound(exp) != -268435456 and - lowerBound(exp) != -33554432 and - lowerBound(exp) != -8388608 and - lowerBound(exp) != -65536 and - lowerBound(exp) != -32768 and - lowerBound(exp) != -128 + upperBound(exp) != 2147483647.toBigInt() and + upperBound(exp) != 268435455.toBigInt() and + upperBound(exp) != 33554431.toBigInt() and + upperBound(exp) != 8388607.toBigInt() and + upperBound(exp) != 65535.toBigInt() and + upperBound(exp) != 32767.toBigInt() and + upperBound(exp) != 255.toBigInt() and + upperBound(exp) != 127.toBigInt() and + upperBound(exp) != 63.toBigInt() and + upperBound(exp) != 31.toBigInt() and + upperBound(exp) != 15.toBigInt() and + upperBound(exp) != 7.toBigInt() and + lowerBound(exp) != "-2147483648".toBigInt() and + lowerBound(exp) != -268435456.toBigInt() and + lowerBound(exp) != -33554432.toBigInt() and + lowerBound(exp) != -8388608.toBigInt() and + lowerBound(exp) != -65536.toBigInt() and + lowerBound(exp) != -32768.toBigInt() and + lowerBound(exp) != -128.toBigInt() } /** Holds if the range of values for the condition is less than the choices. */ predicate isNotAllSelected(SwitchStmt swtmp) { not swtmp.getExpr().isConstant() and - exists(int i | - i != 0 and + exists(QlBuiltins::BigInt i | + i != 0.toBigInt() and ( i = lowerBound(swtmp.getASwitchCase().getExpr()) and upperBound(swtmp.getExpr()) < i @@ -70,7 +70,7 @@ predicate isNotAllSelected(SwitchStmt swtmp) { /** Holds if the range of values for the condition is greater than the selection. */ predicate isConditionBig(SwitchStmt swtmp) { not swtmp.hasDefaultCase() and - not exists(int iu, int il | + not exists(QlBuiltins::BigInt iu, QlBuiltins::BigInt il | ( iu = upperBound(swtmp.getASwitchCase().getExpr()) or iu = upperBound(swtmp.getASwitchCase().getEndExpr()) @@ -130,7 +130,7 @@ from SwitchStmt sw, string msg where isRealRange(sw.getExpr()) and lowerBound(sw.getExpr()) != upperBound(sw.getExpr()) and - lowerBound(sw.getExpr()) != 0 and + lowerBound(sw.getExpr()) != 0.toBigInt() and not exists(Expr cexp | cexp = sw.getASwitchCase().getExpr() and not isRealRange(cexp) or diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBitwiseOrLogicalOperations.ql b/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBitwiseOrLogicalOperations.ql index 8bb1d0ba97bf..e0abfbefaf93 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBitwiseOrLogicalOperations.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBitwiseOrLogicalOperations.ql @@ -97,24 +97,24 @@ predicate isRealRange(Expr exp) { lowerBound(exp).toString() != "-4294967296" and lowerBound(exp).toString() != "-Infinity" and lowerBound(exp).toString() != "NaN" and - upperBound(exp) != 2147483647 and - upperBound(exp) != 268435455 and - upperBound(exp) != 33554431 and - upperBound(exp) != 8388607 and - upperBound(exp) != 65535 and - upperBound(exp) != 32767 and - upperBound(exp) != 255 and - upperBound(exp) != 127 and - lowerBound(exp) != -2147483648 and - lowerBound(exp) != -268435456 and - lowerBound(exp) != -33554432 and - lowerBound(exp) != -8388608 and - lowerBound(exp) != -65536 and - lowerBound(exp) != -32768 and - lowerBound(exp) != -128 + upperBound(exp) != 2147483647.toBigInt() and + upperBound(exp) != 268435455.toBigInt() and + upperBound(exp) != 33554431.toBigInt() and + upperBound(exp) != 8388607.toBigInt() and + upperBound(exp) != 65535.toBigInt() and + upperBound(exp) != 32767.toBigInt() and + upperBound(exp) != 255.toBigInt() and + upperBound(exp) != 127.toBigInt() and + lowerBound(exp) != "-2147483648".toBigInt() and + lowerBound(exp) != -268435456.toBigInt() and + lowerBound(exp) != -33554432.toBigInt() and + lowerBound(exp) != -8388608.toBigInt() and + lowerBound(exp) != -65536.toBigInt() and + lowerBound(exp) != -32768.toBigInt() and + lowerBound(exp) != -128.toBigInt() or - lowerBound(exp) = 0 and - upperBound(exp) = 1 + lowerBound(exp) = 0.toBigInt() and + upperBound(exp) = 1.toBigInt() } /** Holds if expressions are of different size or range */ @@ -128,11 +128,15 @@ predicate isDifferentSize(Expr exp1, Expr exp2, Expr exp3) { isRealRange(exp2) and isRealRange(exp3) ) and - upperBound(exp1).maximum(upperBound(exp2)) - upperBound(exp1).minimum(upperBound(exp2)) < 16 and - lowerBound(exp1).maximum(lowerBound(exp2)) - lowerBound(exp1).minimum(lowerBound(exp2)) < 16 and + upperBound(exp1).maximum(upperBound(exp2)) - upperBound(exp1).minimum(upperBound(exp2)) < + 16.toBigInt() and + lowerBound(exp1).maximum(lowerBound(exp2)) - lowerBound(exp1).minimum(lowerBound(exp2)) < + 16.toBigInt() and ( - upperBound(exp1).maximum(upperBound(exp3)) - upperBound(exp1).minimum(upperBound(exp3)) > 256 or - lowerBound(exp1).maximum(lowerBound(exp2)) - lowerBound(exp1).minimum(lowerBound(exp2)) > 256 + upperBound(exp1).maximum(upperBound(exp3)) - upperBound(exp1).minimum(upperBound(exp3)) > + 256.toBigInt() or + lowerBound(exp1).maximum(lowerBound(exp2)) - lowerBound(exp1).minimum(lowerBound(exp2)) > + 256.toBigInt() ) } @@ -146,10 +150,10 @@ predicate isDifferentResults( isRealRange(exp2) and isRealRange(exp3) ) and - exists(int i1, int i2, int i3 | - i1 in [lowerBound(exp1).floor() .. upperBound(exp1).floor()] and - i2 in [lowerBound(exp2).floor() .. upperBound(exp2).floor()] and - i3 in [lowerBound(exp3).floor() .. upperBound(exp3).floor()] and + exists(QlBuiltins::BigInt i1, QlBuiltins::BigInt i2, QlBuiltins::BigInt i3 | + i1 = lowerBound(exp1) + [0 .. (upperBound(exp1) - lowerBound(exp1)).toInt()].toBigInt() and + i2 = lowerBound(exp2) + [0 .. (upperBound(exp2) - lowerBound(exp2)).toInt()].toBigInt() and + i3 = lowerBound(exp3) + [0 .. (upperBound(exp3) - lowerBound(exp3)).toInt()].toBigInt() and ( op1 instanceof BitwiseOrExpr and op2 instanceof BitwiseAndExpr and