From cd3bccf73afb27e1bb182e42f04ae8de81543985 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 20 Sep 2019 16:59:01 +0100 Subject: [PATCH] CPP: Fix FPs. --- cpp/ql/src/semmle/code/cpp/commons/Printf.qll | 26 ++++++++++++------- .../NonConstantFormat.expected | 8 ------ .../Likely Bugs/Format/NonConstantFormat/a.c | 8 +++--- .../Likely Bugs/Format/NonConstantFormat/b.c | 8 +++--- .../WrongTypeFormatArguments.expected | 4 --- .../Linux_signed_chars/a.c | 4 +-- 6 files changed, 27 insertions(+), 31 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/commons/Printf.qll b/cpp/ql/src/semmle/code/cpp/commons/Printf.qll index bd179fe27cc..e6436a52583 100644 --- a/cpp/ql/src/semmle/code/cpp/commons/Printf.qll +++ b/cpp/ql/src/semmle/code/cpp/commons/Printf.qll @@ -8,25 +8,33 @@ import semmle.code.cpp.commons.StringAnalysis import semmle.code.cpp.models.interfaces.FormattingFunction import semmle.code.cpp.models.implementations.Printf +class PrintfFormatAttribute extends FormatAttribute { + PrintfFormatAttribute() { + getArchetype() = "printf" or + getArchetype() = "__printf__" + } +} + /** * A function that can be identified as a `printf` style formatting * function by its use of the GNU `format` attribute. */ class AttributeFormattingFunction extends FormattingFunction { - FormatAttribute printf_attrib; - override string getCanonicalQLClass() { result = "AttributeFormattingFunction" } AttributeFormattingFunction() { - printf_attrib = getAnAttribute() and - ( - printf_attrib.getArchetype() = "printf" or - printf_attrib.getArchetype() = "__printf__" - ) and - exists(printf_attrib.getFirstFormatArgIndex()) // exclude `vprintf` style format functions + exists(PrintfFormatAttribute printf_attrib | + printf_attrib = getAnAttribute() and + exists(printf_attrib.getFirstFormatArgIndex()) // exclude `vprintf` style format functions + ) } - override int getFormatParameterIndex() { result = printf_attrib.getFormatIndex() } + override int getFormatParameterIndex() { + forex(PrintfFormatAttribute printf_attrib | + printf_attrib = getAnAttribute() | + result = printf_attrib.getFormatIndex() + ) + } } /** 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 882c9aa7815..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 @@ -1,13 +1,5 @@ | NonConstantFormat.c:30:10:30:16 | access to array | The format string argument to printf should be constant to prevent security issues and other potential errors. | | NonConstantFormat.c:41:9:41:27 | call to any_random_function | The format string argument to printf should be constant to prevent security issues and other potential errors. | -| a.c:15:37:15:45 | call to getString | The format string argument to myMultiplyDefinedPrintf should be constant to prevent security issues and other potential errors. | -| a.c:16:27:16:35 | call to getString | The format string argument to myMultiplyDefinedPrintf should be constant to prevent security issues and other potential errors. | -| a.c:17:38:17:46 | call to getString | The format string argument to myMultiplyDefinedPrintf2 should be constant to prevent security issues and other potential errors. | -| a.c:18:28:18:36 | call to getString | The format string argument to myMultiplyDefinedPrintf2 should be constant to prevent security issues and other potential errors. | -| b.c:12:37:12:45 | call to getString | The format string argument to myMultiplyDefinedPrintf should be constant to prevent security issues and other potential errors. | -| b.c:13:27:13:35 | call to getString | The format string argument to myMultiplyDefinedPrintf should be constant to prevent security issues and other potential errors. | -| b.c:14:38:14:46 | call to getString | The format string argument to myMultiplyDefinedPrintf2 should be constant to prevent security issues and other potential errors. | -| b.c:15:28:15:36 | call to getString | The format string argument to myMultiplyDefinedPrintf2 should be constant to prevent security issues and other potential errors. | | nested.cpp:21:23:21:26 | fmt0 | The format string argument to snprintf should be constant to prevent security issues and other potential errors. | | nested.cpp:79:32:79:38 | call to get_fmt | The format string argument to diagnostic should be constant to prevent security issues and other potential errors. | | nested.cpp:87:18:87:20 | fmt | The format string argument to diagnostic should be constant to prevent security issues and other potential errors. | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/a.c b/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/a.c index ffb49067469..75edda0daef 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/a.c +++ b/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/a.c @@ -12,8 +12,8 @@ char *getString(); void test_custom_printf1() { - myMultiplyDefinedPrintf("string", getString()); // GOOD [FALSE POSITIVE] - myMultiplyDefinedPrintf(getString(), "string"); // BAD - myMultiplyDefinedPrintf2("string", getString()); // GOOD (we can't tell which definition is correct so we have to assume this is OK) [FALSE POSITIVE] - myMultiplyDefinedPrintf2(getString(), "string"); // GOOD (we can't tell which definition is correct so we have to assume this is OK) [FALSE POSITIVE] + myMultiplyDefinedPrintf("string", getString()); // GOOD + myMultiplyDefinedPrintf(getString(), "string"); // BAD [NOT DETECTED] + myMultiplyDefinedPrintf2("string", getString()); // GOOD (we can't tell which definition is correct so we have to assume this is OK) + myMultiplyDefinedPrintf2(getString(), "string"); // GOOD (we can't tell which definition is correct so we have to assume this is OK) } diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/b.c b/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/b.c index a6ac46932ba..5d5044fb9ee 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/b.c +++ b/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/b.c @@ -9,8 +9,8 @@ char *getString(); void test_custom_printf2(char *string) { - myMultiplyDefinedPrintf("string", getString()); // GOOD [FALSE POSITIVE] - myMultiplyDefinedPrintf(getString(), "string"); // BAD - myMultiplyDefinedPrintf2("string", getString()); // GOOD (we can't tell which definition is correct so we have to assume this is OK) [FALSE POSITIVE] - myMultiplyDefinedPrintf2(getString(), "string"); // GOOD (we can't tell which definition is correct so we have to assume this is OK) [FALSE POSITIVE] + myMultiplyDefinedPrintf("string", getString()); // GOOD + myMultiplyDefinedPrintf(getString(), "string"); // BAD [NOT DETECTED] + myMultiplyDefinedPrintf2("string", getString()); // GOOD (we can't tell which definition is correct so we have to assume this is OK) + myMultiplyDefinedPrintf2(getString(), "string"); // GOOD (we can't tell which definition is correct so we have to assume this is OK) } \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/WrongTypeFormatArguments.expected b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/WrongTypeFormatArguments.expected index 219c185fa1d..65f4f546976 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/WrongTypeFormatArguments.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/WrongTypeFormatArguments.expected @@ -1,7 +1,3 @@ -| a.c:13:39:13:42 | 1.0 | This argument should be of type 'char *' but is of type 'double' | -| a.c:13:39:13:42 | 1.0 | This argument should be of type 'int' but is of type 'double' | -| a.c:15:40:15:43 | 1.0 | This argument should be of type 'char *' but is of type 'double' | -| a.c:15:40:15:43 | 1.0 | This argument should be of type 'int' but is of type 'double' | | format.h:16:59:16:61 | str | This argument should be of type 'int' but is of type 'char *' | | format.h:16:64:16:64 | i | This argument should be of type 'double' but is of type 'int' | | format.h:16:67:16:67 | d | This argument should be of type 'char *' but is of type 'double' | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/a.c b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/a.c index 27866e7b9ec..f830c058d91 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/a.c +++ b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/a.c @@ -10,7 +10,7 @@ void myMultiplyDefinedPrintf2(const char *format, const char *extraArg, ...); void test_custom_printf1() { myMultiplyDefinedPrintf("%i", "%s", 1); // GOOD - myMultiplyDefinedPrintf("%i", "%s", 1.0f); // BAD + myMultiplyDefinedPrintf("%i", "%s", 1.0f); // BAD [NOT DETECTED] myMultiplyDefinedPrintf2("%i", "%s", 1); // GOOD (we can't tell which definition is correct so we have to assume this is OK) - myMultiplyDefinedPrintf2("%i", "%s", 1.0f); // GOOD (we can't tell which definition is correct so we have to assume this is OK) [FALSE POSITIVE] + myMultiplyDefinedPrintf2("%i", "%s", 1.0f); // GOOD (we can't tell which definition is correct so we have to assume this is OK) }