C++: Missing return-value check for scanf-like functions #1076

.gitignore

@@ -15,6 +15,9 @@
 .vs/*
 !.vs/VSWorkspaceSettings.json
 
+# VSCode temporaries
+.vscode/*.log
+
 # Byte-compiled python files
 *.pyc
 

cpp/ql/src/Critical/MissingCheckScanf.ql

@@ -0,0 +1,118 @@
+/**
+ * @name Missing return-value check for a scanf-like function
+ * @description TODO
+ * @kind problem
+ * @problem.severity warning
+ * @security-severity TODO
+ * @precision TODO
+ * @id cpp/missing-check-scanf
+ * @tags security
+ */
+
+import cpp
+import semmle.code.cpp.commons.Scanf
+import semmle.code.cpp.controlflow.Guards
+import semmle.code.cpp.dataflow.DataFlow
+import semmle.code.cpp.ir.IR
+import semmle.code.cpp.ir.ValueNumbering
+
+/** An expression appearing as an output argument to a `scanf`-like call */
+class ScanfOutput extends Expr {
+  ScanfFunctionCall call;
+  int varargIndex;
+
+  ScanfOutput() {
+    this = call.getArgument(call.getTarget().getNumberOfParameters() + varargIndex) and
+    varargIndex >= 0
+  }
+
+  ScanfFunctionCall getCall() { result = call }
+
+  /**
+   * Any subsequent use of this argument should be surrounded by a
+   * check ensuring that the `scanf`-like function has returned a value
+   * equal to at least `getMinimumGuardConstant()`.
+   */
+  int getMinimumGuardConstant() {
+    result =
+      varargIndex + 1 -
+        count(ScanfFormatLiteral f, int n |
+          n <= varargIndex and f.getUse() = call and f.parseConvSpec(n, _, _, _, "n")
+        )
+  }
+
+  predicate hasGuardedAccess(Access e, boolean isGuarded) {
+    e = getAnAccess() and
+    if
+      exists(int value, int minGuard | minGuard = getMinimumGuardConstant() |
+        e.getBasicBlock() = blockGuardedBy(value, "==", call) and minGuard <= value
+        or
+        e.getBasicBlock() = blockGuardedBy(value, "<", call) and minGuard - 1 <= value
+        or
+        e.getBasicBlock() = blockGuardedBy(value, "<=", call) and minGuard <= value
+      )
+    then isGuarded = true
+    else isGuarded = false
+  }
+
+  /**
+   * Get a subsequent access of the same underlying storage,
+   * but before it gets reset or reused in another `scanf` call.
+   */
+  Access getAnAccess() {
+    exists(Instruction j | result = j.getAst() |
+      j = getASubsequentSameValuedInstruction() and
+      forall(Instruction k |
+        k = [getAResetInstruction(), getAReuseInstruction()] implies j.getASuccessor+() = k
+      )
+    )
+  }
+
+  private Instruction getAResetInstruction() {
+    result = getASubsequentSameValuedInstruction() and
+    result = any(StoreInstruction s).getDestinationAddress()
+  }
+
+  private Instruction getAReuseInstruction() {
+    result = getASubsequentSameValuedInstruction() and
+    exists(Expr e | result.getAst() = e |
+      e instanceof ScanfOutput
+      or
+      e.getParent().(AddressOfExpr) instanceof ScanfOutput
+    )
+  }
+
+  private Instruction getASubsequentSameValuedInstruction() {
+    exists(Instruction i |
+      i.getUnconvertedResultExpression() = this and
+      result = valueNumber(i).getAnInstruction() and
+      i.getASuccessor+() = result and
+      forall(Expr child | child = this.getAChild*() implies child != result.getAst())
+    )
+  }
+}
+
+/** Returns a block guarded by the assertion of $value $op $call */
+BasicBlock blockGuardedBy(int value, string op, ScanfFunctionCall call) {
+  exists(GuardCondition g, Expr left, Expr right |
+    right = g.getAChild() and
+    value = left.getValue().toInt() and
+    DataFlow::localExprFlow(call, right)
+  |
+    g.ensuresEq(left, right, 0, result, true) and op = "=="
+    or
+    g.ensuresLt(left, right, 0, result, true) and op = "<"
+    or
+    g.ensuresLt(left, right, 1, result, true) and op = "<="
+  )
+}
+
+from ScanfOutput output, ScanfFunctionCall call, ScanfFunction fun, Access access
+where
+  call.getTarget() = fun and
+  output.getCall() = call and
+  output.hasGuardedAccess(access, false)
+select access,
+  "$@ is read here, but may not have been written. " +
+    "It should be guarded by a check that $@() returns at least " + output.getMinimumGuardConstant()
+    + ".", access, access.toString(), call, fun.getName()

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

@@ -0,0 +1 @@
+DisableFormat: true

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

@@ -0,0 +1,16 @@
+| test.cpp:30:7:30:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:30:7:30:7 | i | i | test.cpp:29:3:29:7 | call to scanf | scanf |
+| test.cpp:46:7:46:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:46:7:46:7 | i | i | test.cpp:45:3:45:7 | call to scanf | scanf |
+| test.cpp:63:7:63:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:63:7:63:7 | i | i | test.cpp:62:3:62:7 | call to scanf | scanf |
+| test.cpp:77:7:77:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:77:7:77:7 | i | i | test.cpp:76:3:76:8 | call to fscanf | fscanf |
+| test.cpp:84:7:84:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:84:7:84:7 | i | i | test.cpp:83:3:83:8 | call to sscanf | sscanf |
+| test.cpp:133:8:133:8 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:133:8:133:8 | i | i | test.cpp:131:7:131:11 | call to scanf | scanf |
+| test.cpp:142:8:142:8 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:142:8:142:8 | i | i | test.cpp:140:7:140:11 | call to scanf | scanf |
+| test.cpp:174:8:174:8 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:174:8:174:8 | i | i | test.cpp:173:7:173:11 | call to scanf | scanf |
+| test.cpp:193:8:193:8 | j | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 2. | test.cpp:193:8:193:8 | j | j | test.cpp:190:7:190:11 | call to scanf | scanf |
+| test.cpp:214:7:214:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:214:7:214:7 | i | i | test.cpp:213:3:213:7 | call to scanf | scanf |
+| test.cpp:222:7:222:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:222:7:222:7 | i | i | test.cpp:221:3:221:7 | call to scanf | scanf |
+| test.cpp:230:7:230:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:230:7:230:7 | i | i | test.cpp:229:3:229:7 | call to scanf | scanf |
+| test.cpp:242:7:242:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:242:7:242:7 | i | i | test.cpp:241:3:241:7 | call to scanf | scanf |
+| test.cpp:252:8:252:12 | ptr_i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:252:8:252:12 | ptr_i | ptr_i | test.cpp:251:3:251:7 | call to scanf | scanf |
+| test.cpp:260:7:260:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:260:7:260:7 | i | i | test.cpp:259:3:259:7 | call to scanf | scanf |
+| test.cpp:354:25:354:25 | u | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:354:25:354:25 | u | u | test.cpp:353:6:353:11 | call to sscanf | sscanf |

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

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