From 170d12bf5a078ca32a53c353e2846ec7378f7874 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= <d10c@users.noreply.github.com>
Date: Wed, 24 Aug 2022 19:58:19 +0200
Subject: [PATCH] Write MissingCheckScanf.qhelp

---
 cpp/ql/src/Critical/MissingCheckScanf.cpp     | 17 +++++++
 cpp/ql/src/Critical/MissingCheckScanf.qhelp   | 51 +++++++++++++++++++
 cpp/ql/src/Critical/MissingCheckScanf.ql      | 11 ++--
 .../Critical/MissingCheckScanf/test.cpp       |  4 +-
 4 files changed, 76 insertions(+), 7 deletions(-)
 create mode 100644 cpp/ql/src/Critical/MissingCheckScanf.cpp
 create mode 100644 cpp/ql/src/Critical/MissingCheckScanf.qhelp

diff --git a/cpp/ql/src/Critical/MissingCheckScanf.cpp b/cpp/ql/src/Critical/MissingCheckScanf.cpp
new file mode 100644
index 00000000000..c4700a67e47
--- /dev/null
+++ b/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
+}
diff --git a/cpp/ql/src/Critical/MissingCheckScanf.qhelp b/cpp/ql/src/Critical/MissingCheckScanf.qhelp
new file mode 100644
index 00000000000..bb9a6e92107
--- /dev/null
+++ b/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>
diff --git a/cpp/ql/src/Critical/MissingCheckScanf.ql b/cpp/ql/src/Critical/MissingCheckScanf.ql
index 8dcde98633f..dd402cb462d 100644
--- a/cpp/ql/src/Critical/MissingCheckScanf.ql
+++ b/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
  */
diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
index 087999a41b8..e621936dc33 100644
--- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
+++ b/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
 			}