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,42 @@
+<!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.
+</p>
+<p>
+Specifically, the query flags uses of scanf wehere the reurn value is checked
+only against zero.
+</p>
+<p>
+Functions in the <tt>scanf</tt> 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.
+</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>This example shows different ways of guarding a <tt>scanf</tt> output:
+</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,43 @@
+/**
+ * @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 medium
+ * @id cpp/discarded-scanf
+ * @tags security
+ *       correctness
+ *       external/cwe/cwe-252
+ *       external/cwe/cwe-253
+ */
+
+import cpp
+import semmle.code.cpp.commons.Scanf
+
+predicate exprInBooleanContext(Expr e) {
+  e.getParent() instanceof BinaryLogicalOperation
+  or
+  e.getParent() instanceof UnaryLogicalOperation
+  or
+  e = any(IfStmt ifStmt).getCondition()
+  or
+  e = any(WhileStmt whileStmt).getCondition()
+  or
+  exists(EqualityOperation eqOp, Expr other |
+    eqOp.hasOperands(e, other) and
+    other.getValue() = "0"
+  )
+  or
+  exists(Variable v |
+    v.getAnAssignedValue() = e and
+    forex(Expr use | use = v.getAnAccess() | exprInBooleanContext(use))
+  )
+}
+
+from ScanfFunctionCall call
+where
+  exprInBooleanContext(call) and
+  not exists(call.getTarget().getDefinition()) // ignore calls where the scanf is defined as they might be different (e.g. linux)
+select call, "The result of scanf is onyl checkeck against 0, but it can also return EOF."

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