From 53bb6fe7aa7dceaa518cc203a9d398b18d0980ab Mon Sep 17 00:00:00 2001 From: Jacob Ronstadt <147542405+jacob-ronstadt@users.noreply.github.com> Date: Fri, 23 Aug 2024 14:02:35 -0700 Subject: [PATCH] DriverEntrySaveBuffer: Port of c28131 (#122) * C28131 * updates to ql * update id and move out of experimental * add query to ported ca checks suite * update to also check for local pointers to structs --------- Signed-off-by: Jacob Ronstadt <147542405+jacob-ronstadt@users.noreply.github.com> --- .../DriverEntrySaveBuffer.qhelp | 23 ++ .../DriverEntrySaveBuffer.ql | 41 +++ .../DriverEntrySaveBuffer.sarif | 347 ++++++++++++++++++ .../DriverEntrySaveBuffer/driver_snippet.c | 66 ++++ .../test/diff/DriverEntrySaveBuffer.sarif | 21 ++ .../ported_driver_ca_checks.qls | 1 + 6 files changed, 499 insertions(+) create mode 100644 src/drivers/general/queries/DriverEntrySaveBuffer/DriverEntrySaveBuffer.qhelp create mode 100644 src/drivers/general/queries/DriverEntrySaveBuffer/DriverEntrySaveBuffer.ql create mode 100644 src/drivers/general/queries/DriverEntrySaveBuffer/DriverEntrySaveBuffer.sarif create mode 100644 src/drivers/general/queries/DriverEntrySaveBuffer/driver_snippet.c create mode 100644 src/drivers/test/diff/DriverEntrySaveBuffer.sarif diff --git a/src/drivers/general/queries/DriverEntrySaveBuffer/DriverEntrySaveBuffer.qhelp b/src/drivers/general/queries/DriverEntrySaveBuffer/DriverEntrySaveBuffer.qhelp new file mode 100644 index 00000000..64f43191 --- /dev/null +++ b/src/drivers/general/queries/DriverEntrySaveBuffer/DriverEntrySaveBuffer.qhelp @@ -0,0 +1,23 @@ + + + +

+ The DriverEntry routine should save a copy of the argument, not the pointer, because the I/O Manager frees the buffer +

+ + +

+ The driver's DriverEntry routine is saving a copy of the pointer to the buffer instead of saving a copy of the buffer. Because the buffer is freed when the DriverEntry routine returns, the pointer to the buffer will soon be invalid. +

+ + + + + +
  • + + C28131 + +
    • +     +     diff --git a/src/drivers/general/queries/DriverEntrySaveBuffer/DriverEntrySaveBuffer.ql b/src/drivers/general/queries/DriverEntrySaveBuffer/DriverEntrySaveBuffer.ql new file mode 100644 index 00000000..f5f23c11 --- /dev/null +++ b/src/drivers/general/queries/DriverEntrySaveBuffer/DriverEntrySaveBuffer.ql @@ -0,0 +1,41 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** + * @id cpp/drivers/driver-entry-save-buffer + * @name Driver Entry Save Buffer + * @description C28131: The DriverEntry routine should save a copy of the argument, not the pointer, because the I/O Manager frees the buffer + * @platform Desktop + * @security.severity Medium + * @feature.area Multiple + * @impact Exploitable Design + * @repro.text + * @owner.email sdat@microsoft.com + * @opaqueid CQLD-C28131 + * @kind problem + * @problem.severity warning + * @precision medium + * @tags correctness + * wddst + * @scope domainspecific + * @query-version v1 + */ + +import cpp + +from VariableAccess va +where + va.getParent() instanceof AssignExpr and + exists(Parameter p | p.getAnAccess() = va and p.getFunction().getName().matches("DriverEntry%")) and + ( + exists(GlobalVariable gv | + gv = va.getParent().(AssignExpr).getLValue().(VariableAccess).getTarget() + ) + or + exists(FieldAccess fa | + fa.getTarget() = va.getParent().(AssignExpr).getLValue().(VariableAccess).getTarget() and + fa.getQualifier().(VariableAccess).getTarget() instanceof LocalVariable + ) + ) +select va, + "The DriverEntry routine should save a copy of the argument $@, not the pointer, because the I/O Manager frees the buffer", + va, va.toString() diff --git a/src/drivers/general/queries/DriverEntrySaveBuffer/DriverEntrySaveBuffer.sarif b/src/drivers/general/queries/DriverEntrySaveBuffer/DriverEntrySaveBuffer.sarif new file mode 100644 index 00000000..0a9d1503 --- /dev/null +++ b/src/drivers/general/queries/DriverEntrySaveBuffer/DriverEntrySaveBuffer.sarif @@ -0,0 +1,347 @@ +{ + "$schema": "https://json.schemastore.org/sarif-2.1.0.json", + "version": "2.1.0", + "runs": [ + { + "tool": { + "driver": { + "name": "CodeQL", + "organization": "GitHub", + "semanticVersion": "2.17.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" + ] + } + }, + { + "id": "cpp/extractor/summary", + "name": "cpp/extractor/summary", + "shortDescription": { + "text": "C++ extractor telemetry" + }, + "fullDescription": { + "text": "C++ extractor telemetry" + }, + "defaultConfiguration": { + "enabled": true + } + } + ], + "rules": [ + { + "id": "cpp/drivers/driver-entry-save-buffer", + "name": "cpp/drivers/driver-entry-save-buffer", + "shortDescription": { + "text": "Driver Entry Save Buffer" + }, + "fullDescription": { + "text": "C28131: The DriverEntry routine should save a copy of the argument, not the pointer, because the I/O Manager frees the buffer" + }, + "defaultConfiguration": { + "enabled": true, + "level": "warning" + }, + "properties": { + "tags": [ + "correctness", + "wddst" + ], + "description": "C28131: The DriverEntry routine should save a copy of the argument, not the pointer, because the I/O Manager frees the buffer", + "feature.area": "Multiple", + "id": "cpp/drivers/driver-entry-save-buffer", + "impact": "Exploitable Design", + "kind": "problem", + "name": "Driver Entry Save Buffer", + "opaqueid": "CQLD-C28131", + "owner.email": "sdat@microsoft.com", + "platform": "Desktop", + "precision": "medium", + "problem.severity": "warning", + "query-version": "v1", + "repro.text": "", + "scope": "domainspecific", + "security.severity": "Medium" + } + } + ] + }, + "extensions": [ + { + "name": "microsoft/windows-drivers", + "semanticVersion": "1.1.0+51a61366e45774d4f272190ede40570b925b7a47", + "locations": [ + { + "uri": "file:///C:/codeql-home/WDDST/src/", + "description": { + "text": "The QL pack root directory." + } + }, + { + "uri": "file:///C:/codeql-home/WDDST/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.h", + "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.c", + "uriBaseId": "%SRCROOT%", + "index": 2 + } + } + } + ], + "message": { + "text": "" + }, + "level": "none", + "descriptor": { + "id": "cpp/baseline/expected-extracted-files", + "index": 0 + }, + "properties": { + "formattedMessage": { + "text": "" + } + } + }, + { + "message": { + "text": "Internal telemetry for the C++ extractor.\n\nNo action needed.", + "markdown": "Internal telemetry for the C++ extractor.\n\nNo action needed." + }, + "level": "note", + "timeUtc": "2024-08-24T05:00:01.909+00:00", + "descriptor": { + "id": "cpp/extractor/summary", + "index": 1 + }, + "properties": { + "attributes": { + "cache-hits": 0, + "cache-misses": 1, + "extractor-failures": 1, + "extractor-successes": 0, + "trap-caching": "disabled" + }, + "visibility": { + "statusPage": false, + "telemetry": true + } + } + } + ], + "executionSuccessful": true + } + ], + "artifacts": [ + { + "location": { + "uri": "driver/driver_snippet.c", + "uriBaseId": "%SRCROOT%", + "index": 0 + } + }, + { + "location": { + "uri": "driver/fail_driver1.h", + "uriBaseId": "%SRCROOT%", + "index": 1 + } + }, + { + "location": { + "uri": "driver/fail_driver1.c", + "uriBaseId": "%SRCROOT%", + "index": 2 + } + } + ], + "results": [ + { + "ruleId": "cpp/drivers/driver-entry-save-buffer", + "ruleIndex": 0, + "rule": { + "id": "cpp/drivers/driver-entry-save-buffer", + "index": 0 + }, + "message": { + "text": "The DriverEntry routine should save a copy of the argument [RegistryPath](1), not the pointer, because the I/O Manager frees the buffer" + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "driver/driver_snippet.c", + "uriBaseId": "%SRCROOT%", + "index": 0 + }, + "region": { + "startLine": 64, + "startColumn": 23, + "endColumn": 35 + } + } + } + ], + "partialFingerprints": { + "primaryLocationLineHash": "4d740d3f2799e655:1", + "primaryLocationStartColumnFingerprint": "18" + }, + "relatedLocations": [ + { + "id": 1, + "physicalLocation": { + "artifactLocation": { + "uri": "driver/driver_snippet.c", + "uriBaseId": "%SRCROOT%", + "index": 0 + }, + "region": { + "startLine": 64, + "startColumn": 23, + "endColumn": 35 + } + }, + "message": { + "text": "RegistryPath" + } + } + ] + }, + { + "ruleId": "cpp/drivers/driver-entry-save-buffer", + "ruleIndex": 0, + "rule": { + "id": "cpp/drivers/driver-entry-save-buffer", + "index": 0 + }, + "message": { + "text": "The DriverEntry routine should save a copy of the argument [RegistryPath](1), not the pointer, because the I/O Manager frees the buffer" + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "driver/driver_snippet.c", + "uriBaseId": "%SRCROOT%", + "index": 0 + }, + "region": { + "startLine": 20, + "startColumn": 13, + "endColumn": 25 + } + } + } + ], + "partialFingerprints": { + "primaryLocationLineHash": "ea30678974f0f370:1", + "primaryLocationStartColumnFingerprint": "8" + }, + "relatedLocations": [ + { + "id": 1, + "physicalLocation": { + "artifactLocation": { + "uri": "driver/driver_snippet.c", + "uriBaseId": "%SRCROOT%", + "index": 0 + }, + "region": { + "startLine": 20, + "startColumn": 13, + "endColumn": 25 + } + }, + "message": { + "text": "RegistryPath" + } + } + ] + } + ], + "columnKind": "utf16CodeUnits", + "properties": { + "semmle.formatSpecifier": "sarifv2.1.0" + } + } + ] +} \ No newline at end of file diff --git a/src/drivers/general/queries/DriverEntrySaveBuffer/driver_snippet.c b/src/drivers/general/queries/DriverEntrySaveBuffer/driver_snippet.c new file mode 100644 index 00000000..fef0c8ea --- /dev/null +++ b/src/drivers/general/queries/DriverEntrySaveBuffer/driver_snippet.c @@ -0,0 +1,66 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// +// driver_snippet.c +// +#include "ntstrsafe.h" + +#define SET_DISPATCH 1 +// Template. Not called in this test. +void top_level_call() {} + +PUNICODE_STRING g_RP1; + +NTSTATUS +DriverEntryBad( + PDRIVER_OBJECT DriverObject, + PUNICODE_STRING RegistryPath + ) +{ + g_RP1 = RegistryPath; + return 0; +} + + +UNICODE_STRING g_RP2; + +NTSTATUS +DriverEntryGood( + PDRIVER_OBJECT DriverObject, + PUNICODE_STRING RegistryPath + ) +{ + return RtlUnicodeStringCopy(&g_RP2,RegistryPath); +} + + +UNICODE_STRING g_RP3; + +NTSTATUS +DriverEntryGood2( + PDRIVER_OBJECT DriverObject, + PUNICODE_STRING RegistryPath + ) +{ + g_RP3 = *RegistryPath; + return 0; +} + +typedef struct _test_struct { + int a; + PUNICODE_STRING g_RP4; + char b; +} test_struct; + +test_struct g_test_struct; + +NTSTATUS +DriverEntryBad2( + PDRIVER_OBJECT DriverObject, + PUNICODE_STRING RegistryPath + ) +{ + test_struct* localPtr = &g_test_struct; + localPtr->g_RP4 = RegistryPath; + return 0; +} \ No newline at end of file diff --git a/src/drivers/test/diff/DriverEntrySaveBuffer.sarif b/src/drivers/test/diff/DriverEntrySaveBuffer.sarif new file mode 100644 index 00000000..dea8275e --- /dev/null +++ b/src/drivers/test/diff/DriverEntrySaveBuffer.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 diff --git a/src/windows-driver-suites/ported_driver_ca_checks.qls b/src/windows-driver-suites/ported_driver_ca_checks.qls index 19642b02..b50f59c0 100644 --- a/src/windows-driver-suites/ported_driver_ca_checks.qls +++ b/src/windows-driver-suites/ported_driver_ca_checks.qls @@ -19,6 +19,7 @@ - drivers/general/queries/IrqlTooLow/IrqlTooLow.ql - drivers/general/queries/IrqlSetTooHigh/IrqlSetTooHigh.ql - drivers/general/queries/IrqlSetTooLow/IrqlSetTooLow.ql + - drivers/general/queries/DriverEntrySaveBuffer/DriverEntrySaveBuffer.ql - drivers/general/queries/CurrentFunctionTypeNotCorrect/CurrentFunctionTypeNotCorrect.ql - drivers/general/queries/IoInitializeTimerCall/IoInitializeTimerCall.ql - drivers/general/queries/OperandAssignment/OperandAssignment.ql