Unicode string freed query (#95)

* WIP. TODO fix duplicate results * fix global tracking * fix query to find instances where created variable not freed * minor updates * update other files for query * updates for pull request changes. also needed to move the invlude of driver_snippet.c to be the first include in fail_driver1.c so that ntifs.h could be included in driver_snippet.c without errors --------- Co-authored-by: NateD-MSFT <[email protected]>
microsoft · Dec 1, 2023 · e8d637b · e8d637b
1 parent 0976a3c
commit e8d637b
Show file tree

Hide file tree

Showing 7 changed files with 381 additions and 1 deletion.
diff --git a/src/drivers/general/queries/experimental/UnicodeStringFreed/UnicodeStringFreed.ql b/src/drivers/general/queries/experimental/UnicodeStringFreed/UnicodeStringFreed.ql
@@ -0,0 +1,76 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+/**
+ * @id cpp/drivers/unicode-string-not-freed
+ * @name Unicode String Not Freed
+ * @description UnicodeString objects created with RtlCreateUnicodeString must be freed with RtlFreeUnicodeString.
+ * @platform Desktop
+ * @security.severity Medium
+ * @feature.area Multiple
+ * @impact Exploitable Design
+ * @repro.text A UNICODE_STRING object is created with RtlCreateUnicodeString but not freed with RtlFreeUnicodeString.
+ * @owner.email [email protected]
+ * @opaqueid CQLD-D0006
+ * @kind problem
+ * @problem.severity warning
+ * @precision medium
+ * @tags correctness
+ *       wddst
+ * @scope domainspecific
+ * @query-version v1
+ */
+
+
+import cpp
+import semmle.code.cpp.dataflow.new.DataFlow
+
+class UnicodeStringAccess extends VariableAccess {
+  UnicodeStringAccess() { this.getTarget() instanceof UnicodeString }
+}
+
+class UnicodeString extends Variable {
+  UnicodeString() { this.getType().getName() = "PUNICODE_STRING" }
+}
+
+module UnicodeStringDataFlowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    exists(FunctionCall rtlCreate |
+      rtlCreate.getArgument(0).getAChild*() = source.asExpr() and
+      rtlCreate.getTarget().getName() = ("RtlCreateUnicodeString")
+    )
+  }
+
+  predicate isSink(DataFlow::Node sink) {
+    exists(FunctionCall rtlFree |
+      rtlFree.getArgument(0).getAChild*() = sink.asExpr() and
+      rtlFree.getTarget().getName() = ("RtlFreeUnicodeString")
+    )
+  }
+
+  predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(FunctionCall fc, DataFlow::VariableNode var |
+      fc.getArgument(0).getAChild*() = pred.asExpr() and
+      (
+        fc.getTarget().getName() = ("RtlFreeUnicodeString")
+        or
+        fc.getTarget().getName() = ("RtlCreateUnicodeString")
+      ) and
+      fc.getArgument(0).getAChild*() instanceof UnicodeStringAccess
+    |
+      succ = var and
+      var.asVariable() instanceof UnicodeString and
+      var.asVariable() instanceof GlobalVariable and
+      var.asVariable().getName() = fc.getArgument(0).(VariableAccess).getTarget().getName()
+    )
+  }
+}
+
+module Flow = DataFlow::Global<UnicodeStringDataFlowConfig>;
+
+from DataFlow::Node source
+where
+  not exists(DataFlow::Node sink | Flow::flow(source, sink)) and
+  UnicodeStringDataFlowConfig::isSource(source)
+select source,
+  "PUNICODE_STRING object $@ created with RtlCreateUnicodeString but not freed with RtlFreeUnicodeString",
+  source.asExpr(), source.asExpr().toString()
diff --git a/src/drivers/general/queries/experimental/UnicodeStringFreed/UnicodeStringFreed.qlhelp b/src/drivers/general/queries/experimental/UnicodeStringFreed/UnicodeStringFreed.qlhelp
@@ -0,0 +1,23 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+	<overview>
+		<p>
+			A UNICODE_STRING pointer that is allocated with RtlCreateUnicodeString is allocated from paged pool and must be freed by calling RtlFreeUnicodeString
+		</p>
+	</overview>
+	<recommendation>
+		<p>
+			Ensure that a UNICODE_STRING allocated with RtlCreateUnicodeString is freed using RtlFreeUnicodeString
+		</p>
+	</recommendation>
+	<example>
+		<sample src="driver_snippet.c" />
+	</example>
+	<references>
+		<li>
+			<a href="https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/nf-ntifs-rtlcreateunicodestring">
+				RtlCreateUnicodeString function (MSDN)
+			</a>
+		</li>
+	</references>
+</qhelp>
diff --git a/src/drivers/general/queries/experimental/UnicodeStringFreed/UnicodeStringFreed.sarif b/src/drivers/general/queries/experimental/UnicodeStringFreed/UnicodeStringFreed.sarif
@@ -0,0 +1,219 @@
+{
+  "$schema" : "https://json.schemastore.org/sarif-2.1.0.json",
+  "version" : "2.1.0",
+  "runs" : [ {
+    "tool" : {
+      "driver" : {
+        "name" : "CodeQL",
+        "organization" : "GitHub",
+        "semanticVersion" : "2.14.6",
+        "notifications" : [ {
+          "id" : "cpp/baseline/expected-extracted-files",
+          "name" : "cpp/baseline/expected-extracted-files",
+          "shortDescription" : {
+            "text" : "Expected extracted files"
+          },
+          "fullDescription" : {
+            "text" : "Files appearing in the source archive that are expected to be extracted."
+          },
+          "defaultConfiguration" : {
+            "enabled" : true
+          },
+          "properties" : {
+            "tags" : [ "expected-extracted-files", "telemetry" ]
+          }
+        } ],
+        "rules" : [ {
+          "id" : "cpp/drivers/unicode-string-not-freed",
+          "name" : "cpp/drivers/unicode-string-not-freed",
+          "shortDescription" : {
+            "text" : "Unicode String Not Freed"
+          },
+          "fullDescription" : {
+            "text" : "UnicodeString objects created with RtlCreateUnicodeString must be freed with RtlFreeUnicodeString."
+          },
+          "defaultConfiguration" : {
+            "enabled" : true,
+            "level" : "warning"
+          },
+          "properties" : {
+            "tags" : [ "correctness", "wddst" ],
+            "description" : "UnicodeString objects created with RtlCreateUnicodeString must be freed with RtlFreeUnicodeString.",
+            "feature.area" : "Multiple",
+            "id" : "cpp/drivers/unicode-string-not-freed",
+            "impact" : "Exploitable Design",
+            "kind" : "problem",
+            "name" : "Unicode String Not Freed",
+            "opaqueid" : "CQLD-D0006",
+            "owner.email" : "[email protected]",
+            "platform" : "Desktop",
+            "precision" : "medium",
+            "problem.severity" : "warning",
+            "query-version" : "v1",
+            "repro.text" : "A UNICODE_STRING object is created with RtlCreateUnicodeString but not freed with RtlFreeUnicodeString.",
+            "scope" : "domainspecific",
+            "security.severity" : "Medium"
+          }
+        } ]
+      },
+      "extensions" : [ {
+        "name" : "microsoft/windows-drivers",
+        "semanticVersion" : "0.2.0+60b1cef4f56fea2f366724a51097b65da6b795b9",
+        "locations" : [ {
+          "uri" : "file:///C:/codeql-home/Windows-Driver-Developer-Supplemental-Tools/src/",
+          "description" : {
+            "text" : "The QL pack root directory."
+          }
+        }, {
+          "uri" : "file:///C:/codeql-home/Windows-Driver-Developer-Supplemental-Tools/src/qlpack.yml",
+          "description" : {
+            "text" : "The QL pack definition file."
+          }
+        } ]
+      } ]
+    },
+    "invocations" : [ {
+      "toolExecutionNotifications" : [ {
+        "locations" : [ {
+          "physicalLocation" : {
+            "artifactLocation" : {
+              "uri" : "driver/driver_snippet.c",
+              "uriBaseId" : "%SRCROOT%",
+              "index" : 0
+            }
+          }
+        } ],
+        "message" : {
+          "text" : ""
+        },
+        "level" : "none",
+        "descriptor" : {
+          "id" : "cpp/baseline/expected-extracted-files",
+          "index" : 0
+        },
+        "properties" : {
+          "formattedMessage" : {
+            "text" : ""
+          }
+        }
+      }, {
+        "locations" : [ {
+          "physicalLocation" : {
+            "artifactLocation" : {
+              "uri" : "driver/fail_driver1.c",
+              "uriBaseId" : "%SRCROOT%",
+              "index" : 1
+            }
+          }
+        } ],
+        "message" : {
+          "text" : ""
+        },
+        "level" : "none",
+        "descriptor" : {
+          "id" : "cpp/baseline/expected-extracted-files",
+          "index" : 0
+        },
+        "properties" : {
+          "formattedMessage" : {
+            "text" : ""
+          }
+        }
+      }, {
+        "locations" : [ {
+          "physicalLocation" : {
+            "artifactLocation" : {
+              "uri" : "driver/fail_driver1.h",
+              "uriBaseId" : "%SRCROOT%",
+              "index" : 2
+            }
+          }
+        } ],
+        "message" : {
+          "text" : ""
+        },
+        "level" : "none",
+        "descriptor" : {
+          "id" : "cpp/baseline/expected-extracted-files",
+          "index" : 0
+        },
+        "properties" : {
+          "formattedMessage" : {
+            "text" : ""
+          }
+        }
+      } ],
+      "executionSuccessful" : true
+    } ],
+    "artifacts" : [ {
+      "location" : {
+        "uri" : "driver/driver_snippet.c",
+        "uriBaseId" : "%SRCROOT%",
+        "index" : 0
+      }
+    }, {
+      "location" : {
+        "uri" : "driver/fail_driver1.c",
+        "uriBaseId" : "%SRCROOT%",
+        "index" : 1
+      }
+    }, {
+      "location" : {
+        "uri" : "driver/fail_driver1.h",
+        "uriBaseId" : "%SRCROOT%",
+        "index" : 2
+      }
+    } ],
+    "results" : [ {
+      "ruleId" : "cpp/drivers/unicode-string-not-freed",
+      "ruleIndex" : 0,
+      "rule" : {
+        "id" : "cpp/drivers/unicode-string-not-freed",
+        "index" : 0
+      },
+      "message" : {
+        "text" : "PUNICODE_STRING object [unicodeStringNotFreed](1) created with RtlCreateUnicodeString but not freed with RtlFreeUnicodeString"
+      },
+      "locations" : [ {
+        "physicalLocation" : {
+          "artifactLocation" : {
+            "uri" : "driver/driver_snippet.c",
+            "uriBaseId" : "%SRCROOT%",
+            "index" : 0
+          },
+          "region" : {
+            "startLine" : 31,
+            "startColumn" : 28,
+            "endColumn" : 49
+          }
+        }
+      } ],
+      "partialFingerprints" : {
+        "primaryLocationLineHash" : "e56c61209e3933d3:1",
+        "primaryLocationStartColumnFingerprint" : "23"
+      },
+      "relatedLocations" : [ {
+        "id" : 1,
+        "physicalLocation" : {
+          "artifactLocation" : {
+            "uri" : "driver/driver_snippet.c",
+            "uriBaseId" : "%SRCROOT%",
+            "index" : 0
+          },
+          "region" : {
+            "startLine" : 31,
+            "startColumn" : 28,
+            "endColumn" : 49
+          }
+        },
+        "message" : {
+          "text" : "unicodeStringNotFreed"
+        }
+      } ]
+    } ],
+    "columnKind" : "utf16CodeUnits",
+    "properties" : {
+      "semmle.formatSpecifier" : "sarifv2.1.0"
+    }
+  } ]
+}
diff --git a/src/drivers/general/queries/experimental/UnicodeStringFreed/driver_snippet.c b/src/drivers/general/queries/experimental/UnicodeStringFreed/driver_snippet.c
@@ -0,0 +1,40 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+//
+// driver_snippet.c
+//
+#include "ntifs.h"
+
+#define SET_DISPATCH 1
+// Template. Not called in this test.
+void top_level_call() {}
+PUNICODE_STRING unicodeStringGlobal;
+void free_unicode_string(PUNICODE_STRING unicodeStr)
+{
+    RtlFreeUnicodeString(unicodeStr);
+}
+
+void free_global_unicode_string()
+{
+    RtlFreeUnicodeString(unicodeStringGlobal);
+
+}
+
+void create_unicode_string(void)
+{
+    PUNICODE_STRING unicodeStringLocal = NULL;
+    PUNICODE_STRING unicodeStringNotFreed = NULL;
+    PUNICODE_STRING unicodeStringArg = NULL;
+
+    PCWSTR sourceString = L"Hello World";
+    RtlCreateUnicodeString(unicodeStringLocal, sourceString);
+    RtlCreateUnicodeString(unicodeStringNotFreed, sourceString);
+    RtlCreateUnicodeString(unicodeStringArg, sourceString);
+    RtlCreateUnicodeString(unicodeStringGlobal, sourceString);
+
+
+    RtlFreeUnicodeString(unicodeStringLocal);
+
+    free_unicode_string(unicodeStringArg);
+
+}
diff --git a/src/drivers/test/WDMTestTemplate/driver/fail_driver1.c b/src/drivers/test/WDMTestTemplate/driver/fail_driver1.c
@@ -21,9 +21,9 @@ Module Name:
 
 --*/
 
+#include "driver_snippet.c"
 #include "fail_driver1.h"
 
-#include "driver_snippet.c"
 
 #define _DRIVER_NAME_ "fail_driver1"
 

diff --git a/src/drivers/test/build_create_analyze_test.cmd b/src/drivers/test/build_create_analyze_test.cmd
@@ -33,6 +33,7 @@ call :test IllegalFieldAccess2 WDMTestTemplate wdm queries
 call :test RoutineFunctionTypeNotExpected WDMTestTemplate general queries
 call :test KeSetEventIrql WDMTestTemplate general queries\experimental
 call :test KeSetEventPageable WDMTestTemplate general queries
+call :test UnicodeStringFreed WDMTestTemplate general queries\experimental
 
 exit /b 0