From e8d637b8c6b2f485689804d8c6d0b50cbefc8357 Mon Sep 17 00:00:00 2001 From: Jacob Ronstadt <147542405+jacob-ronstadt@users.noreply.github.com> Date: Fri, 1 Dec 2023 13:39:47 -0800 Subject: [PATCH] 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 <34494373+NateD-MSFT@users.noreply.github.com> --- .../UnicodeStringFreed/UnicodeStringFreed.ql | 76 ++++++ .../UnicodeStringFreed.qlhelp | 23 ++ .../UnicodeStringFreed.sarif | 219 ++++++++++++++++++ .../UnicodeStringFreed/driver_snippet.c | 40 ++++ .../WDMTestTemplate/driver/fail_driver1.c | 2 +- .../test/build_create_analyze_test.cmd | 1 + .../test/diff/UnicodeStringFreed.sarif | 21 ++ 7 files changed, 381 insertions(+), 1 deletion(-) create mode 100644 src/drivers/general/queries/experimental/UnicodeStringFreed/UnicodeStringFreed.ql create mode 100644 src/drivers/general/queries/experimental/UnicodeStringFreed/UnicodeStringFreed.qlhelp create mode 100644 src/drivers/general/queries/experimental/UnicodeStringFreed/UnicodeStringFreed.sarif create mode 100644 src/drivers/general/queries/experimental/UnicodeStringFreed/driver_snippet.c create mode 100644 src/drivers/test/diff/UnicodeStringFreed.sarif diff --git a/src/drivers/general/queries/experimental/UnicodeStringFreed/UnicodeStringFreed.ql b/src/drivers/general/queries/experimental/UnicodeStringFreed/UnicodeStringFreed.ql new file mode 100644 index 00000000..db589562 --- /dev/null +++ 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 sdat@microsoft.com + * @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; + +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 new file mode 100644 index 00000000..d0ee9f5c --- /dev/null +++ b/src/drivers/general/queries/experimental/UnicodeStringFreed/UnicodeStringFreed.qlhelp @@ -0,0 +1,23 @@ + + + +

+ A UNICODE_STRING pointer that is allocated with RtlCreateUnicodeString is allocated from paged pool and must be freed by calling RtlFreeUnicodeString +

+ + +

+ Ensure that a UNICODE_STRING allocated with RtlCreateUnicodeString is freed using RtlFreeUnicodeString +

+ + + + + +
  • + + RtlCreateUnicodeString function (MSDN) + +
    • +     +     diff --git a/src/drivers/general/queries/experimental/UnicodeStringFreed/UnicodeStringFreed.sarif b/src/drivers/general/queries/experimental/UnicodeStringFreed/UnicodeStringFreed.sarif new file mode 100644 index 00000000..8348e823 --- /dev/null +++ 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" : "sdat@microsoft.com", + "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" + } + } ] +} \ No newline at end of file diff --git a/src/drivers/general/queries/experimental/UnicodeStringFreed/driver_snippet.c b/src/drivers/general/queries/experimental/UnicodeStringFreed/driver_snippet.c new file mode 100644 index 00000000..a13af3b2 --- /dev/null +++ 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 index f90ed298..e5feb084 100644 --- 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 index b2c27cb6..88d368c1 100644 --- 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 diff --git a/src/drivers/test/diff/UnicodeStringFreed.sarif b/src/drivers/test/diff/UnicodeStringFreed.sarif new file mode 100644 index 00000000..8d5a80f9 --- /dev/null +++ b/src/drivers/test/diff/UnicodeStringFreed.sarif @@ -0,0 +1,21 @@ +{ + "all": { + "+": 0, + "-": 0 + }, + "error": { + "+": 0, + "-": 0, + "codes": [] + }, + "warning": { + "+": 0, + "-": 0, + "codes": [] + }, + "note": { + "+": 0, + "-": 0, + "codes": [] + } +} \ No newline at end of file