+This rule finds relational comparisons between the result of an unsigned subtraction and the value 0.
+Such comparisons are likely to be wrong as the value of an unsigned subtraction can never be negative. So the
+relational comparison ends up checking whether the result of the subtraction is equal to 0.
+This is probably not what the programmer intended.
+
If a relational comparison is intended, consider casting the result of the subtraction to a signed type. + If the intention was to test for equality, consider replacing the relational comparison with an equality test. +
+ +The code compares the unsigned difference with zero. -It is highly probable that the condition is wrong if the difference expression has the unsigned type. -The condition holds in all the cases when difference is not equal to zero. -It means that we may use condition not equal. But the programmer probably wanted to compare the difference of elements.
- -False positives include code in which the first difference element is always greater than or equal to the second. -For comparison, ">" such conditions are equivalent to "! =", And are recommended for replacement. -For comparison "> =", the conditions are always true and are recommended to be excluded.
- -Use a simple comparison of two elements, instead of comparing their difference to zero.
- -The following example demonstrates an erroneous and corrected use of comparison.
-When using the new operator to allocate memory, you need to pay attention to the different ways of detecting errors. ::operator new(std::size_t) throws an exception on error, whereas ::operator new(std::size_t, const std::nothrow_t &) returns zero on error. The programmer can get confused and check the error that occurs when allocating memory incorrectly. That can lead to an unhandled program termination or to a violation of the program logic.
Use the correct error detection method corresponding with the memory allocation.
+ +The following example demonstrates various approaches to detecting memory allocation errors using the new operator.
Potentially dangerous use of the strlen function to calculate the length of a string.
+The expression buffer[strlen(buffer)] = 0 is potentially dangerous, if the variable buffer does not have a terminal zero, then access beyond the bounds of the allocated memory is possible, which will lead to undefined behavior.
+If terminal zero is present, then the specified expression is meaningless.
False positives include heavily nested strlen. This situation is unlikely.
+ +We recommend using another method for calculating the string length
+ +The following example demonstrates an erroneous and corrected use of the strlen function.
+Query results for "); // GOOD: Escape HTML characters before adding to a page char* query_escaped = escape_html(query); @@ -37,14 +37,25 @@ void good_server(char* query) { puts(do_search(query)); } -int main(int argc, char** argv) { - char* raw_query = getenv("QUERY_STRING"); - if (strcmp("good", argv[0]) == 0) { - good_server(raw_query); - } else if (strcmp("bad1", argv[0]) == 0) { - bad_server1(raw_query); - } else { - bad_server2(raw_query); - } +int sscanf(const char *s, const char *format, ...); + +void good_server2(char* query) { + puts("
Query results for "); + // GOOD: Only an integer is added to the page. + int i = 0; + sscanf(query, "value=%i", &i); + printf("\n
%i
\n", i); } +int main(int argc, char** argv) { + char* raw_query = getenv("QUERY_STRING"); + if (strcmp("good1", argv[0]) == 0) { + good_server1(raw_query); + } else if (strcmp("bad1", argv[0]) == 0) { + bad_server1(raw_query); + } else if (strcmp("bad2", argv[0]) == 0) { + bad_server2(raw_query); + } else if (strcmp("good2", argv[0]) == 0) { + good_server2(raw_query); + } +} diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected index 0299afff063..9876b9695ad 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected @@ -59,21 +59,18 @@ edges | test.cpp:227:24:227:37 | (const char *)... | test.cpp:229:9:229:18 | local_size | | test.cpp:241:2:241:32 | Chi [array content] | test.cpp:279:17:279:20 | get_size output argument [array content] | | test.cpp:241:2:241:32 | Chi [array content] | test.cpp:295:18:295:21 | get_size output argument [array content] | -| test.cpp:241:2:241:32 | ChiTotal [post update] [array content] | test.cpp:241:2:241:32 | Chi [array content] | -| test.cpp:241:2:241:32 | ChiTotal [post update] [array content] | test.cpp:279:17:279:20 | get_size output argument [array content] | -| test.cpp:241:2:241:32 | ChiTotal [post update] [array content] | test.cpp:295:18:295:21 | get_size output argument [array content] | -| test.cpp:241:18:241:23 | call to getenv | test.cpp:241:2:241:32 | ChiTotal [post update] [array content] | -| test.cpp:241:18:241:31 | (const char *)... | test.cpp:241:2:241:32 | ChiTotal [post update] [array content] | +| test.cpp:241:18:241:23 | call to getenv | test.cpp:241:2:241:32 | Chi [array content] | +| test.cpp:241:18:241:31 | (const char *)... | test.cpp:241:2:241:32 | Chi [array content] | | test.cpp:249:20:249:25 | call to getenv | test.cpp:253:11:253:29 | ... * ... | | test.cpp:249:20:249:25 | call to getenv | test.cpp:253:11:253:29 | ... * ... | | test.cpp:249:20:249:33 | (const char *)... | test.cpp:253:11:253:29 | ... * ... | | test.cpp:249:20:249:33 | (const char *)... | test.cpp:253:11:253:29 | ... * ... | -| test.cpp:279:17:279:20 | get_size output argument | test.cpp:281:11:281:28 | ... * ... | -| test.cpp:279:17:279:20 | get_size output argument | test.cpp:281:11:281:28 | ... * ... | -| test.cpp:279:17:279:20 | get_size output argument [array content] | test.cpp:279:17:279:20 | get_size output argument | -| test.cpp:295:18:295:21 | get_size output argument | test.cpp:298:10:298:27 | ... * ... | -| test.cpp:295:18:295:21 | get_size output argument | test.cpp:298:10:298:27 | ... * ... | -| test.cpp:295:18:295:21 | get_size output argument [array content] | test.cpp:295:18:295:21 | get_size output argument | +| test.cpp:279:17:279:20 | Chi | test.cpp:281:11:281:28 | ... * ... | +| test.cpp:279:17:279:20 | Chi | test.cpp:281:11:281:28 | ... * ... | +| test.cpp:279:17:279:20 | get_size output argument [array content] | test.cpp:279:17:279:20 | Chi | +| test.cpp:295:18:295:21 | Chi | test.cpp:298:10:298:27 | ... * ... | +| test.cpp:295:18:295:21 | Chi | test.cpp:298:10:298:27 | ... * ... | +| test.cpp:295:18:295:21 | get_size output argument [array content] | test.cpp:295:18:295:21 | Chi | | test.cpp:301:19:301:24 | call to getenv | test.cpp:305:11:305:28 | ... * ... | | test.cpp:301:19:301:24 | call to getenv | test.cpp:305:11:305:28 | ... * ... | | test.cpp:301:19:301:32 | (const char *)... | test.cpp:305:11:305:28 | ... * ... | @@ -147,7 +144,6 @@ nodes | test.cpp:237:2:237:8 | Argument 0 | semmle.label | Argument 0 | | test.cpp:241:2:241:32 | Chi [array content] | semmle.label | Chi [array content] | | test.cpp:241:2:241:32 | ChiPartial | semmle.label | ChiPartial | -| test.cpp:241:2:241:32 | ChiTotal [post update] [array content] | semmle.label | ChiTotal [post update] [array content] | | test.cpp:241:18:241:23 | call to getenv | semmle.label | call to getenv | | test.cpp:241:18:241:31 | (const char *)... | semmle.label | (const char *)... | | test.cpp:249:20:249:25 | call to getenv | semmle.label | call to getenv | @@ -155,12 +151,12 @@ nodes | test.cpp:253:11:253:29 | ... * ... | semmle.label | ... * ... | | test.cpp:253:11:253:29 | ... * ... | semmle.label | ... * ... | | test.cpp:253:11:253:29 | ... * ... | semmle.label | ... * ... | -| test.cpp:279:17:279:20 | get_size output argument | semmle.label | get_size output argument | +| test.cpp:279:17:279:20 | Chi | semmle.label | Chi | | test.cpp:279:17:279:20 | get_size output argument [array content] | semmle.label | get_size output argument [array content] | | test.cpp:281:11:281:28 | ... * ... | semmle.label | ... * ... | | test.cpp:281:11:281:28 | ... * ... | semmle.label | ... * ... | | test.cpp:281:11:281:28 | ... * ... | semmle.label | ... * ... | -| test.cpp:295:18:295:21 | get_size output argument | semmle.label | get_size output argument | +| test.cpp:295:18:295:21 | Chi | semmle.label | Chi | | test.cpp:295:18:295:21 | get_size output argument [array content] | semmle.label | get_size output argument [array content] | | test.cpp:298:10:298:27 | ... * ... | semmle.label | ... * ... | | test.cpp:298:10:298:27 | ... * ... | semmle.label | ... * ... | 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 c684f1da6ce..ca8dd38fc3b 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 @@ -43,23 +43,19 @@ edges | 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 | | test.cpp:13:2:13:15 | Chi [array content] | test.cpp:30:13:30:14 | get_rand2 output argument [array content] | -| test.cpp:13:2:13:15 | ChiTotal [post update] [array content] | test.cpp:13:2:13:15 | Chi [array content] | -| test.cpp:13:2:13:15 | ChiTotal [post update] [array content] | test.cpp:30:13:30:14 | get_rand2 output argument [array content] | -| test.cpp:13:10:13:13 | call to rand | test.cpp:13:2:13:15 | ChiTotal [post update] [array content] | -| test.cpp:13:10:13:13 | call to rand | test.cpp:13:2:13:15 | ChiTotal [post update] [array content] | +| test.cpp:13:10:13:13 | call to rand | test.cpp:13:2:13:15 | Chi [array content] | +| test.cpp:13:10:13:13 | call to rand | test.cpp:13:2:13:15 | Chi [array content] | | test.cpp:18:2:18:14 | Chi [array content] | test.cpp:36:13:36:13 | get_rand3 output argument [array content] | -| test.cpp:18:2:18:14 | ChiTotal [post update] [array content] | test.cpp:18:2:18:14 | Chi [array content] | -| test.cpp:18:2:18:14 | ChiTotal [post update] [array content] | test.cpp:36:13:36:13 | get_rand3 output argument [array content] | -| test.cpp:18:9:18:12 | call to rand | test.cpp:18:2:18:14 | ChiTotal [post update] [array content] | -| test.cpp:18:9:18:12 | call to rand | test.cpp:18:2:18:14 | ChiTotal [post update] [array content] | +| test.cpp:18:9:18:12 | call to rand | test.cpp:18:2:18:14 | Chi [array content] | +| test.cpp:18:9:18:12 | call to rand | test.cpp:18:2:18:14 | Chi [array content] | | test.cpp:24:11:24:18 | call to get_rand | test.cpp:25:7:25:7 | r | | test.cpp:24:11:24:18 | call to get_rand | test.cpp:25:7:25:7 | r | -| test.cpp:30:13:30:14 | get_rand2 output argument | test.cpp:31:7:31:7 | r | -| test.cpp:30:13:30:14 | get_rand2 output argument | test.cpp:31:7:31:7 | r | -| test.cpp:30:13:30:14 | get_rand2 output argument [array content] | test.cpp:30:13:30:14 | get_rand2 output argument | -| test.cpp:36:13:36:13 | get_rand3 output argument | test.cpp:37:7:37:7 | r | -| test.cpp:36:13:36:13 | get_rand3 output argument | test.cpp:37:7:37:7 | r | -| test.cpp:36:13:36:13 | get_rand3 output argument [array content] | test.cpp:36:13:36:13 | get_rand3 output argument | +| test.cpp:30:13:30:14 | Chi | test.cpp:31:7:31:7 | r | +| test.cpp:30:13:30:14 | Chi | test.cpp:31:7:31:7 | r | +| test.cpp:30:13:30:14 | get_rand2 output argument [array content] | test.cpp:30:13:30:14 | Chi | +| test.cpp:36:13:36:13 | Chi | test.cpp:37:7:37:7 | r | +| test.cpp:36:13:36:13 | Chi | test.cpp:37:7:37:7 | r | +| test.cpp:36:13:36:13 | get_rand3 output argument [array content] | test.cpp:36:13:36:13 | Chi | nodes | test.c:18:13:18:16 | call to rand | semmle.label | call to rand | | test.c:18:13:18:16 | call to rand | semmle.label | call to rand | @@ -114,24 +110,22 @@ nodes | test.cpp:8:9:8:12 | call to rand | semmle.label | call to rand | | test.cpp:13:2:13:15 | Chi [array content] | semmle.label | Chi [array content] | | test.cpp:13:2:13:15 | ChiPartial | semmle.label | ChiPartial | -| test.cpp:13:2:13:15 | ChiTotal [post update] [array content] | semmle.label | ChiTotal [post update] [array content] | | test.cpp:13:10:13:13 | call to rand | semmle.label | call to rand | | test.cpp:13:10:13:13 | call to rand | semmle.label | call to rand | | test.cpp:18:2:18:14 | Chi [array content] | semmle.label | Chi [array content] | | test.cpp:18:2:18:14 | ChiPartial | semmle.label | ChiPartial | -| test.cpp:18:2:18:14 | ChiTotal [post update] [array content] | semmle.label | ChiTotal [post update] [array content] | | test.cpp:18:9:18:12 | call to rand | semmle.label | call to rand | | test.cpp:18:9:18:12 | call to rand | semmle.label | call to rand | | test.cpp:24:11:24:18 | call to get_rand | semmle.label | call to get_rand | | test.cpp:25:7:25:7 | r | semmle.label | r | | test.cpp:25:7:25:7 | r | semmle.label | r | | test.cpp:25:7:25:7 | r | semmle.label | r | -| test.cpp:30:13:30:14 | get_rand2 output argument | semmle.label | get_rand2 output argument | +| test.cpp:30:13:30:14 | Chi | semmle.label | Chi | | test.cpp:30:13:30:14 | get_rand2 output argument [array content] | semmle.label | get_rand2 output argument [array content] | | test.cpp:31:7:31:7 | r | semmle.label | r | | test.cpp:31:7:31:7 | r | semmle.label | r | | test.cpp:31:7:31:7 | r | semmle.label | r | -| test.cpp:36:13:36:13 | get_rand3 output argument | semmle.label | get_rand3 output argument | +| test.cpp:36:13:36:13 | Chi | semmle.label | Chi | | test.cpp:36:13:36:13 | get_rand3 output argument [array content] | semmle.label | get_rand3 output argument [array content] | | test.cpp:37:7:37:7 | r | semmle.label | r | | test.cpp:37:7:37:7 | r | semmle.label | r | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero/UnsignedDifferenceExpressionComparedZero.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero/UnsignedDifferenceExpressionComparedZero.expected new file mode 100644 index 00000000000..2d28795d126 --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero/UnsignedDifferenceExpressionComparedZero.expected @@ -0,0 +1,10 @@ +| test.cpp:6:5:6:13 | ... > ... | Unsigned subtraction can never be negative. | +| test.cpp:10:8:10:24 | ... > ... | Unsigned subtraction can never be negative. | +| test.cpp:15:9:15:25 | ... > ... | Unsigned subtraction can never be negative. | +| test.cpp:32:12:32:20 | ... > ... | Unsigned subtraction can never be negative. | +| test.cpp:39:12:39:20 | ... > ... | Unsigned subtraction can never be negative. | +| test.cpp:47:5:47:13 | ... > ... | Unsigned subtraction can never be negative. | +| test.cpp:55:5:55:13 | ... > ... | Unsigned subtraction can never be negative. | +| test.cpp:62:5:62:13 | ... > ... | Unsigned subtraction can never be negative. | +| test.cpp:69:5:69:13 | ... > ... | Unsigned subtraction can never be negative. | +| test.cpp:75:8:75:16 | ... > ... | Unsigned subtraction can never be negative. | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero/UnsignedDifferenceExpressionComparedZero.qlref b/cpp/ql/test/query-tests/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero/UnsignedDifferenceExpressionComparedZero.qlref new file mode 100644 index 00000000000..9681978c0ad --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero/UnsignedDifferenceExpressionComparedZero.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.ql diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero/test.cpp new file mode 100644 index 00000000000..11de69e8c62 --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero/test.cpp @@ -0,0 +1,77 @@ +int getAnInt(); + +bool cond(); + +void test(unsigned x, unsigned y, bool unknown) { + if(x - y > 0) { } // BAD + + unsigned total = getAnInt(); + unsigned limit = getAnInt(); + while(limit - total > 0) { // BAD + total += getAnInt(); + } + + if(total <= limit) { + while(limit - total > 0) { // GOOD [FALSE POSITIVE] + total += getAnInt(); + if(total > limit) break; + } + } + + if(x >= y) { + bool b = x - y > 0; // GOOD + } + + if((int)(x - y) >= 0) { } // GOOD. Maybe an overflow happened, but the result is converted to the "likely intended" result before the comparison + + if(unknown) { + y = x & 0xFF; + } else { + y = x; + } + bool b1 = x - y > 0; // GOOD [FALSE POSITIVE] + + x = getAnInt(); + y = getAnInt(); + if(y > x) { + y = x - 1; + } + bool b2 = x - y > 0; // GOOD [FALSE POSITIVE] + + int N = getAnInt(); + y = x; + while(cond()) { + if(unknown) { y--; } + } + + if(x - y > 0) { } // GOOD [FALSE POSITIVE] + + x = y; + while(cond()) { + if(unknown) break; + y--; + } + + if(x - y > 0) { } // GOOD [FALSE POSITIVE] + + y = 0; + for(int i = 0; i < x; ++i) { + if(unknown) { ++y; } + } + + if(x - y > 0) { } // GOOD [FALSE POSITIVE] + + x = y; + while(cond()) { + if(unknown) { x++; } + } + + if(x - y > 0) { } // GOOD [FALSE POSITIVE] + + int n = getAnInt(); + if (n > x - y) { n = x - y; } + if (n > 0) { + y += n; // NOTE: `n` is at most `x - y` at this point. + if (x - y > 0) {} // GOOD [FALSE POSITIVE] + } +} \ No newline at end of file diff --git a/csharp/autobuilder/Semmle.Autobuild.Shared/Solution.cs b/csharp/autobuilder/Semmle.Autobuild.Shared/Solution.cs index 2f16872dd74..c5d65794fb5 100644 --- a/csharp/autobuilder/Semmle.Autobuild.Shared/Solution.cs +++ b/csharp/autobuilder/Semmle.Autobuild.Shared/Solution.cs @@ -45,6 +45,7 @@ namespace Semmle.Autobuild.Shared private readonly SolutionFile? solution; private readonly IEnumerable