diff --git a/cpp/ql/src/Critical/IncorrectCheckScanf.cpp b/cpp/ql/src/Critical/IncorrectCheckScanf.cpp new file mode 100644 index 00000000000..acec7143c63 --- /dev/null +++ b/cpp/ql/src/Critical/IncorrectCheckScanf.cpp @@ -0,0 +1,30 @@ +{ + int i, j; + + // BAD: The result is only checked against zero + if (scanf("%d %d", &i, &j)) { + use(i); + use(j); + } + + // BAD: The result is only checked against zero + if (scanf("%d %d", &i, &j) == 0) { + i = 0; + j = 0; + } + use(i); + use(j); + + if (scanf("%d %d", &i, &j) == 2) { + // GOOD: the result is checked against 2 + } + + // GOOD: the result is compared directly + int r = scanf("%d %d", &i, &j); + if (r < 2) { + return; + } + if (r == 1) { + j = 0; + } +} diff --git a/cpp/ql/src/Critical/IncorrectCheckScanf.qhelp b/cpp/ql/src/Critical/IncorrectCheckScanf.qhelp new file mode 100644 index 00000000000..eda89280bd7 --- /dev/null +++ b/cpp/ql/src/Critical/IncorrectCheckScanf.qhelp @@ -0,0 +1,37 @@ + + + + + +

+This query finds calls of scanf-like functions with +improper return-value checking. Specifically, it flags uses of scanf where the return value is only checked against zero. +

+

+Functions in the scanf family return either EOF (a negative value) +in case of IO failure, or the number of items successfully read from the +input. Consequently, a simple check that the return value is nonzero +is not enough. +

+ + + +

+Ensure that all uses of scanf check the return value against the expected number of arguments +rather than just against zero. +

+ + + +

The following examples show different ways of guarding a scanf output. In the BAD examples, the results are only checked against zero. In the GOOD examples, the results are checked against the expected number of matches instead.

+ + + + +
  • SEI CERT C++ Coding Standard: ERR62-CPP. Detect errors when converting a string to a number.
    • +
  • SEI CERT C Coding Standard: ERR33-C. Detect and handle standard library errors.
    • +
  • cppreference.com: scanf, fscanf, sscanf, scanf_s, fscanf_s, sscanf_s.
    • +     +     diff --git a/cpp/ql/src/Critical/IncorrectCheckScanf.ql b/cpp/ql/src/Critical/IncorrectCheckScanf.ql new file mode 100644 index 00000000000..e294ec33f44 --- /dev/null +++ b/cpp/ql/src/Critical/IncorrectCheckScanf.ql @@ -0,0 +1,21 @@ +/** + * @name Incorrect return-value check for a 'scanf'-like function + * @description Failing to account for EOF in a call to a scanf-like function can lead to + * undefined behavior. + * @kind problem + * @problem.severity warning + * @security-severity 7.5 + * @precision high + * @id cpp/incorrectly-checked-scanf + * @tags security + * correctness + * external/cwe/cwe-253 + */ + +import cpp +import semmle.code.cpp.commons.Scanf +import ScanfChecks + +from ScanfFunctionCall call +where incorrectlyCheckedScanf(call) +select call, "The result of scanf is only checked against 0, but it can also return EOF." diff --git a/cpp/ql/src/Critical/MissingCheckScanf.ql b/cpp/ql/src/Critical/MissingCheckScanf.ql index a5e1de0d0b3..78560383b00 100644 --- a/cpp/ql/src/Critical/MissingCheckScanf.ql +++ b/cpp/ql/src/Critical/MissingCheckScanf.ql @@ -19,6 +19,7 @@ import semmle.code.cpp.controlflow.Guards import semmle.code.cpp.dataflow.new.DataFlow::DataFlow import semmle.code.cpp.ir.IR import semmle.code.cpp.ir.ValueNumbering +import ScanfChecks /** Holds if `n` reaches an argument to a call to a `scanf`-like function. */ pragma[nomagic] @@ -60,7 +61,9 @@ predicate isSink(ScanfFunctionCall call, int index, Node n, Expr input) { * argument that has not been previously initialized. */ predicate isRelevantScanfCall(ScanfFunctionCall call, int index, Expr output) { - exists(Node n | fwdFlow0(n) and isSink(call, index, n, output)) + exists(Node n | fwdFlow0(n) and isSink(call, index, n, output)) and + // Exclude results from incorrectky checked scanf query + not incorrectlyCheckedScanf(call) } /** diff --git a/cpp/ql/src/Critical/ScanfChecks.qll b/cpp/ql/src/Critical/ScanfChecks.qll new file mode 100644 index 00000000000..97f8c87074b --- /dev/null +++ b/cpp/ql/src/Critical/ScanfChecks.qll @@ -0,0 +1,29 @@ +private import cpp +private import semmle.code.cpp.commons.Scanf +private import semmle.code.cpp.controlflow.IRGuards +private import semmle.code.cpp.ir.ValueNumbering + +private predicate exprInBooleanContext(Expr e) { + exists(IRGuardCondition gc | + exists(Instruction i, ConstantInstruction zero | + zero.getValue() = "0" and + i.getUnconvertedResultExpression() = e and + gc.comparesEq(valueNumber(i).getAUse(), zero.getAUse(), 0, _, _) + ) + or + gc.getUnconvertedResultExpression() = e + ) +} + +private predicate isLinuxKernel() { + // For the purpose of sscanf, we check the header guards for the files that it is defined in (which have changed) + exists(Macro macro | macro.getName() in ["_LINUX_KERNEL_SPRINTF_H_", "_LINUX_KERNEL_H"]) +} + +/** + * 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 isLinuxKernel() // scanf in the linux kernel can't return EOF +} diff --git a/cpp/ql/src/change-notes/2023-12-04-incorrectly-checked-scanf.md b/cpp/ql/src/change-notes/2023-12-04-incorrectly-checked-scanf.md new file mode 100644 index 00000000000..3bebd2dff46 --- /dev/null +++ b/cpp/ql/src/change-notes/2023-12-04-incorrectly-checked-scanf.md @@ -0,0 +1,4 @@ +--- +category: newQuery +--- +* The `cpp/incorrectly-checked-scanf` query has been added. This finds results where the return value of scanf is not checked correctly. Some of these were previously found by `cpp/missing-check-scanf` and will no longer be reported there. diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/IncorrectCheckScanf.expected b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/IncorrectCheckScanf.expected new file mode 100644 index 00000000000..c0ed43fee9b --- /dev/null +++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/IncorrectCheckScanf.expected @@ -0,0 +1,5 @@ +| test.cpp:162:7:162:11 | call to scanf | The result of scanf is only checked against 0, but it can also return EOF. | +| test.cpp:171:7:171:11 | call to scanf | The result of scanf is only checked against 0, but it can also return EOF. | +| 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. | diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/IncorrectCheckScanf.qlref b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/IncorrectCheckScanf.qlref new file mode 100644 index 00000000000..b166b6b60b9 --- /dev/null +++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/IncorrectCheckScanf.qlref @@ -0,0 +1 @@ +Critical/IncorrectCheckScanf.ql \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected index cecdf9fa5cc..864f8cb7cc2 100644 --- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected +++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected @@ -5,9 +5,6 @@ | test.cpp:98:7:98:8 | * ... | 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:97:3:97:7 | call to scanf | call to scanf | | test.cpp:108:7:108: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:107:3:107:8 | call to fscanf | call to fscanf | | test.cpp:115:7:115: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:114:3:114:8 | call to sscanf | call to sscanf | -| test.cpp:164:8:164:8 | 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:162:7:162:11 | call to scanf | call to scanf | -| test.cpp:173:8:173:8 | 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:171:7:171:11 | call to scanf | call to scanf | -| test.cpp:205:8:205:8 | 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:204:7:204:11 | call to scanf | call to scanf | | test.cpp:224:8:224:8 | j | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 2. | test.cpp:221:7:221:11 | call to scanf | call to scanf | | test.cpp:248:9:248:9 | d | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 2. | test.cpp:246:25:246:29 | call to scanf | call to scanf | | test.cpp:252:9:252:9 | d | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 2. | test.cpp:250:14:250:18 | call to scanf | call to scanf | diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp index 0d52b516739..c14642bef9c 100644 --- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp +++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp @@ -429,3 +429,21 @@ void scan_and_static_variable() { scanf("%d", &i); use(i); // GOOD: static variables are always 0-initialized } + +void bad_check() { + { + int i = 0; + if (scanf("%d", &i) != 0) { + return; + } + use(i); // GOOD [FALSE POSITIVE]: Technically no security issue, but code is incorrect. + } + { + int i = 0; + int r = scanf("%d", &i); + if (!r) { + return; + } + use(i); // GOOD [FALSE POSITIVE]: Technically no security issue, but code is incorrect. + } +} \ No newline at end of file