Merge pull request #14910 from alexet/incorrect-scanf

CPP: Add query for detecteing incorrect error checking for scanf

Author: alexet

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;
+  }
+}

cpp/ql/src/Critical/IncorrectCheckScanf.qhelp

@@ -0,0 +1,37 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>
+This query finds calls of <tt>scanf</tt>-like functions with
+improper return-value checking. Specifically, it flags uses of <code>scanf</code> where the return value is only checked against zero.
+</p>
+<p>
+Functions in the <tt>scanf</tt> family return either <tt>EOF</tt> (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.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Ensure that all uses of <tt>scanf</tt> check the return value against the expected number of arguments
+rather than just against zero.
+</p>
+</recommendation>
+
+<example>
+<p>The following examples show different ways of guarding a <tt>scanf</tt> 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.</p>
+<sample src="IncorrectCheckScanf.cpp" />
+</example>
+
+<references>
+<li>SEI CERT C++ Coding Standard: <a href="https://wiki.sei.cmu.edu/confluence/display/cplusplus/ERR62-CPP.+Detect+errors+when+converting+a+string+to+a+number">ERR62-CPP. Detect errors when converting a string to a number</a>.</li>
+<li>SEI CERT C Coding Standard: <a href="https://wiki.sei.cmu.edu/confluence/display/c/ERR33-C.+Detect+and+handle+standard+library+errors">ERR33-C. Detect and handle standard library errors</a>.</li>
+<li>cppreference.com: <a href="https://en.cppreference.com/w/c/io/fscanf">scanf, fscanf, sscanf, scanf_s, fscanf_s, sscanf_s</a>.</li>
+</references>
+</qhelp>

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."

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)
 }
 
 /**

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
+}

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.

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. |

cpp/ql/test/query-tests/Critical/MissingCheckScanf/IncorrectCheckScanf.qlref

@@ -0,0 +1 @@
+Critical/IncorrectCheckScanf.ql

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 |

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.
+	}
+}