CPP: Add query for CWE-754: Improper Check for Unusual or Exceptional Conditions when using functions scanf

cpp/ql/src/experimental/Security/CWE/CWE-754/ImproperCheckReturnValueScanf.cpp

@@ -0,0 +1,10 @@
+...
+  r = scanf("%i", &i);
+  if (r == 1) // GOOD
+    return i;
+  else
+    return -1;
+...
+  scanf("%i", &i); // BAD
+  return i;
+...

cpp/ql/src/experimental/Security/CWE/CWE-754/ImproperCheckReturnValueScanf.qhelp

@@ -0,0 +1,27 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>The `scanf` family functions does not require the memory pointed to by its additional pointer arguments to be initialized before calling. The user is required to check the return value of `scanf` and similar functions to establish how many of the additional arguments were assigned values. Not checking the return value and reading one of the arguments not assigned a value is undefined behavior and may have unexpected consequences.</p>
+</overview>
+
+<recommendation>
+<p>
+The user should check the return value of `scanf` and related functions and check that any additional argument was assigned a value before reading the additional argument.
+</p>
+</recommendation>
+<example>
+<p>The first first example below is correct, as value of `i` is only read once it is checked that `scanf` has read one item. The second example is incorrect, as the return value of `scanf` is not checked, and as `scanf` might have failed to read any item before returning.</p>
+<sample src="ImproperCheckReturnValueScanf.cpp" />
+
+</example>
+<references>
+
+<li>
+  CERT C Coding Standard:
+  <a href="https://wiki.sei.cmu.edu/confluence/display/c/EXP12-C.+Do+not+ignore+values+returned+by+functions">EXP12-C. Do not ignore values returned by functions</a>.
+</li>
+
+</references>
+</qhelp>

cpp/ql/src/experimental/Security/CWE/CWE-754/ImproperCheckReturnValueScanf.ql

@@ -0,0 +1,103 @@
+/**
+ * @name Improper check of return value of scanf
+ * @description Not checking the return value of scanf and related functions may lead to undefined behavior.
+ * @kind problem
+ * @id cpp/improper-check-return-value-scanf
+ * @problem.severity warning
+ * @precision medium
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-754
+ *       external/cwe/cwe-908
+ */
+
+import cpp
+import semmle.code.cpp.commons.Exclusions
+
+/** Returns the position of the first argument being filled. */
+int posArgumentInFunctionCall(FunctionCall fc) {
+  (
+    fc.getTarget().hasGlobalOrStdName(["scanf", "scanf_s"]) and
+    result = 1
+    or
+    fc.getTarget().hasGlobalOrStdName(["fscanf", "sscanf", "fscanf_s", "sscanf_s"]) and
+    result = 2
+  )
+}
+
+/** Holds if a function argument was not initialized but used after the call. */
+predicate argumentIsNotInitializedAndIsUsed(Variable vt, FunctionCall fc) {
+  // Fillable argument was not initialized.
+  vt instanceof LocalScopeVariable and
+  not vt.getAnAssignment().getASuccessor+() = fc and
+  (
+    not vt.hasInitializer()
+    or
+    exists(Expr e, Variable v |
+      e = vt.getInitializer().getExpr() and
+      v = e.(AddressOfExpr).getOperand().(VariableAccess).getTarget() and
+      (
+        not v.hasInitializer() and
+        not v.getAnAssignment().getASuccessor+() = fc
+      )
+    )
+  ) and
+  not exists(AssignExpr ae |
+    ae.getLValue() = vt.getAnAccess().getParent() and
+    ae.getASuccessor+() = fc
+  ) and
+  not exists(FunctionCall f0 |
+    f0.getAnArgument().getAChild() = vt.getAnAccess() and
+    f0.getASuccessor+() = fc
+  ) and
+  exists(Expr e0 |
+    // After the call, the completed arguments are assigned or returned as the result of the operation of the upper function.
+    fc.getASuccessor+() = e0 and
+    (
+      (
+        e0.(Assignment).getRValue().(VariableAccess).getTarget() = vt or
+        e0.(Assignment).getRValue().(ArrayExpr).getArrayBase().(VariableAccess).getTarget() = vt
+      )
+      or
+      e0.getEnclosingStmt() instanceof ReturnStmt and
+      e0.(VariableAccess).getTarget() = vt
+      or
+      not exists(Expr e1 |
+        fc.getASuccessor+() = e1 and
+        e1.(VariableAccess).getTarget() = vt
+      )
+    )
+  )
+}
+
+from FunctionCall fc, int i
+where
+  // Function return value is not evaluated.
+  fc instanceof ExprInVoidContext and
+  not isFromMacroDefinition(fc) and
+  i in [posArgumentInFunctionCall(fc) .. fc.getNumberOfArguments() - 1] and
+  (
+    argumentIsNotInitializedAndIsUsed(fc.getArgument(i).(VariableAccess).getTarget(), fc) or
+    argumentIsNotInitializedAndIsUsed(fc.getArgument(i)
+          .(AddressOfExpr)
+          .getOperand()
+          .(VariableAccess)
+          .getTarget(), fc) or
+    argumentIsNotInitializedAndIsUsed(fc.getArgument(i)
+          .(ArrayExpr)
+          .getArrayBase()
+          .(VariableAccess)
+          .getTarget(), fc)
+  ) and
+  // After the call, filled arguments are not evaluated.
+  not exists(Expr e0, int i1 |
+    i1 in [posArgumentInFunctionCall(fc) .. fc.getNumberOfArguments() - 1] and
+    fc.getASuccessor+() = e0 and
+    e0.getEnclosingElement() instanceof ComparisonOperation and
+    (
+      e0.(VariableAccess).getTarget() = fc.getArgument(i1).(VariableAccess).getTarget() or
+      e0.(VariableAccess).getTarget() =
+        fc.getArgument(i1).(AddressOfExpr).getOperand().(VariableAccess).getTarget()
+    )
+  )
+select fc, "Unchecked return value for call to '" + fc.getTarget().getName() + "'."

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-754/semmle/tests/ImproperCheckReturnValueScanf.expected

@@ -0,0 +1,8 @@
+| test.cpp:52:3:52:7 | call to scanf | Unchecked return value for call to 'scanf'. |
+| test.cpp:53:3:53:7 | call to scanf | Unchecked return value for call to 'scanf'. |
+| test.cpp:54:3:54:7 | call to scanf | Unchecked return value for call to 'scanf'. |
+| test.cpp:105:3:105:7 | call to scanf | Unchecked return value for call to 'scanf'. |
+| test.cpp:106:3:106:7 | call to scanf | Unchecked return value for call to 'scanf'. |
+| test.cpp:107:3:107:7 | call to scanf | Unchecked return value for call to 'scanf'. |
+| test.cpp:115:3:115:7 | call to scanf | Unchecked return value for call to 'scanf'. |
+| test.cpp:120:3:120:7 | call to scanf | Unchecked return value for call to 'scanf'. |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-754/semmle/tests/ImproperCheckReturnValueScanf.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-754/ImproperCheckReturnValueScanf.ql

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-754/semmle/tests/test.cpp

@@ -0,0 +1,129 @@
+int scanf(const char *format, ...);
+int sscanf(const char *str, const char *format, ...);
+int globalVal;
+char * globalVala;
+int * globalValp;
+char globalVala2;
+int functionWork1(int retIndex) {
+  int i;
+  char a[10];
+  int b;
+  int *p = &b;
+  if (scanf("%i", &i) != 1) // GOOD
+    return -1;
+  if (scanf("%s", a) != 1) // GOOD
+    return -1;
+  if (scanf("%i", p) != 1) // GOOD
+    return -1;
+  if(retIndex == 0)
+    return (int)*a;
+  if(retIndex == 1)
+    return *p;
+  return i;
+}
+
+int functionWork1_(int retIndex) {
+  int i;
+  char a[10];
+  int b;
+  int *p = &b;
+  int r;
+  r = scanf("%i", &i);
+  if (r != 1) // GOOD
+    return -1;
+  r = scanf("%s", a);
+  if (r == 1) // GOOD
+    return -1;
+  r = scanf("%i", p);
+  if (r != 1) // GOOD
+    return -1;
+  if(retIndex == 0)
+    return (int)*a;
+  if(retIndex == 1)
+    return *p;
+  return i;
+}
+
+int functionWork1b(int retIndex) {
+  int i;
+  char a[10];
+  int b;
+  int *p = &b;
+  scanf("%i", &i); // BAD
+  scanf("%s", a); // BAD
+  scanf("%i", p); // BAD
+  if(retIndex == 0)
+    return (int)*a;
+  if(retIndex == 1)
+    return *p;
+  return i;
+}
+int functionWork1_() {
+  int i;
+  scanf("%i",&i); // BAD [NOT DETECTED]
+  if(i<10)
+    return -1;
+  return i;
+}
+int functionWork2(int retIndex) {
+  int i = 0;
+  char a[10] = "";
+  int b = 1;
+  int *p = &b;
+  scanf("%i", &i); // GOOD:Argument initialized even when scanf fails.
+  scanf("%s", a); // GOOD:Argument initialized even when scanf fails.
+  scanf("%i", p); // GOOD:Argument initialized even when scanf fails.
+  if(retIndex == 0)
+    return (int)*a;
+  if(retIndex == 1)
+    return *p;
+  return i;
+}
+
+int functionWork2_(int retIndex) {
+  int i;
+  i = 0;
+  char a[10];
+  a[0] = '\0';
+  int b;
+  b=1;
+  int *p = &b;
+  scanf("%i", &i); // GOOD:Argument initialized even when scanf fails.
+  scanf("%s", a); // GOOD:Argument initialized even when scanf fails.
+  scanf("%i", p); // GOOD:Argument initialized even when scanf fails.
+  if(retIndex == 0)
+    return (int)*a;
+  if(retIndex == 1)
+    return *p;
+  return i;
+}
+int functionWork2b() {
+  int i;
+  char a[10];
+  int b;
+  int *p = &b;
+  scanf("%i", &i); // BAD
+  scanf("%s", a); // BAD
+  scanf("%i", p); // BAD
+  globalVal = i;
+  globalVala = a;
+  globalValp = p;
+  return 0;
+}
+int functionWork2b_() {
+  char a[10];
+  scanf("%s", a); // BAD
+  globalVala2 = a[0];
+  return 0;
+}
+int functionWork3b(int * i) {
+  scanf("%i", i); // BAD
+  return 0;
+}
+int functionWork3() {
+  char number[] = "42";
+  int d;
+  sscanf(number, "%d", &d); // GOOD: sscanf always succeeds
+  if (d < 16)
+    return -1;
+}