From 572600b42869d69b88e8d1f98d3bef12251abba8 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 8 Apr 2024 11:51:25 +0100 Subject: [PATCH 1/4] C++: Add testcase with FP. --- .../semmle/tests/BadlyBoundedWrite.expected | 4 ++-- .../semmle/tests/OverrunWriteFloat.expected | 4 ++-- .../semmle/tests/UnboundedWrite.expected | 3 +++ .../tests/VeryLikelyOverrunWrite.expected | 22 +++++++++---------- .../Security/CWE/CWE-120/semmle/tests/tests.c | 5 +++++ 5 files changed, 23 insertions(+), 15 deletions(-) diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/BadlyBoundedWrite.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/BadlyBoundedWrite.expected index 9abc89c68f17..2aa7ba1a9ff6 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/BadlyBoundedWrite.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/BadlyBoundedWrite.expected @@ -1,2 +1,2 @@ -| tests.c:43:3:43:10 | call to snprintf | This 'call to snprintf' operation is limited to 111 bytes but the destination is only 110 bytes. | -| tests.c:46:3:46:10 | call to snprintf | This 'call to snprintf' operation is limited to 111 bytes but the destination is only 110 bytes. | +| tests.c:48:3:48:10 | call to snprintf | This 'call to snprintf' operation is limited to 111 bytes but the destination is only 110 bytes. | +| tests.c:51:3:51:10 | call to snprintf | This 'call to snprintf' operation is limited to 111 bytes but the destination is only 110 bytes. | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/OverrunWriteFloat.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/OverrunWriteFloat.expected index 765d75ed3b95..b0eb468e21ef 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/OverrunWriteFloat.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/OverrunWriteFloat.expected @@ -1,2 +1,2 @@ -| tests.c:103:3:103:9 | call to sprintf | This 'call to sprintf' operation may require 318 bytes because of float conversions, but the target is only 256 bytes. | -| tests.c:105:3:105:9 | call to sprintf | This 'call to sprintf' operation may require 346 bytes because of float conversions, but the target is only 256 bytes. | +| tests.c:108:3:108:9 | call to sprintf | This 'call to sprintf' operation may require 318 bytes because of float conversions, but the target is only 256 bytes. | +| tests.c:110:3:110:9 | call to sprintf | This 'call to sprintf' operation may require 346 bytes because of float conversions, but the target is only 256 bytes. | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/UnboundedWrite.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/UnboundedWrite.expected index 0ebcbb8cde4e..0e5852b59733 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/UnboundedWrite.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/UnboundedWrite.expected @@ -2,6 +2,7 @@ edges | tests.c:16:26:16:29 | **argv | tests.c:28:22:28:28 | *access to array | provenance | | | tests.c:16:26:16:29 | **argv | tests.c:29:28:29:34 | *access to array | provenance | | | tests.c:16:26:16:29 | **argv | tests.c:34:10:34:16 | *access to array | provenance | | +| tests.c:16:26:16:29 | **argv | tests.c:39:22:39:45 | *buffer_with_tainted_char | provenance | | nodes | tests.c:16:26:16:29 | **argv | semmle.label | **argv | | tests.c:28:22:28:28 | *access to array | semmle.label | *access to array | @@ -9,6 +10,7 @@ nodes | tests.c:31:15:31:23 | scanf output argument | semmle.label | scanf output argument | | tests.c:33:21:33:29 | scanf output argument | semmle.label | scanf output argument | | tests.c:34:10:34:16 | *access to array | semmle.label | *access to array | +| tests.c:39:22:39:45 | *buffer_with_tainted_char | semmle.label | *buffer_with_tainted_char | subpaths #select | tests.c:28:3:28:9 | call to sprintf | tests.c:16:26:16:29 | **argv | tests.c:28:22:28:28 | *access to array | This 'call to sprintf' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | **argv | a command-line argument | @@ -16,3 +18,4 @@ subpaths | tests.c:31:15:31:23 | buffer100 | tests.c:31:15:31:23 | scanf output argument | tests.c:31:15:31:23 | scanf output argument | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:31:15:31:23 | scanf output argument | value read by scanf | | tests.c:33:21:33:29 | buffer100 | tests.c:33:21:33:29 | scanf output argument | tests.c:33:21:33:29 | scanf output argument | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:33:21:33:29 | scanf output argument | value read by scanf | | tests.c:34:25:34:33 | buffer100 | tests.c:16:26:16:29 | **argv | tests.c:34:10:34:16 | *access to array | This 'sscanf string argument' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | **argv | a command-line argument | +| tests.c:39:3:39:9 | call to sprintf | tests.c:16:26:16:29 | **argv | tests.c:39:22:39:45 | *buffer_with_tainted_char | This 'call to sprintf' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | **argv | a command-line argument | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/VeryLikelyOverrunWrite.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/VeryLikelyOverrunWrite.expected index 641be44149e5..9e7e714d14e1 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/VeryLikelyOverrunWrite.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/VeryLikelyOverrunWrite.expected @@ -5,17 +5,17 @@ | tests2.cpp:37:3:37:8 | call to wcscpy | This 'call to wcscpy' operation requires 28 bytes but the destination is only 24 bytes. | | tests2.cpp:42:3:42:8 | call to wcscpy | This 'call to wcscpy' operation requires 32 bytes but the destination is only 28 bytes. | | tests2.cpp:47:3:47:8 | call to wcscpy | This 'call to wcscpy' operation requires 36 bytes but the destination is only 32 bytes. | -| tests.c:54:3:54:9 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 10 bytes. | -| tests.c:58:3:58:9 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 10 bytes. | -| tests.c:62:17:62:24 | buffer10 | This 'scanf string argument' operation requires 11 bytes but the destination is only 10 bytes. | -| tests.c:63:17:63:24 | buffer10 | This 'scanf string argument' operation requires 12 bytes but the destination is only 10 bytes. | -| tests.c:86:3:86:8 | call to strcpy | This 'call to strcpy' operation requires 6 bytes but the destination is only 5 bytes. | -| tests.c:93:3:93:8 | call to strcpy | This 'call to strcpy' operation requires 6 bytes but the destination is only 5 bytes. | -| tests.c:120:3:120:9 | call to sprintf | This 'call to sprintf' operation requires 17 bytes but the destination is only 1 bytes. | -| tests.c:121:3:121:9 | call to sprintf | This 'call to sprintf' operation requires 17 bytes but the destination is only 16 bytes. | -| tests.c:136:2:136:8 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 10 bytes. | -| tests.c:186:3:186:9 | call to sprintf | This 'call to sprintf' operation requires 9 bytes but the destination is only 2 bytes. | -| tests.c:189:3:189:9 | call to sprintf | This 'call to sprintf' operation requires 3 bytes but the destination is only 2 bytes. | +| tests.c:59:3:59:9 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 10 bytes. | +| tests.c:63:3:63:9 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 10 bytes. | +| tests.c:67:17:67:24 | buffer10 | This 'scanf string argument' operation requires 11 bytes but the destination is only 10 bytes. | +| tests.c:68:17:68:24 | buffer10 | This 'scanf string argument' operation requires 12 bytes but the destination is only 10 bytes. | +| tests.c:91:3:91:8 | call to strcpy | This 'call to strcpy' operation requires 6 bytes but the destination is only 5 bytes. | +| tests.c:98:3:98:8 | call to strcpy | This 'call to strcpy' operation requires 6 bytes but the destination is only 5 bytes. | +| tests.c:125:3:125:9 | call to sprintf | This 'call to sprintf' operation requires 17 bytes but the destination is only 1 bytes. | +| tests.c:126:3:126:9 | call to sprintf | This 'call to sprintf' operation requires 17 bytes but the destination is only 16 bytes. | +| tests.c:141:2:141:8 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 10 bytes. | +| tests.c:191:3:191:9 | call to sprintf | This 'call to sprintf' operation requires 9 bytes but the destination is only 2 bytes. | +| tests.c:194:3:194:9 | call to sprintf | This 'call to sprintf' operation requires 3 bytes but the destination is only 2 bytes. | | unions.c:26:2:26:7 | call to strcpy | This 'call to strcpy' operation requires 21 bytes but the destination is only 16 bytes. | | unions.c:27:2:27:7 | call to strcpy | This 'call to strcpy' operation requires 21 bytes but the destination is only 16 bytes. | | var_size_struct.cpp:22:3:22:8 | call to strcpy | This 'call to strcpy' operation requires 10 bytes but the destination is only 9 bytes. | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/tests.c b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/tests.c index 5d37ff374ba3..43e802bef825 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/tests.c +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/tests.c @@ -32,6 +32,11 @@ int main(int argc, char *argv[]) scanf("%i", i); // GOOD: no problems with non-strings scanf("%i %s", i, buffer100); // BAD: second format parameter may overflow sscanf(argv[0], "%s", buffer100); // BAD: argv[0] could be more than 100 characters + + char* argv1 = argv[1]; + char buffer_with_tainted_char[4] = {0}; + buffer_with_tainted_char[0] = *argv1; + sprintf(buffer100, buffer_with_tainted_char); // GOOD [FALSE POSITIVE]: Only a single character in buffer_with_tainted_char is tainted } // Test cases for BadlyBoundedWrite.ql From 9beaea531844d43e853ea8a561836beb37883b2c Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 8 Apr 2024 11:52:39 +0100 Subject: [PATCH 2/4] C++: Block assignments to integral and float types. --- cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql b/cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql index bbc58874c8ec..092539535433 100644 --- a/cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql +++ b/cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql @@ -92,6 +92,11 @@ module Config implements DataFlow::ConfigSig { predicate isBarrier(DataFlow::Node node) { // Block flow if the node is guarded by any <, <= or = operations. node = DataFlow::BarrierGuard::getABarrierNode() + or + // Block flow if we manage to flow to a single `int` or `float`. + node.asDefinition().getUnspecifiedType() instanceof IntegralType + or + node.asDefinition().getUnspecifiedType() instanceof FloatingPointType } } From 90aa1776aa77f033ec55c94c67fd84bd9c3f49bb Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 8 Apr 2024 11:52:45 +0100 Subject: [PATCH 3/4] C++: Accept test changes. --- .../Security/CWE/CWE-120/semmle/tests/UnboundedWrite.expected | 3 --- .../test/query-tests/Security/CWE/CWE-120/semmle/tests/tests.c | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/UnboundedWrite.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/UnboundedWrite.expected index 0e5852b59733..0ebcbb8cde4e 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/UnboundedWrite.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/UnboundedWrite.expected @@ -2,7 +2,6 @@ edges | tests.c:16:26:16:29 | **argv | tests.c:28:22:28:28 | *access to array | provenance | | | tests.c:16:26:16:29 | **argv | tests.c:29:28:29:34 | *access to array | provenance | | | tests.c:16:26:16:29 | **argv | tests.c:34:10:34:16 | *access to array | provenance | | -| tests.c:16:26:16:29 | **argv | tests.c:39:22:39:45 | *buffer_with_tainted_char | provenance | | nodes | tests.c:16:26:16:29 | **argv | semmle.label | **argv | | tests.c:28:22:28:28 | *access to array | semmle.label | *access to array | @@ -10,7 +9,6 @@ nodes | tests.c:31:15:31:23 | scanf output argument | semmle.label | scanf output argument | | tests.c:33:21:33:29 | scanf output argument | semmle.label | scanf output argument | | tests.c:34:10:34:16 | *access to array | semmle.label | *access to array | -| tests.c:39:22:39:45 | *buffer_with_tainted_char | semmle.label | *buffer_with_tainted_char | subpaths #select | tests.c:28:3:28:9 | call to sprintf | tests.c:16:26:16:29 | **argv | tests.c:28:22:28:28 | *access to array | This 'call to sprintf' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | **argv | a command-line argument | @@ -18,4 +16,3 @@ subpaths | tests.c:31:15:31:23 | buffer100 | tests.c:31:15:31:23 | scanf output argument | tests.c:31:15:31:23 | scanf output argument | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:31:15:31:23 | scanf output argument | value read by scanf | | tests.c:33:21:33:29 | buffer100 | tests.c:33:21:33:29 | scanf output argument | tests.c:33:21:33:29 | scanf output argument | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:33:21:33:29 | scanf output argument | value read by scanf | | tests.c:34:25:34:33 | buffer100 | tests.c:16:26:16:29 | **argv | tests.c:34:10:34:16 | *access to array | This 'sscanf string argument' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | **argv | a command-line argument | -| tests.c:39:3:39:9 | call to sprintf | tests.c:16:26:16:29 | **argv | tests.c:39:22:39:45 | *buffer_with_tainted_char | This 'call to sprintf' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | **argv | a command-line argument | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/tests.c b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/tests.c index 43e802bef825..2e49b68b436c 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/tests.c +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/tests.c @@ -36,7 +36,7 @@ int main(int argc, char *argv[]) char* argv1 = argv[1]; char buffer_with_tainted_char[4] = {0}; buffer_with_tainted_char[0] = *argv1; - sprintf(buffer100, buffer_with_tainted_char); // GOOD [FALSE POSITIVE]: Only a single character in buffer_with_tainted_char is tainted + sprintf(buffer100, buffer_with_tainted_char); // GOOD: Only a single character in buffer_with_tainted_char is tainted } // Test cases for BadlyBoundedWrite.ql From 920c987d01c324c8f0e106f1f8bc3de3e3633000 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 8 Apr 2024 11:57:18 +0100 Subject: [PATCH 4/4] C++: Add change note. --- cpp/ql/src/change-notes/2024-04-08-unbounded-write.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 cpp/ql/src/change-notes/2024-04-08-unbounded-write.md diff --git a/cpp/ql/src/change-notes/2024-04-08-unbounded-write.md b/cpp/ql/src/change-notes/2024-04-08-unbounded-write.md new file mode 100644 index 000000000000..e70746193c26 --- /dev/null +++ b/cpp/ql/src/change-notes/2024-04-08-unbounded-write.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The "Unbounded write" query (`cpp/unbounded-write`) has been improved to produce fewer false positives. \ No newline at end of file