From 18e5d3cce84ac9da8cccc1beef9dd35b0b4f6a52 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 21 Jun 2021 14:02:06 +0200 Subject: [PATCH 1/3] C++: Add false positive with multiplication. --- .../uncontrolled/ArithmeticUncontrolled.expected | 13 +++++++++++++ .../CWE/CWE-190/semmle/uncontrolled/test.c | 14 ++++++++++++-- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/uncontrolled/ArithmeticUncontrolled.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/uncontrolled/ArithmeticUncontrolled.expected index 097efb73b9fb..96200f469f5c 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/uncontrolled/ArithmeticUncontrolled.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/uncontrolled/ArithmeticUncontrolled.expected @@ -19,6 +19,11 @@ edges | test.c:99:14:99:19 | call to rand | test.c:100:5:100:5 | r | | test.c:99:14:99:19 | call to rand | test.c:100:5:100:5 | r | | test.c:99:14:99:19 | call to rand | test.c:100:5:100:5 | r | +| test.c:104:13:104:16 | call to rand | test.c:106:5:106:11 | r | +| test.c:104:13:104:16 | call to rand | test.c:106:5:106:11 | r | +| test.c:106:5:106:11 | r | test.c:110:18:110:18 | r | +| test.c:110:18:110:18 | r | test.c:111:3:111:3 | r | +| test.c:110:18:110:18 | r | test.c:111:3:111:3 | r | | test.cpp:8:9:8:12 | Store | test.cpp:24:11:24:18 | call to get_rand | | test.cpp:8:9:8:12 | call to rand | test.cpp:8:9:8:12 | Store | | test.cpp:8:9:8:12 | call to rand | test.cpp:8:9:8:12 | Store | @@ -62,6 +67,13 @@ nodes | test.c:100:5:100:5 | r | semmle.label | r | | test.c:100:5:100:5 | r | semmle.label | r | | test.c:100:5:100:5 | r | semmle.label | r | +| test.c:104:13:104:16 | call to rand | semmle.label | call to rand | +| test.c:104:13:104:16 | call to rand | semmle.label | call to rand | +| test.c:106:5:106:11 | r | semmle.label | r | +| test.c:110:18:110:18 | r | semmle.label | r | +| test.c:111:3:111:3 | r | semmle.label | r | +| test.c:111:3:111:3 | r | semmle.label | r | +| test.c:111:3:111:3 | r | semmle.label | r | | test.cpp:8:9:8:12 | Store | semmle.label | Store | | test.cpp:8:9:8:12 | call to rand | semmle.label | call to rand | | test.cpp:8:9:8:12 | call to rand | semmle.label | call to rand | @@ -93,6 +105,7 @@ nodes | test.c:45:5:45:5 | r | test.c:44:13:44:16 | call to rand | test.c:45:5:45:5 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:44:13:44:16 | call to rand | Uncontrolled value | | test.c:77:9:77:9 | r | test.c:75:13:75:19 | ... ^ ... | test.c:77:9:77:9 | r | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:75:13:75:19 | ... ^ ... | Uncontrolled value | | test.c:100:5:100:5 | r | test.c:99:14:99:19 | call to rand | test.c:100:5:100:5 | r | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:99:14:99:19 | call to rand | Uncontrolled value | +| test.c:111:3:111:3 | r | test.c:104:13:104:16 | call to rand | test.c:111:3:111:3 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:104:13:104:16 | call to rand | Uncontrolled value | | test.cpp:25:7:25:7 | r | test.cpp:8:9:8:12 | call to rand | test.cpp:25:7:25:7 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:8:9:8:12 | call to rand | Uncontrolled value | | test.cpp:31:7:31:7 | r | test.cpp:13:10:13:13 | call to rand | test.cpp:31:7:31:7 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:13:10:13:13 | call to rand | Uncontrolled value | | test.cpp:37:7:37:7 | r | test.cpp:18:9:18:12 | call to rand | test.cpp:37:7:37:7 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:18:9:18:12 | call to rand | Uncontrolled value | 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 61f39a8e8519..eb89dfd42e4a 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 @@ -3,12 +3,12 @@ int rand(void); void trySlice(int start, int end); +void add_100(int); #define RAND() rand() #define RANDN(n) (rand() % n) #define RAND2() (rand() ^ rand()) - - +#define RAND_MAX 32767 @@ -99,4 +99,14 @@ void randomTester() { *ptr_r = RAND(); r -= 100; // BAD } + + { + int r = rand(); + r = ((2.0 / (RAND_MAX + 1)) * r - 1.0); + add_100(r); + } } + +void add_100(int r) { + r += 100; // GOOD [FALSE POSITIVE] +} \ No newline at end of file From 238c483e5b35ca0575e565168c03cb94ccb84841 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 21 Jun 2021 14:05:34 +0200 Subject: [PATCH 2/3] C++: Make any non-overflowing arithmetic operation a barrier. --- .../Security/CWE/CWE-190/ArithmeticUncontrolled.ql | 11 ++++++++--- .../uncontrolled/ArithmeticUncontrolled.expected | 13 ------------- .../Security/CWE/CWE-190/semmle/uncontrolled/test.c | 2 +- 3 files changed, 9 insertions(+), 17 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql b/cpp/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql index 359ac7a0d1a0..6b9801a5fb22 100644 --- a/cpp/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql +++ b/cpp/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql @@ -36,7 +36,7 @@ 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 + * an `AssignRemExpr`) with left-hand side `left` and right-hand 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`. */ @@ -59,10 +59,15 @@ predicate boundedBitwiseAnd(Expr e, Expr andExpr, Expr operand1, Expr operand2) } /** - * Holds if `fc` is a part of the left operand of a binary operation that greatly reduces the range - * of possible values. + * Holds if `e` is an operand of an operation that greatly reduces the range of possible values. */ predicate bounded(Expr e) { + ( + e instanceof UnaryArithmeticOperation or + e instanceof BinaryArithmeticOperation + ) 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: diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/uncontrolled/ArithmeticUncontrolled.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/uncontrolled/ArithmeticUncontrolled.expected index 96200f469f5c..097efb73b9fb 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/uncontrolled/ArithmeticUncontrolled.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/uncontrolled/ArithmeticUncontrolled.expected @@ -19,11 +19,6 @@ edges | test.c:99:14:99:19 | call to rand | test.c:100:5:100:5 | r | | test.c:99:14:99:19 | call to rand | test.c:100:5:100:5 | r | | test.c:99:14:99:19 | call to rand | test.c:100:5:100:5 | r | -| test.c:104:13:104:16 | call to rand | test.c:106:5:106:11 | r | -| test.c:104:13:104:16 | call to rand | test.c:106:5:106:11 | r | -| test.c:106:5:106:11 | r | test.c:110:18:110:18 | r | -| test.c:110:18:110:18 | r | test.c:111:3:111:3 | r | -| test.c:110:18:110:18 | r | test.c:111:3:111:3 | r | | test.cpp:8:9:8:12 | Store | test.cpp:24:11:24:18 | call to get_rand | | test.cpp:8:9:8:12 | call to rand | test.cpp:8:9:8:12 | Store | | test.cpp:8:9:8:12 | call to rand | test.cpp:8:9:8:12 | Store | @@ -67,13 +62,6 @@ nodes | test.c:100:5:100:5 | r | semmle.label | r | | test.c:100:5:100:5 | r | semmle.label | r | | test.c:100:5:100:5 | r | semmle.label | r | -| test.c:104:13:104:16 | call to rand | semmle.label | call to rand | -| test.c:104:13:104:16 | call to rand | semmle.label | call to rand | -| test.c:106:5:106:11 | r | semmle.label | r | -| test.c:110:18:110:18 | r | semmle.label | r | -| test.c:111:3:111:3 | r | semmle.label | r | -| test.c:111:3:111:3 | r | semmle.label | r | -| test.c:111:3:111:3 | r | semmle.label | r | | test.cpp:8:9:8:12 | Store | semmle.label | Store | | test.cpp:8:9:8:12 | call to rand | semmle.label | call to rand | | test.cpp:8:9:8:12 | call to rand | semmle.label | call to rand | @@ -105,7 +93,6 @@ nodes | test.c:45:5:45:5 | r | test.c:44:13:44:16 | call to rand | test.c:45:5:45:5 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:44:13:44:16 | call to rand | Uncontrolled value | | test.c:77:9:77:9 | r | test.c:75:13:75:19 | ... ^ ... | test.c:77:9:77:9 | r | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:75:13:75:19 | ... ^ ... | Uncontrolled value | | test.c:100:5:100:5 | r | test.c:99:14:99:19 | call to rand | test.c:100:5:100:5 | r | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:99:14:99:19 | call to rand | Uncontrolled value | -| test.c:111:3:111:3 | r | test.c:104:13:104:16 | call to rand | test.c:111:3:111:3 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:104:13:104:16 | call to rand | Uncontrolled value | | test.cpp:25:7:25:7 | r | test.cpp:8:9:8:12 | call to rand | test.cpp:25:7:25:7 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:8:9:8:12 | call to rand | Uncontrolled value | | test.cpp:31:7:31:7 | r | test.cpp:13:10:13:13 | call to rand | test.cpp:31:7:31:7 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:13:10:13:13 | call to rand | Uncontrolled value | | test.cpp:37:7:37:7 | r | test.cpp:18:9:18:12 | call to rand | test.cpp:37:7:37:7 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:18:9:18:12 | call to rand | Uncontrolled value | 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 eb89dfd42e4a..127266f2a6a8 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 @@ -108,5 +108,5 @@ void randomTester() { } void add_100(int r) { - r += 100; // GOOD [FALSE POSITIVE] + r += 100; // GOOD } \ No newline at end of file From a611e76ed206ed3593a1e50a9642c003761593c9 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Wed, 23 Jun 2021 10:28:00 +0200 Subject: [PATCH 3/3] C++: Respond to review comments. --- cpp/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql b/cpp/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql index 6b9801a5fb22..5bd9d3244021 100644 --- a/cpp/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql +++ b/cpp/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql @@ -59,12 +59,14 @@ predicate boundedBitwiseAnd(Expr e, Expr andExpr, Expr operand1, Expr operand2) } /** - * Holds if `e` is an operand of an operation that greatly reduces the range of possible values. + * Holds if `e` is an arithmetic expression that cannot overflow, or if `e` is an operand of an + * operation that may greatly reduces the range of possible values. */ predicate bounded(Expr e) { ( e instanceof UnaryArithmeticOperation or - e instanceof BinaryArithmeticOperation + e instanceof BinaryArithmeticOperation or + e instanceof AssignArithmeticOperation ) and not convertedExprMightOverflow(e) or @@ -90,7 +92,7 @@ predicate bounded(Expr e) { boundedBitwiseAnd(e, andExpr, andExpr.getAnOperand(), andExpr.getAnOperand()) ) or - // Optimitically assume that a division always yields a much smaller value. + // Optimitically assume that a division or right shift always yields a much smaller value. boundedDiv(e, any(DivExpr div).getLeftOperand()) or boundedDiv(e, any(AssignDivExpr div).getLValue())