From 32d7faa3b82bcb261da5f99f5b3710b93b378343 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Mon, 19 Jun 2023 16:57:36 -0700 Subject: [PATCH] Account for the signedness of the lesser operand --- .../CWE/CWE-190/ComparisonWithWiderType.ql | 11 +++++++++- .../ComparisonWithWiderType.expected | 2 ++ .../semmle/ComparisonWithWiderType/test.c | 21 ++++++++++++++++++- 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql b/cpp/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql index 6636d100746..5bbc04746e1 100644 --- a/cpp/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql +++ b/cpp/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql @@ -45,13 +45,22 @@ Element friendlyLoc(Expr e) { not e instanceof Access and not e instanceof Call and result = e } +int getComparisonSizeAdjustment(Expr e) { + if e.getType().(IntegralType).isSigned() + then result = 1 + else result = 0 +} + from Loop l, RelationalOperation rel, VariableAccess small, Expr large where small = rel.getLesserOperand() and large = rel.getGreaterOperand() and rel = l.getCondition().getAChild*() and forall(Expr conv | conv = large.getConversion*() | - upperBound(conv).log2() > getComparisonSize(small) * 8 + // 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)) ) 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/test/query-tests/Security/CWE/CWE-190/semmle/ComparisonWithWiderType/ComparisonWithWiderType.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/ComparisonWithWiderType/ComparisonWithWiderType.expected index d04bff0a812..fb46cacf4a9 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/ComparisonWithWiderType/ComparisonWithWiderType.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/ComparisonWithWiderType/ComparisonWithWiderType.expected @@ -1,6 +1,7 @@ | test.c:4:14:4:18 | ... < ... | Comparison between $@ of type char and $@ of wider type int. | test.c:3:7:3:7 | c | c | test.c:2:17:2:17 | x | x | | test.c:9:14:9:18 | ... > ... | Comparison between $@ of type char and $@ of wider type int. | test.c:8:7:8:7 | c | c | test.c:7:17:7:17 | x | x | | test.c:14:14:14:18 | ... < ... | Comparison between $@ of type short and $@ of wider type int. | test.c:13:8:13:8 | s | s | test.c:12:17:12:17 | x | x | +| test.c:42:15:42:29 | ... < ... | Comparison between $@ of type short and $@ of wider type int. | test.c:41:9:41:10 | s1 | s1 | test.c:42:20:42:29 | 65535 | 65535 | | test.c:65:14:65:18 | ... < ... | Comparison between $@ of type short and $@ of wider type int. | test.c:64:8:64:8 | s | s | test.c:63:17:63:17 | x | x | | test.c:87:14:87:18 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:83:16:83:16 | c | c | test.c:84:15:84:15 | x | x | | test.c:91:14:91:23 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type int. | test.c:83:16:83:16 | c | c | test.c:91:18:91:23 | 65280 | 65280 | @@ -13,3 +14,4 @@ | test.c:107:14:107:26 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:83:16:83:16 | c | c | test.c:107:19:107:25 | ... >> ... | ... >> ... | | test.c:128:15:128:21 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:121:16:121:17 | uc | uc | test.c:123:19:123:20 | sz | sz | | test.c:139:15:139:21 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:121:16:121:17 | uc | uc | test.c:123:19:123:20 | sz | sz | +| test.c:156:9:156:14 | ... < ... | Comparison between $@ of type short and $@ of wider type int. | test.c:150:8:150:8 | s | s | test.c:151:6:151:7 | sx | sx | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/ComparisonWithWiderType/test.c b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/ComparisonWithWiderType/test.c index f0b7f445aeb..8361ae3e31b 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/ComparisonWithWiderType/test.c +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/ComparisonWithWiderType/test.c @@ -39,7 +39,7 @@ void test5 () { void test6() { short s1; - for (s1 = 0; s1 < 0x0000ffff; s1++) {} + for (s1 = 0; s1 < 0x0000ffff; s1++) {} // BAD } void test7(long long l) { @@ -145,3 +145,22 @@ void test13() { sz = (unsigned)sx & (unsigned)sy; for (uc = 0; uc < sz; uc++) {} // GOOD } + +void test14() { + short s = 0; + int sx = 0x7FFF + 1; + + // BAD: 's' is compared with a value of a wider type. + // 's' overflows before reaching 'sx', + // causing an infinite loop + while (s < sx) { + s += 1; + } + + unsigned int ux = 0; + + // GOOD: 'ux' has a type at least as wide as 'max_get' + while (ux < sx) { + ux += 1; + } +}