From e99c68885c6ed9fb11d57e752199afe0288d3be6 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Thu, 20 Jun 2019 14:00:42 +0200 Subject: [PATCH 1/2] C++: Demonstrate ArrayExpr FP --- .../Format/NonConstantFormat/NonConstantFormat.expected | 1 + .../Likely Bugs/Format/NonConstantFormat/test.cpp | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/NonConstantFormat.expected b/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/NonConstantFormat.expected index 837ac3101e7..a95981a0925 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/NonConstantFormat.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/NonConstantFormat.expected @@ -18,3 +18,4 @@ | test.cpp:85:12:85:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. | | test.cpp:90:12:90:18 | ++ ... | The format string argument to printf should be constant to prevent security issues and other potential errors. | | test.cpp:107:12:107:24 | new[] | The format string argument to printf should be constant to prevent security issues and other potential errors. | +| test.cpp:142:10:142:20 | access to array | The format string argument to printf should be constant to prevent security issues and other potential errors. | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/test.cpp index 117b9232060..6d7a5d574ef 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/test.cpp @@ -133,3 +133,11 @@ void another_func(void) { printf("Hello, World\n"); // GOOD printf(gettext("Hello, World\n")); // GOOD } + +void set_value_of(int *i); + +void print_ith_message() { + int i; + set_value_of(&i); + printf(messages[i], 1U); // GOOD [FALSE POSITIVE] +} From cace411974265e979cc812b4cdc5d1b1ad171bc7 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Thu, 20 Jun 2019 14:31:34 +0200 Subject: [PATCH 2/2] C++: NonConstantFormat taint only for string types To speed up the taint analysis in `NonConstantFormat.ql` and to remove FPs that were due to taint spreading from `i` to `a[i]`, this commit stops the taint tracking in `NonConstantFormat.ql` at every node that could not possibly contain a string. I tested performance on Wireshark, and it's fine. Pulling out the `isSanitizerNode` prevented `isSanitizer` from turning into four half-slow RA predicates due to both CPE and `#antijoin_rhs` transformations happening. --- .../Likely Bugs/Format/NonConstantFormat.ql | 23 +++++++++++++++++-- .../NonConstantFormat.expected | 1 - .../Format/NonConstantFormat/test.cpp | 2 +- 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Format/NonConstantFormat.ql b/cpp/ql/src/Likely Bugs/Format/NonConstantFormat.ql index ead88181994..324c9128ba5 100644 --- a/cpp/ql/src/Likely Bugs/Format/NonConstantFormat.ql +++ b/cpp/ql/src/Likely Bugs/Format/NonConstantFormat.ql @@ -51,6 +51,15 @@ predicate underscoreMacro(Expr e) { ) } +/** + * Holds if `t` cannot hold a character array, directly or indirectly. + */ +predicate cannotContainString(Type t) { + t.getUnspecifiedType() instanceof BuiltInType + or + t.getUnspecifiedType() instanceof IntegralOrEnumType +} + predicate isNonConst(DataFlow::Node node) { exists(Expr e | e = node.asExpr() | exists(FunctionCall fc | fc = e.(FunctionCall) | @@ -99,16 +108,26 @@ predicate isNonConst(DataFlow::Node node) { node instanceof DataFlow::DefinitionByReferenceNode } +pragma[noinline] +predicate isSanitizerNode(DataFlow::Node node) { + underscoreMacro(node.asExpr()) + or + cannotContainString(node.getType()) +} + class NonConstFlow extends TaintTracking::Configuration { NonConstFlow() { this = "NonConstFlow" } - override predicate isSource(DataFlow::Node source) { isNonConst(source) } + override predicate isSource(DataFlow::Node source) { + isNonConst(source) and + not cannotContainString(source.getType()) + } override predicate isSink(DataFlow::Node sink) { exists(FormattingFunctionCall fc | sink.asExpr() = fc.getArgument(fc.getFormatParameterIndex())) } - override predicate isSanitizer(DataFlow::Node node) { underscoreMacro(node.asExpr()) } + override predicate isSanitizer(DataFlow::Node node) { isSanitizerNode(node) } } from FormattingFunctionCall call, Expr formatString diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/NonConstantFormat.expected b/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/NonConstantFormat.expected index a95981a0925..837ac3101e7 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/NonConstantFormat.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/NonConstantFormat.expected @@ -18,4 +18,3 @@ | test.cpp:85:12:85:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. | | test.cpp:90:12:90:18 | ++ ... | The format string argument to printf should be constant to prevent security issues and other potential errors. | | test.cpp:107:12:107:24 | new[] | The format string argument to printf should be constant to prevent security issues and other potential errors. | -| test.cpp:142:10:142:20 | access to array | The format string argument to printf should be constant to prevent security issues and other potential errors. | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/test.cpp index 6d7a5d574ef..24b5617321b 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/test.cpp @@ -139,5 +139,5 @@ void set_value_of(int *i); void print_ith_message() { int i; set_value_of(&i); - printf(messages[i], 1U); // GOOD [FALSE POSITIVE] + printf(messages[i], 1U); // GOOD }