Merge pull request #3921 from catenacyber/NullCheckParam

C++: Adds another redundant null check rule

Author: jbj

cpp/ql/src/experimental/Likely Bugs/RedundantNullCheckParam.cpp

@@ -0,0 +1,11 @@
+void test(char *arg1, int *arg2) {
+    if (arg1[0] == 'A') {
+        if (arg2 != NULL) { //maybe redundant
+            *arg2 = 42;
+        }
+    }
+    if (arg1[1] == 'B')
+    {
+        *arg2 = 54; //dereferenced without checking first
+    }
+}

cpp/ql/src/experimental/Likely Bugs/RedundantNullCheckParam.qhelp

@@ -0,0 +1,29 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>This rule finds comparisons of a function parameter to null that occur when in another path the parameter is dereferenced without a guard check.  It's
+likely either the check is not required and can be removed, or it should be added before the dereference
+so that a null pointer dereference does not occur.</p>
+</overview>
+
+<recommendation>
+<p>A check should be added to before the dereference, in a way that prevents a null pointer value from
+being dereferenced.  If it's clear that the pointer cannot be null, consider removing the check instead.</p>
+</recommendation>
+
+<example>
+<sample src="RedundantNullCheckParam.cpp" />
+</example>
+
+<references>
+<li>
+  <a href="https://www.owasp.org/index.php/Null_Dereference">
+    Null Dereference
+  </a> 
+</li>
+</references>
+
+</qhelp>

cpp/ql/src/experimental/Likely Bugs/RedundantNullCheckParam.ql

@@ -0,0 +1,56 @@
+/**
+ * @name Redundant null check or missing null check of parameter
+ * @description Checking a parameter for nullness in one path,
+ *              and not in another is likely to be a sign that either
+ *              the check can be removed, or added in the other case.
+ * @kind problem
+ * @id cpp/redundant-null-check-param
+ * @problem.severity recommendation
+ * @tags reliability
+ *       security
+ *       external/cwe/cwe-476
+ */
+
+import cpp
+
+predicate blockDominates(Block check, Block access) {
+  check.getLocation().getStartLine() <= access.getLocation().getStartLine() and
+  check.getLocation().getEndLine() >= access.getLocation().getEndLine()
+}
+
+predicate isCheckedInstruction(VariableAccess unchecked, VariableAccess checked) {
+  checked = any(VariableAccess va | va.getTarget() = unchecked.getTarget()) and
+  //Simple test if the first access in this code path is dereferenced
+  not dereferenced(checked) and
+  blockDominates(checked.getEnclosingBlock(), unchecked.getEnclosingBlock())
+}
+
+predicate candidateResultUnchecked(VariableAccess unchecked) {
+  not isCheckedInstruction(unchecked, _)
+}
+
+predicate candidateResultChecked(VariableAccess check, EqualityOperation eqop) {
+  //not dereferenced to check against pointer, not its pointed value
+  not dereferenced(check) and
+  //assert macros are not taken into account
+  not check.isInMacroExpansion() and
+  // is part of a comparison against some constant NULL
+  eqop.getAnOperand() = check and
+  eqop.getAnOperand() instanceof NullValue
+}
+
+from VariableAccess unchecked, VariableAccess check, EqualityOperation eqop, Parameter param
+where
+  // a dereference
+  dereferenced(unchecked) and
+  // for a function parameter
+  unchecked.getTarget() = param and
+  // this function parameter is not overwritten
+  count(param.getAnAssignment()) = 0 and
+  check.getTarget() = param and
+  // which is once checked
+  candidateResultChecked(check, eqop) and
+  // and which has not been checked before in this code path
+  candidateResultUnchecked(unchecked)
+select check, "This null check is redundant or there is a missing null check before $@ ", unchecked,
+  "where dereferencing happens"