Write MissingCheckScanf.qhelp

Author: d10c

cpp/ql/src/Critical/MissingCheckScanf.cpp

@@ -0,0 +1,17 @@
+{
+  int i, j, r;
+
+  r = scanf("%d %d", &i, &j);
+
+  use(i); // BAD: i is not guarded
+
+  if (r >= 1) {
+    use(i); // GOOD: i is guarded correctly
+    use(j); // BAD: j is guarded incorrectly
+  }
+
+  if (r != 2)
+    return;
+
+  use(j); // GOOD: j is guarded correctly
+}

cpp/ql/src/Critical/MissingCheckScanf.qhelp

@@ -0,0 +1,51 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>
+This query finds calls of <tt>scanf</tt>-like functions with missing or
+improper return-value checking.
+</p>
+<p>
+Specifically, the query flags uses of variables that may have been modified by
+<tt>scanf</tt> and subsequently are used without being guarded by a correct
+return-value check. A proper check is one that asserts the corresponding
+<tt>scanf</tt> to have returned (at least) a certain minimum constant.
+</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 truthy (nonzero)
+is not enough.
+</p>
+<warning>
+This query has medium precision because, in the current implementation, it
+takes a strict stance on unguarded uses of output variables, and flags them
+as problematic even if they had already been initialized.
+</warning>
+</overview>
+
+<recommendation>
+<p>
+Ensure that all subsequent uses of <tt>scanf</tt> output arguments occur in a
+branch of an <tt>if</tt> statement (or similar), in which it is known that the
+corresponding <tt>scanf</tt> call has in fact read all possible items from its
+input. This can be done by comparing the return value to a numerical constant.
+</p>
+</recommendation>
+
+<example>
+<p>This example shows different ways of guarding a <tt>scanf</tt> output:
+</p>
+<sample src="MissingCheckScanf.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/MissingCheckScanf.ql

@@ -1,10 +1,11 @@
 /**
- * @name Missing return-value check for a scanf-like function
- * @description TODO
+ * @name Missing return-value check for a 'scanf'-like function
+ * @description Without checking that a call to 'scanf' actually wrote to an
+ *              output variable, reading from it can lead to unexpected behavior.
  * @kind problem
- * @problem.severity warning
- * @security-severity TODO
- * @precision TODO
+ * @problem.severity recommendation
+ * @security-severity 4.5
+ * @precision medium
  * @id cpp/missing-check-scanf
  * @tags security
  */

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

@@ -222,11 +222,11 @@ int main()
 			if (maybe()) {
 				break;
 			}
-			else if (maybe() && (scanf("%5c %d", c, &d) == 1)) { // GOOD [FALSE POSITIVE]
+			else if (maybe() && (scanf("%5c %d", c, &d) == 1)) { // GOOD
 				use(*(int *)c); // GOOD
 				use(d); // BAD
 			}
-			else if ((scanf("%5c %d", c, &d) == 1) && maybe()) { // GOOD [FALSE POSITIVE]
+			else if ((scanf("%5c %d", c, &d) == 1) && maybe()) { // GOOD
 				use(*(int *)c); // GOOD
 				use(d); // BAD
 			}