diff --git a/cpp/ql/src/Security/CWE/CWE-190/Bounded.qll b/cpp/ql/src/Security/CWE/CWE-190/Bounded.qll index 9f872304021d..b6b0d608d2ae 100644 --- a/cpp/ql/src/Security/CWE/CWE-190/Bounded.qll +++ b/cpp/ql/src/Security/CWE/CWE-190/Bounded.qll @@ -7,25 +7,6 @@ private import cpp private import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis private import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils -/** - * An operand `e` of a division expression (i.e., `e` is an operand of either a `DivExpr` or - * a `AssignDivExpr`) is bounded when `e` is the left-hand side of the division. - */ -pragma[inline] -private predicate boundedDiv(Expr e, Expr left) { e = left } - -/** - * An operand `e` of a remainder expression `rem` (i.e., `rem` is either a `RemExpr` or - * an `AssignRemExpr`) with left-hand side `left` and right-ahnd side `right` is bounded - * when `e` is `left` and `right` is upper bounded by some number that is less than the maximum integer - * allowed by the result type of `rem`. - */ -pragma[inline] -private predicate boundedRem(Expr e, Expr rem, Expr left, Expr right) { - e = left and - upperBound(right.getFullyConverted()) < exprMaxVal(rem.getFullyConverted()) -} - /** * An operand `e` of a bitwise and expression `andExpr` (i.e., `andExpr` is either an `BitwiseAndExpr` * or an `AssignAndExpr`) with operands `operand1` and `operand2` is the operand that is not `e` is upper @@ -50,19 +31,10 @@ predicate bounded(Expr e) { ) and not convertedExprMightOverflow(e) or - // For `%` and `&` we require that `e` is bounded by a value that is strictly smaller than the - // maximum possible value of the result type of the operation. - // For example, the function call `rand()` is considered bounded in the following program: - // ``` - // int i = rand() % (UINT8_MAX + 1); - // ``` - // but not in: - // ``` - // unsigned char uc = rand() % (UINT8_MAX + 1); - // ``` - exists(RemExpr rem | boundedRem(e, rem, rem.getLeftOperand(), rem.getRightOperand())) + // Optimitically assume that a remainder expression always yields a much smaller value. + e = any(RemExpr rem).getLeftOperand() or - exists(AssignRemExpr rem | boundedRem(e, rem, rem.getLValue(), rem.getRValue())) + e = any(AssignRemExpr rem).getLValue() or exists(BitwiseAndExpr andExpr | boundedBitwiseAnd(e, andExpr, andExpr.getAnOperand(), andExpr.getAnOperand()) @@ -73,11 +45,11 @@ predicate bounded(Expr e) { ) or // Optimitically assume that a division always yields a much smaller value. - boundedDiv(e, any(DivExpr div).getLeftOperand()) + e = any(DivExpr div).getLeftOperand() or - boundedDiv(e, any(AssignDivExpr div).getLValue()) + e = any(AssignDivExpr div).getLValue() or - boundedDiv(e, any(RShiftExpr shift).getLeftOperand()) + e = any(RShiftExpr shift).getLeftOperand() or - boundedDiv(e, any(AssignRShiftExpr div).getLValue()) + e = any(AssignRShiftExpr div).getLValue() } diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/uncontrolled/test.c b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/uncontrolled/test.c index 415d9dfd31b1..bc74725b083f 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/uncontrolled/test.c +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/uncontrolled/test.c @@ -109,4 +109,13 @@ void randomTester() { void add_100(int r) { r += 100; // GOOD -} \ No newline at end of file +} + +void randomTester2(int bound, int min, int max) { + int r1 = rand() % bound; + r1 += 100; // GOOD (`bound` may possibly be MAX_INT in which case this could + // still overflow, but it's most likely fine) + + int r2 = (rand() % (max - min + 1)) + min; + r2 += 100; // GOOD (This is a common way to clamp the random value between [min, max]) +}