From 834b07e6adff2080faabe86fbb5ca115c3f9aae1 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 13 Nov 2023 15:54:41 +0000 Subject: [PATCH 1/5] C++: Add failing tests. --- .../CWE-119/semmle/tests/UnboundedWrite.expected | 10 +++++----- .../Security/CWE/CWE-119/semmle/tests/tests.cpp | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected index 981b1fc82654..e0bafea2d11e 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected @@ -1,16 +1,16 @@ edges | main.cpp:6:27:6:30 | argv indirection | main.cpp:10:20:10:23 | argv indirection | -| main.cpp:10:20:10:23 | argv indirection | tests.cpp:618:32:618:35 | argv indirection | +| main.cpp:10:20:10:23 | argv indirection | tests.cpp:631:32:631:35 | argv indirection | | tests.cpp:613:19:613:24 | source indirection | tests.cpp:615:17:615:22 | source indirection | -| tests.cpp:618:32:618:35 | argv indirection | tests.cpp:643:9:643:15 | access to array indirection | -| tests.cpp:643:9:643:15 | access to array indirection | tests.cpp:613:19:613:24 | source indirection | +| tests.cpp:631:32:631:35 | argv indirection | tests.cpp:656:9:656:15 | access to array indirection | +| tests.cpp:656:9:656:15 | access to array indirection | tests.cpp:613:19:613:24 | source indirection | nodes | main.cpp:6:27:6:30 | argv indirection | semmle.label | argv indirection | | main.cpp:10:20:10:23 | argv indirection | semmle.label | argv indirection | | tests.cpp:613:19:613:24 | source indirection | semmle.label | source indirection | | tests.cpp:615:17:615:22 | source indirection | semmle.label | source indirection | -| tests.cpp:618:32:618:35 | argv indirection | semmle.label | argv indirection | -| tests.cpp:643:9:643:15 | access to array indirection | semmle.label | access to array indirection | +| tests.cpp:631:32:631:35 | argv indirection | semmle.label | argv indirection | +| tests.cpp:656:9:656:15 | access to array indirection | semmle.label | access to array indirection | subpaths #select | tests.cpp:615:2:615:7 | call to strcpy | main.cpp:6:27:6:30 | argv indirection | tests.cpp:615:17:615:22 | source indirection | This 'call to strcpy' with input from $@ may overflow the destination. | main.cpp:6:27:6:30 | argv indirection | a command-line argument | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp index b621eb473fd1..840ec23a1392 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp @@ -615,6 +615,19 @@ void test24(char* source) { strcpy(buffer, source); // BAD } +struct my_struct { + char* home; +}; + +void test25(char* source) { + my_struct s; + + s.home = source; + + char buf[100]; + strcpy(buf, s.home); // BAD [NOT DETECTED] +} + int tests_main(int argc, char *argv[]) { long long arr17[19]; @@ -641,6 +654,7 @@ int tests_main(int argc, char *argv[]) test22(argc == 0, argv[0]); test23(); test24(argv[0]); + test25(argv[0]); return 0; } From cc6268339b54c175ea01f646429ddb624673b452 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 13 Nov 2023 15:57:22 +0000 Subject: [PATCH 2/5] C++: Fix failing test and accept test cases. --- .../Security/CWE/CWE-120/UnboundedWrite.ql | 19 +++++++++++++++---- .../semmle/tests/UnboundedWrite.expected | 16 ++++++++++++++++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql b/cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql index ec6601d0c0a5..142fd7cbd7fb 100644 --- a/cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql +++ b/cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql @@ -60,9 +60,14 @@ predicate unboundedWriteSource(Expr e, BufferWrite bw) { predicate isSource(FS::FlowSource source, string sourceType) { source.getSourceType() = sourceType } -predicate isSink(DataFlow::Node sink, BufferWrite bw) { - unboundedWriteSource(sink.asIndirectExpr(), bw) - or +/** + * Holds if `bw` is a `BufferWrite` that reads from `stdin`. In such cases the + * sink is the outgoing argument that is written to. + * + * By factoring these cases out into this predicate we can place an out barrier + * on exactly these sinks in `Config`. + */ +predicate isSinkFromStdIn(DataFlow::Node sink, BufferWrite bw) { // `gets` and `scanf` reads from stdin so there's no real input. // The `BufferWrite` library models this as the call itself being // the source. In this case we mark the output argument as being @@ -72,6 +77,12 @@ predicate isSink(DataFlow::Node sink, BufferWrite bw) { unboundedWriteSource(sink.asDefiningArgument(), bw) } +predicate isSink(DataFlow::Node sink, BufferWrite bw) { + unboundedWriteSource(sink.asIndirectExpr(), bw) + or + isSinkFromStdIn(sink, bw) +} + predicate lessThanOrEqual(IRGuardCondition g, Expr e, boolean branch) { exists(Operand left | g.comparesLt(left, _, _, true, branch) or @@ -86,7 +97,7 @@ module Config implements DataFlow::ConfigSig { predicate isSink(DataFlow::Node sink) { isSink(sink, _) } - predicate isBarrierOut(DataFlow::Node node) { isSink(node) } + predicate isBarrierOut(DataFlow::Node node) { isSinkFromStdIn(node, _) } predicate isBarrier(DataFlow::Node node) { // Block flow if the node is guarded by any <, <= or = operations. diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected index e0bafea2d11e..ede0018c7663 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected @@ -2,15 +2,31 @@ edges | main.cpp:6:27:6:30 | argv indirection | main.cpp:10:20:10:23 | argv indirection | | main.cpp:10:20:10:23 | argv indirection | tests.cpp:631:32:631:35 | argv indirection | | tests.cpp:613:19:613:24 | source indirection | tests.cpp:615:17:615:22 | source indirection | +| tests.cpp:622:19:622:24 | source indirection | tests.cpp:625:2:625:16 | ... = ... indirection | +| tests.cpp:625:2:625:16 | ... = ... indirection | tests.cpp:625:4:625:7 | s indirection [post update] [home indirection] | +| tests.cpp:625:4:625:7 | s indirection [post update] [home indirection] | tests.cpp:628:14:628:14 | s indirection [home indirection] | +| tests.cpp:628:14:628:14 | s indirection [home indirection] | tests.cpp:628:14:628:19 | home indirection | +| tests.cpp:628:14:628:14 | s indirection [home indirection] | tests.cpp:628:16:628:19 | home indirection | +| tests.cpp:628:16:628:19 | home indirection | tests.cpp:628:14:628:19 | home indirection | | tests.cpp:631:32:631:35 | argv indirection | tests.cpp:656:9:656:15 | access to array indirection | +| tests.cpp:631:32:631:35 | argv indirection | tests.cpp:657:9:657:15 | access to array indirection | | tests.cpp:656:9:656:15 | access to array indirection | tests.cpp:613:19:613:24 | source indirection | +| tests.cpp:657:9:657:15 | access to array indirection | tests.cpp:622:19:622:24 | source indirection | nodes | main.cpp:6:27:6:30 | argv indirection | semmle.label | argv indirection | | main.cpp:10:20:10:23 | argv indirection | semmle.label | argv indirection | | tests.cpp:613:19:613:24 | source indirection | semmle.label | source indirection | | tests.cpp:615:17:615:22 | source indirection | semmle.label | source indirection | +| tests.cpp:622:19:622:24 | source indirection | semmle.label | source indirection | +| tests.cpp:625:2:625:16 | ... = ... indirection | semmle.label | ... = ... indirection | +| tests.cpp:625:4:625:7 | s indirection [post update] [home indirection] | semmle.label | s indirection [post update] [home indirection] | +| tests.cpp:628:14:628:14 | s indirection [home indirection] | semmle.label | s indirection [home indirection] | +| tests.cpp:628:14:628:19 | home indirection | semmle.label | home indirection | +| tests.cpp:628:16:628:19 | home indirection | semmle.label | home indirection | | tests.cpp:631:32:631:35 | argv indirection | semmle.label | argv indirection | | tests.cpp:656:9:656:15 | access to array indirection | semmle.label | access to array indirection | +| tests.cpp:657:9:657:15 | access to array indirection | semmle.label | access to array indirection | subpaths #select | tests.cpp:615:2:615:7 | call to strcpy | main.cpp:6:27:6:30 | argv indirection | tests.cpp:615:17:615:22 | source indirection | This 'call to strcpy' with input from $@ may overflow the destination. | main.cpp:6:27:6:30 | argv indirection | a command-line argument | +| tests.cpp:628:2:628:7 | call to strcpy | main.cpp:6:27:6:30 | argv indirection | tests.cpp:628:14:628:19 | home indirection | This 'call to strcpy' with input from $@ may overflow the destination. | main.cpp:6:27:6:30 | argv indirection | a command-line argument | From 9aafbfce1334659f18e725570fe332f1cc0a6cf1 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 13 Nov 2023 16:17:18 +0000 Subject: [PATCH 3/5] C++: Fix test annotation. --- .../query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp index 840ec23a1392..e566251823ca 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp @@ -625,7 +625,7 @@ void test25(char* source) { s.home = source; char buf[100]; - strcpy(buf, s.home); // BAD [NOT DETECTED] + strcpy(buf, s.home); // BAD } int tests_main(int argc, char *argv[]) From c73e6f1fa834c0416fa690375bc94475775e968e Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 13 Nov 2023 16:51:07 +0000 Subject: [PATCH 4/5] C++: Accept more test changes. --- .../CWE/CWE-120/semmle/tests/UnboundedWrite.expected | 6 ++++++ 1 file changed, 6 insertions(+) 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 f44ec61da83b..ee80627ae7dd 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 @@ -1,18 +1,24 @@ edges | tests.c:16:26:16:29 | argv indirection | tests.c:28:22:28:28 | access to array indirection | | tests.c:16:26:16:29 | argv indirection | tests.c:29:28:29:34 | access to array indirection | +| tests.c:16:26:16:29 | argv indirection | tests.c:31:15:31:23 | buffer100 indirection | +| tests.c:16:26:16:29 | argv indirection | tests.c:33:21:33:29 | buffer100 indirection | | tests.c:16:26:16:29 | argv indirection | tests.c:34:10:34:16 | access to array indirection | nodes | tests.c:16:26:16:29 | argv indirection | semmle.label | argv indirection | | tests.c:28:22:28:28 | access to array indirection | semmle.label | access to array indirection | | tests.c:29:28:29:34 | access to array indirection | semmle.label | access to array indirection | +| tests.c:31:15:31:23 | buffer100 indirection | semmle.label | buffer100 indirection | | tests.c:31:15:31:23 | scanf output argument | semmle.label | scanf output argument | +| tests.c:33:21:33:29 | buffer100 indirection | semmle.label | buffer100 indirection | | tests.c:33:21:33:29 | scanf output argument | semmle.label | scanf output argument | | tests.c:34:10:34:16 | access to array indirection | semmle.label | access to array indirection | subpaths #select | tests.c:28:3:28:9 | call to sprintf | tests.c:16:26:16:29 | argv indirection | tests.c:28:22:28:28 | access to array indirection | This 'call to sprintf' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | argv indirection | a command-line argument | | tests.c:29:3:29:9 | call to sprintf | tests.c:16:26:16:29 | argv indirection | tests.c:29:28:29:34 | access to array indirection | This 'call to sprintf' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | argv indirection | a command-line argument | +| tests.c:31:15:31:23 | buffer100 | tests.c:16:26:16:29 | argv indirection | tests.c:31:15:31:23 | buffer100 indirection | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | argv indirection | a command-line argument | | 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:16:26:16:29 | argv indirection | tests.c:33:21:33:29 | buffer100 indirection | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | argv indirection | a command-line argument | | 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 indirection | tests.c:34:10:34:16 | access to array indirection | This 'sscanf string argument' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | argv indirection | a command-line argument | From 967bbbc1a7e77c6ab9d2dd7d3671fbbd5fe8421b Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 14 Nov 2023 09:29:47 +0000 Subject: [PATCH 5/5] C++: Block flow out of sinks that are qualifiers. This removes the new result duplication and keeps the new result. --- .../Security/CWE/CWE-120/UnboundedWrite.ql | 32 +++++++------------ .../semmle/tests/UnboundedWrite.expected | 6 ---- 2 files changed, 11 insertions(+), 27 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql b/cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql index 142fd7cbd7fb..bbc58874c8ec 100644 --- a/cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql +++ b/cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql @@ -52,35 +52,25 @@ predicate isUnboundedWrite(BufferWrite bw) { * Holds if `e` is a source buffer going into an unbounded write `bw` or a * qualifier of (a qualifier of ...) such a source. */ -predicate unboundedWriteSource(Expr e, BufferWrite bw) { - isUnboundedWrite(bw) and e = bw.getASource() +predicate unboundedWriteSource(Expr e, BufferWrite bw, boolean qualifier) { + isUnboundedWrite(bw) and e = bw.getASource() and qualifier = false or - exists(FieldAccess fa | unboundedWriteSource(fa, bw) and e = fa.getQualifier()) + exists(FieldAccess fa | unboundedWriteSource(fa, bw, _) and e = fa.getQualifier()) and + qualifier = true } predicate isSource(FS::FlowSource source, string sourceType) { source.getSourceType() = sourceType } -/** - * Holds if `bw` is a `BufferWrite` that reads from `stdin`. In such cases the - * sink is the outgoing argument that is written to. - * - * By factoring these cases out into this predicate we can place an out barrier - * on exactly these sinks in `Config`. - */ -predicate isSinkFromStdIn(DataFlow::Node sink, BufferWrite bw) { +predicate isSink(DataFlow::Node sink, BufferWrite bw, boolean qualifier) { + unboundedWriteSource(sink.asIndirectExpr(), bw, qualifier) + or // `gets` and `scanf` reads from stdin so there's no real input. // The `BufferWrite` library models this as the call itself being // the source. In this case we mark the output argument as being // the sink so that we report a path where source = sink (because // the same output argument is also included in `isSource`). bw.getASource() = bw and - unboundedWriteSource(sink.asDefiningArgument(), bw) -} - -predicate isSink(DataFlow::Node sink, BufferWrite bw) { - unboundedWriteSource(sink.asIndirectExpr(), bw) - or - isSinkFromStdIn(sink, bw) + unboundedWriteSource(sink.asDefiningArgument(), bw, qualifier) } predicate lessThanOrEqual(IRGuardCondition g, Expr e, boolean branch) { @@ -95,9 +85,9 @@ predicate lessThanOrEqual(IRGuardCondition g, Expr e, boolean branch) { module Config implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { isSource(source, _) } - predicate isSink(DataFlow::Node sink) { isSink(sink, _) } + predicate isSink(DataFlow::Node sink) { isSink(sink, _, _) } - predicate isBarrierOut(DataFlow::Node node) { isSinkFromStdIn(node, _) } + predicate isBarrierOut(DataFlow::Node node) { isSink(node, _, false) } predicate isBarrier(DataFlow::Node node) { // Block flow if the node is guarded by any <, <= or = operations. @@ -127,7 +117,7 @@ from BufferWrite bw, Flow::PathNode source, Flow::PathNode sink, string sourceTy where Flow::flowPath(source, sink) and isSource(source.getNode(), sourceType) and - isSink(sink.getNode(), bw) + isSink(sink.getNode(), bw, _) select bw, source, sink, "This '" + bw.getBWDesc() + "' with input from $@ may overflow the destination.", source.getNode(), sourceType 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 ee80627ae7dd..f44ec61da83b 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 @@ -1,24 +1,18 @@ edges | tests.c:16:26:16:29 | argv indirection | tests.c:28:22:28:28 | access to array indirection | | tests.c:16:26:16:29 | argv indirection | tests.c:29:28:29:34 | access to array indirection | -| tests.c:16:26:16:29 | argv indirection | tests.c:31:15:31:23 | buffer100 indirection | -| tests.c:16:26:16:29 | argv indirection | tests.c:33:21:33:29 | buffer100 indirection | | tests.c:16:26:16:29 | argv indirection | tests.c:34:10:34:16 | access to array indirection | nodes | tests.c:16:26:16:29 | argv indirection | semmle.label | argv indirection | | tests.c:28:22:28:28 | access to array indirection | semmle.label | access to array indirection | | tests.c:29:28:29:34 | access to array indirection | semmle.label | access to array indirection | -| tests.c:31:15:31:23 | buffer100 indirection | semmle.label | buffer100 indirection | | tests.c:31:15:31:23 | scanf output argument | semmle.label | scanf output argument | -| tests.c:33:21:33:29 | buffer100 indirection | semmle.label | buffer100 indirection | | tests.c:33:21:33:29 | scanf output argument | semmle.label | scanf output argument | | tests.c:34:10:34:16 | access to array indirection | semmle.label | access to array indirection | subpaths #select | tests.c:28:3:28:9 | call to sprintf | tests.c:16:26:16:29 | argv indirection | tests.c:28:22:28:28 | access to array indirection | This 'call to sprintf' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | argv indirection | a command-line argument | | tests.c:29:3:29:9 | call to sprintf | tests.c:16:26:16:29 | argv indirection | tests.c:29:28:29:34 | access to array indirection | This 'call to sprintf' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | argv indirection | a command-line argument | -| tests.c:31:15:31:23 | buffer100 | tests.c:16:26:16:29 | argv indirection | tests.c:31:15:31:23 | buffer100 indirection | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | argv indirection | a command-line argument | | 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:16:26:16:29 | argv indirection | tests.c:33:21:33:29 | buffer100 indirection | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | argv indirection | a command-line argument | | 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 indirection | tests.c:34:10:34:16 | access to array indirection | This 'sscanf string argument' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | argv indirection | a command-line argument |