From 3a66fd717546fe0eaccf6739ce080c51d76ea5b0 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 29 Jan 2024 10:59:38 +0000 Subject: [PATCH 1/4] C++: Add FP test. --- .../MissingCheckScanf/IncorrectCheckScanf.expected | 1 + .../query-tests/Critical/MissingCheckScanf/test.cpp | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/IncorrectCheckScanf.expected b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/IncorrectCheckScanf.expected index c0ed43fee9b3..97800636a06c 100644 --- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/IncorrectCheckScanf.expected +++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/IncorrectCheckScanf.expected @@ -3,3 +3,4 @@ | test.cpp:204:7:204:11 | call to scanf | The result of scanf is only checked against 0, but it can also return EOF. | | test.cpp:436:7:436:11 | call to scanf | The result of scanf is only checked against 0, but it can also return EOF. | | test.cpp:443:11:443:15 | call to scanf | The result of scanf is only checked against 0, but it can also return EOF. | +| test.cpp:455:12:455:17 | call to sscanf | The result of scanf is only checked against 0, but it can also return EOF. | diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp index c14642bef9cd..112c53998170 100644 --- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp +++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp @@ -446,4 +446,16 @@ void bad_check() { } use(i); // GOOD [FALSE POSITIVE]: Technically no security issue, but code is incorrect. } +} + +#define EOF (-1) + +void disjunct_boolean_condition(const char* modifier_data) { + long value; + auto rc = sscanf(modifier_data, "%lx", &value); + + if((rc == EOF) || (rc == 0)) { + return; + } + use(value); // GOOD } \ No newline at end of file From 41f44f598a6b101b0f2f29ba678c2020d91a4755 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 29 Jan 2024 11:30:18 +0000 Subject: [PATCH 2/4] C++: Explicitly check that a check for EOF isn't present. --- cpp/ql/src/Critical/ScanfChecks.qll | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/cpp/ql/src/Critical/ScanfChecks.qll b/cpp/ql/src/Critical/ScanfChecks.qll index 97f8c87074b2..f6671e6fd28a 100644 --- a/cpp/ql/src/Critical/ScanfChecks.qll +++ b/cpp/ql/src/Critical/ScanfChecks.qll @@ -20,10 +20,37 @@ private predicate isLinuxKernel() { exists(Macro macro | macro.getName() in ["_LINUX_KERNEL_SPRINTF_H_", "_LINUX_KERNEL_H"]) } +/** + * Gets the value of the EOF macro. + * + * This is typically `"-1"`, but this is not guaranteed to be the case on all + * systems. + */ +private string getEofValue() { + exists(MacroInvocation mi | + mi.getMacroName() = "EOF" and + result = unique( | | mi.getExpr().getValue()) + ) +} + +/** + * Holds if the value of `call` has been checked to not equal `EOF`. + */ +private predicate checkedForEof(ScanfFunctionCall call) { + exists(IRGuardCondition gc | + exists(Instruction i, ConstantInstruction eof | + eof.getValue() = getEofValue() and + i.getUnconvertedResultExpression() = call and + gc.comparesEq(valueNumber(i).getAUse(), eof.getAUse(), 0, _, _) + ) + ) +} + /** * Holds if `call` is a `scanf`-like call were the result is only checked against 0, but it can also return EOF. */ predicate incorrectlyCheckedScanf(ScanfFunctionCall call) { exprInBooleanContext(call) and + not checkedForEof(call) and not isLinuxKernel() // scanf in the linux kernel can't return EOF } From a5794509ec39e0f932db4be840094f7763835825 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 29 Jan 2024 11:30:46 +0000 Subject: [PATCH 3/4] C++: Accept test changes. --- .../Critical/MissingCheckScanf/IncorrectCheckScanf.expected | 1 - .../Critical/MissingCheckScanf/MissingCheckScanf.expected | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/IncorrectCheckScanf.expected b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/IncorrectCheckScanf.expected index 97800636a06c..c0ed43fee9b3 100644 --- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/IncorrectCheckScanf.expected +++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/IncorrectCheckScanf.expected @@ -3,4 +3,3 @@ | test.cpp:204:7:204:11 | call to scanf | The result of scanf is only checked against 0, but it can also return EOF. | | test.cpp:436:7:436:11 | call to scanf | The result of scanf is only checked against 0, but it can also return EOF. | | test.cpp:443:11:443:15 | call to scanf | The result of scanf is only checked against 0, but it can also return EOF. | -| test.cpp:455:12:455:17 | call to sscanf | The result of scanf is only checked against 0, but it can also return EOF. | diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected index 864f8cb7cc2c..7405eff2dcd9 100644 --- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected +++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected @@ -14,3 +14,4 @@ | test.cpp:404:25:404:25 | u | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:403:6:403:11 | call to sscanf | call to sscanf | | test.cpp:416:7:416:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:413:7:413:11 | call to scanf | call to scanf | | test.cpp:423:7:423:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:420:7:420:11 | call to scanf | call to scanf | +| test.cpp:460:6:460:10 | value | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:455:12:455:17 | call to sscanf | call to sscanf | From 044d94c5802858d5d830417c9486cd191d3d0881 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 29 Jan 2024 13:47:17 +0000 Subject: [PATCH 4/4] C++: Add change note --- .../src/change-notes/2024-01-29-incorrectly-checked-scanf.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 cpp/ql/src/change-notes/2024-01-29-incorrectly-checked-scanf.md diff --git a/cpp/ql/src/change-notes/2024-01-29-incorrectly-checked-scanf.md b/cpp/ql/src/change-notes/2024-01-29-incorrectly-checked-scanf.md new file mode 100644 index 000000000000..7085b9ce0a8c --- /dev/null +++ b/cpp/ql/src/change-notes/2024-01-29-incorrectly-checked-scanf.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The "Incorrect return-value check for a 'scanf'-like function" query (`cpp/incorrectly-checked-scanf`) no longer reports an alert when an explicit check for EOF is added.