EssentialTypes: Implement Rule 21.14

lcartey · lcartey · commit b38552c78ed7 · 2023-02-15T23:04:40.000Z
Adds a query to detect the use of memcmp to compare null-terminated
strings, using global data flow from hard-coded string literals or
array literals.
diff --git a/c/misra/src/rules/RULE-21-14/MemcmpUsedToCompareNullTerminatedStrings.ql b/c/misra/src/rules/RULE-21-14/MemcmpUsedToCompareNullTerminatedStrings.ql
@@ -0,0 +1,80 @@
+/**
+ * @id c/misra/memcmp-used-to-compare-null-terminated-strings
+ * @name RULE-21-14: The Standard Library function memcmp shall not be used to compare null terminated strings
+ * @description Using memcmp to compare null terminated strings may give unexpected results because
+ *              memcmp compares by size with no consideration for the null terminator.
+ * @kind path-problem
+ * @precision very-high
+ * @problem.severity error
+ * @tags external/misra/id/rule-21-14
+ *       maintainability
+ *       correctness
+ *       external/misra/obligation/required
+ */
+
+import cpp
+import codingstandards.c.misra
+import codingstandards.c.misra.EssentialTypes
+import semmle.code.cpp.dataflow.TaintTracking
+import DataFlow::PathGraph
+
+// Data flow from a StringLiteral or from an array of characters, to a memcmp call
+class NullTerminatedStringToMemcmpConfiguration extends TaintTracking::Configuration {
+  NullTerminatedStringToMemcmpConfiguration() { this = "NullTerminatedStringToMemcmpConfiguration" }
+
+  override predicate isSource(DataFlow::Node source) {
+    source.asExpr() instanceof StringLiteral
+    or
+    exists(Variable v, ArrayAggregateLiteral aal |
+      aal = v.getInitializer().getExpr() and
+      // The array element type is an essentially character type
+      getEssentialTypeCategory(aal.getElementType()) = EssentiallyCharacterType() and
+      // Includes a null terminator somewhere in the array initializer
+      aal.getElementExpr(_).getValue().toInt() = 0
+    |
+      // For local variables, use the array aggregate literal as the source
+      aal = source.asExpr()
+      or
+      // ArrayAggregateLiterals used as initializers for global variables are not viable sources
+      // for global data flow, so we instead report variable accesses as sources, where the variable
+      // is constant or is not assigned in the program
+      v instanceof GlobalVariable and
+      source.asExpr() = v.getAnAccess() and
+      (
+        v.isConst()
+        or
+        not exists(Expr e | e = v.getAnAssignedValue() and not e = aal)
+      )
+    )
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(FunctionCall memcmp |
+      memcmp.getTarget().hasGlobalOrStdName("memcmp") and
+      sink.asExpr() = memcmp.getArgument([0, 1])
+    )
+  }
+}
+
+from
+  FunctionCall memcmp, DataFlow::PathNode source, DataFlow::PathNode sink,
+  DataFlow::PathNode source1, DataFlow::PathNode arg1, DataFlow::PathNode source2,
+  DataFlow::PathNode arg2
+where
+  not isExcluded(memcmp, EssentialTypesPackage::memcmpUsedToCompareNullTerminatedStringsQuery()) and
+  memcmp.getTarget().hasGlobalOrStdName("memcmp") and
+  arg1.getNode().asExpr() = memcmp.getArgument(0) and
+  arg2.getNode().asExpr() = memcmp.getArgument(1) and
+  // There is a path from a null-terminated string to each argument
+  exists(NullTerminatedStringToMemcmpConfiguration cfg |
+    cfg.hasFlowPath(source1, arg1) and
+    cfg.hasFlowPath(source2, arg2)
+  ) and
+  // Produce multiple paths for each result, one for each source/arg pair
+  (
+    source = source1 and sink = arg1
+    or
+    source = source2 and sink = arg2
+  )
+select memcmp, source, sink, "memcmp used to compare $@ with $@.", source1,
+  "null-terminated string", source2, "null-terminated string"
diff --git a/c/misra/test/rules/RULE-21-14/MemcmpUsedToCompareNullTerminatedStrings.expected b/c/misra/test/rules/RULE-21-14/MemcmpUsedToCompareNullTerminatedStrings.expected
@@ -0,0 +1,34 @@
+edges
+| test.c:12:13:12:15 | a | test.c:14:10:14:10 | a |
+| test.c:12:13:12:15 | a | test.c:23:13:23:13 | a |
+| test.c:12:13:12:15 | a | test.c:24:10:24:10 | a |
+| test.c:13:13:13:15 | b | test.c:14:13:14:13 | b |
+| test.c:18:15:18:28 | {...} | test.c:21:10:21:10 | e |
+| test.c:19:15:19:28 | {...} | test.c:21:13:21:13 | f |
+nodes
+| test.c:10:10:10:12 | a | semmle.label | a |
+| test.c:10:15:10:17 | b | semmle.label | b |
+| test.c:12:13:12:15 | a | semmle.label | a |
+| test.c:13:13:13:15 | b | semmle.label | b |
+| test.c:14:10:14:10 | a | semmle.label | a |
+| test.c:14:13:14:13 | b | semmle.label | b |
+| test.c:16:10:16:10 | c | semmle.label | c |
+| test.c:16:13:16:13 | d | semmle.label | d |
+| test.c:18:15:18:28 | {...} | semmle.label | {...} |
+| test.c:19:15:19:28 | {...} | semmle.label | {...} |
+| test.c:21:10:21:10 | e | semmle.label | e |
+| test.c:21:13:21:13 | f | semmle.label | f |
+| test.c:23:13:23:13 | a | semmle.label | a |
+| test.c:24:10:24:10 | a | semmle.label | a |
+| test.c:26:13:26:13 | c | semmle.label | c |
+| test.c:27:10:27:10 | c | semmle.label | c |
+subpaths
+#select
+| test.c:10:3:10:8 | call to memcmp | test.c:10:10:10:12 | a | test.c:10:10:10:12 | a | memcmp used to compare $@ with $@. | test.c:10:10:10:12 | a | null-terminated string | test.c:10:15:10:17 | b | null-terminated string |
+| test.c:10:3:10:8 | call to memcmp | test.c:10:15:10:17 | b | test.c:10:15:10:17 | b | memcmp used to compare $@ with $@. | test.c:10:10:10:12 | a | null-terminated string | test.c:10:15:10:17 | b | null-terminated string |
+| test.c:14:3:14:8 | call to memcmp | test.c:12:13:12:15 | a | test.c:14:10:14:10 | a | memcmp used to compare $@ with $@. | test.c:12:13:12:15 | a | null-terminated string | test.c:13:13:13:15 | b | null-terminated string |
+| test.c:14:3:14:8 | call to memcmp | test.c:13:13:13:15 | b | test.c:14:13:14:13 | b | memcmp used to compare $@ with $@. | test.c:12:13:12:15 | a | null-terminated string | test.c:13:13:13:15 | b | null-terminated string |
+| test.c:16:3:16:8 | call to memcmp | test.c:16:10:16:10 | c | test.c:16:10:16:10 | c | memcmp used to compare $@ with $@. | test.c:16:10:16:10 | c | null-terminated string | test.c:16:13:16:13 | d | null-terminated string |
+| test.c:16:3:16:8 | call to memcmp | test.c:16:13:16:13 | d | test.c:16:13:16:13 | d | memcmp used to compare $@ with $@. | test.c:16:10:16:10 | c | null-terminated string | test.c:16:13:16:13 | d | null-terminated string |
+| test.c:21:3:21:8 | call to memcmp | test.c:18:15:18:28 | {...} | test.c:21:10:21:10 | e | memcmp used to compare $@ with $@. | test.c:18:15:18:28 | {...} | null-terminated string | test.c:19:15:19:28 | {...} | null-terminated string |
+| test.c:21:3:21:8 | call to memcmp | test.c:19:15:19:28 | {...} | test.c:21:13:21:13 | f | memcmp used to compare $@ with $@. | test.c:18:15:18:28 | {...} | null-terminated string | test.c:19:15:19:28 | {...} | null-terminated string |
diff --git a/c/misra/test/rules/RULE-21-14/MemcmpUsedToCompareNullTerminatedStrings.qlref b/c/misra/test/rules/RULE-21-14/MemcmpUsedToCompareNullTerminatedStrings.qlref
@@ -0,0 +1 @@
+rules/RULE-21-14/MemcmpUsedToCompareNullTerminatedStrings.ql
diff --git a/c/misra/test/rules/RULE-21-14/test.c b/c/misra/test/rules/RULE-21-14/test.c
@@ -0,0 +1,28 @@
+#include <string.h>
+
+extern char a[10];
+extern char b[10];
+
+char c[10] = {'a', 'b', 0};
+char d[10] = {'a', 'b', 0};
+
+void testCmp(int *p) {
+  memcmp("a", "b", 1); // NON_COMPLIANT
+
+  strcpy(a, "a");
+  strcpy(b, "b");
+  memcmp(a, b, 1); // NON_COMPLIANT
+
+  memcmp(c, d, 1); // NON_COMPLIANT
+
+  char e[10] = {'a', 'b', 0};
+  char f[10] = {'a', 'b', 0};
+
+  memcmp(e, f, 1); // NON_COMPLIANT
+
+  memcmp(p, a, 1); // COMPLIANT
+  memcmp(a, p, 1); // COMPLIANT
+
+  memcmp(p, c, 1); // COMPLIANT
+  memcmp(c, p, 1); // COMPLIANT
+}
diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/EssentialTypes.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/EssentialTypes.qll
@@ -14,6 +14,7 @@ newtype EssentialTypesQuery =
   TImplicitConversionOfCompositeExpressionQuery() or
   TInappropriateCastOfCompositeExpressionQuery() or
   TLoopOverEssentiallyFloatTypeQuery() or
+  TMemcmpUsedToCompareNullTerminatedStringsQuery() or
   TMemcmpOnInappropriateEssentialTypeArgsQuery()
 
 predicate isEssentialTypesQueryMetadata(Query query, string queryId, string ruleId, string category) {
@@ -107,6 +108,15 @@ predicate isEssentialTypesQueryMetadata(Query query, string queryId, string rule
   ruleId = "RULE-14-1" and
   category = "required"
   or
+  query =
+    // `Query` instance for the `memcmpUsedToCompareNullTerminatedStrings` query
+    EssentialTypesPackage::memcmpUsedToCompareNullTerminatedStringsQuery() and
+  queryId =
+    // `@id` for the `memcmpUsedToCompareNullTerminatedStrings` query
+    "c/misra/memcmp-used-to-compare-null-terminated-strings" and
+  ruleId = "RULE-21-14" and
+  category = "required"
+  or
   query =
     // `Query` instance for the `memcmpOnInappropriateEssentialTypeArgs` query
     EssentialTypesPackage::memcmpOnInappropriateEssentialTypeArgsQuery() and
@@ -188,6 +198,13 @@ module EssentialTypesPackage {
       TQueryC(TEssentialTypesPackageQuery(TLoopOverEssentiallyFloatTypeQuery()))
   }
 
+  Query memcmpUsedToCompareNullTerminatedStringsQuery() {
+    //autogenerate `Query` type
+    result =
+      // `Query` type for `memcmpUsedToCompareNullTerminatedStrings` query
+      TQueryC(TEssentialTypesPackageQuery(TMemcmpUsedToCompareNullTerminatedStringsQuery()))
+  }
+
   Query memcmpOnInappropriateEssentialTypeArgsQuery() {
     //autogenerate `Query` type
     result =
diff --git a/rule_packages/c/EssentialTypes.json b/rule_packages/c/EssentialTypes.json
@@ -190,6 +190,26 @@
       ],
       "title": "A loop counter shall not have essentially floating type"
     },
+    "RULE-21-14": {
+      "properties": {
+        "obligation": "required"
+      },
+      "queries": [
+        {
+          "description": "Using memcmp to compare null terminated strings may give unexpected results because memcmp compares by size with no consideration for the null terminator.",
+          "kind": "path-problem",
+          "name": "The Standard Library function memcmp shall not be used to compare null terminated strings",
+          "precision": "very-high",
+          "severity": "error",
+          "short_name": "MemcmpUsedToCompareNullTerminatedStrings",
+          "tags": [
+            "maintainability",
+            "correctness"
+          ]
+        }
+      ],
+      "title": "The Standard Library function memcmp shall not be used to compare null terminated strings"
+    },
     "RULE-21-16": {
       "properties": {
         "obligation": "required"
diff --git a/rules.csv b/rules.csv
@@ -756,7 +756,7 @@ c,MISRA-C-2012,RULE-21-10,Yes,Required,,,The Standard Library time and date func
 c,MISRA-C-2012,RULE-21-11,Yes,Required,,,The standard header file <tgmath.h> shall not be used,,Banned,Easy,
 c,MISRA-C-2012,RULE-21-12,Yes,Advisory,,,The exception handling features of <fenv.h> should not be used,,Banned,Easy,
 c,MISRA-C-2012,RULE-21-13,Yes,Mandatory,,,Any value passed to a function in <ctype.h> shall be representable as an unsigned char or be the value EOF,,Types,Medium,
-c,MISRA-C-2012,RULE-21-14,Yes,Required,,,The Standard Library function memcmp shall not be used to compare null terminated strings,,Types,Hard,
+c,MISRA-C-2012,RULE-21-14,Yes,Required,,,The Standard Library function memcmp shall not be used to compare null terminated strings,,EssentialTypes,Hard,
 c,MISRA-C-2012,RULE-21-15,Yes,Required,,,"The pointer arguments to the Standard Library functions memcpy, memmove and memcmp shall be pointers to qualified or unqualified versions of compatible types",,Types,Medium,
 c,MISRA-C-2012,RULE-21-16,Yes,Required,,,"The pointer arguments to the Standard Library function memcmp shall point to either a pointer type, an essentially signed type, an essentially unsigned type, an essentially Boolean type or an essentially enum type",,EssentialTypes,Medium,
 c,MISRA-C-2012,RULE-21-17,Yes,Mandatory,,,Use of the string handling functions from <string.h> shall not result in accesses beyond the bounds of the objects referenced by their pointer parameters,,Memory2,Hard,

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+rules/RULE-21-14/MemcmpUsedToCompareNullTerminatedStrings.ql`