From d71fb9398750bd323f15787ff06a3d275d0bc0fc Mon Sep 17 00:00:00 2001 From: Jacob Ronstadt <147542405+jacob-ronstadt@users.noreply.github.com> Date: Fri, 23 Aug 2024 10:20:21 -0700 Subject: [PATCH] NtstatusExplicitCast2: CodeQL port of C29715 (#150) * port of C29715 * add other bool types --- .../NtstatusExplicitCast2.qhelp | 55 ++++ .../NtstatusExplicitCast2.ql | 30 ++ .../NtstatusExplicitCast2.sarif | 273 ++++++++++++++++++ .../NtstatusExplicitCast2/driver_snippet.c | 38 +++ .../test/diff/NtstatusExplicitCast2.sarif | 21 ++ 5 files changed, 417 insertions(+) create mode 100644 src/drivers/general/queries/NtstatusExplicitCast2/NtstatusExplicitCast2.qhelp create mode 100644 src/drivers/general/queries/NtstatusExplicitCast2/NtstatusExplicitCast2.ql create mode 100644 src/drivers/general/queries/NtstatusExplicitCast2/NtstatusExplicitCast2.sarif create mode 100644 src/drivers/general/queries/NtstatusExplicitCast2/driver_snippet.c create mode 100644 src/drivers/test/diff/NtstatusExplicitCast2.sarif diff --git a/src/drivers/general/queries/NtstatusExplicitCast2/NtstatusExplicitCast2.qhelp b/src/drivers/general/queries/NtstatusExplicitCast2/NtstatusExplicitCast2.qhelp new file mode 100644 index 00000000..fe033d83 --- /dev/null +++ b/src/drivers/general/queries/NtstatusExplicitCast2/NtstatusExplicitCast2.qhelp @@ -0,0 +1,55 @@ + + + +

+ Cast between semantically different integer types. This warning indicates that a Boolean is being cast to NTSTATUS. This is likely to give undesirable results. For example, the typical failure value for functions that return a Boolean (FALSE) is a success status when tested as an NTSTATUS. +

+ + +

+ Typically, a function that returns Boolean returns either 1 (for TRUE) or 0 (for FALSE). Both these values are treated as success codes by the NT_SUCCESS macro. Thus, the failure case will never be detected. +

+ + + +

+ Bad cast from Boolean to NTSTATUS +

+ + +

+ Correct use of Boolean +

+ + + + +

+

+ + +
  • + + Warning C28715 + +
    • +     +     diff --git a/src/drivers/general/queries/NtstatusExplicitCast2/NtstatusExplicitCast2.ql b/src/drivers/general/queries/NtstatusExplicitCast2/NtstatusExplicitCast2.ql new file mode 100644 index 00000000..a7a12485 --- /dev/null +++ b/src/drivers/general/queries/NtstatusExplicitCast2/NtstatusExplicitCast2.ql @@ -0,0 +1,30 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** + * @id cpp/drivers/ntstatus-explicit-cast2 + * @kind problem + * @name Ntstatus Explicit Cast 2 + * @description Cast between semantically different integer types (Boolean to NTSTATUS). + * @platform Desktop + * @feature.area Multiple + * @impact Insecure Coding Practice + * @repro.text This warning indicates that a Boolean is being cast to NTSTATUS. This is likely to give undesirable results. For example, the typical failure value for functions that return a Boolean (FALSE) is a success status when tested as an NTSTATUS. + * @opaqueid CQLD-C28715 + * @problem.severity warning + * @precision medium + * @tags correctness + * @scope domainspecific + * @query-version v1 + */ + +import cpp + +from Conversion c +where + ( + c.getType().toString().toLowerCase().matches("boolean") or + c.getType().toString().toLowerCase().matches("bool") or + c.getType().toString().matches("VARIANT_BOOL") + ) and + c.getConversion().getType().toString().matches("NTSTATUS") +select c, "Cast between semantically different integer types: Boolean to NTSTATUS" diff --git a/src/drivers/general/queries/NtstatusExplicitCast2/NtstatusExplicitCast2.sarif b/src/drivers/general/queries/NtstatusExplicitCast2/NtstatusExplicitCast2.sarif new file mode 100644 index 00000000..6705cc6b --- /dev/null +++ b/src/drivers/general/queries/NtstatusExplicitCast2/NtstatusExplicitCast2.sarif @@ -0,0 +1,273 @@ +{ + "$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/ntstatus-explicit-cast2", + "name": "cpp/drivers/ntstatus-explicit-cast2", + "shortDescription": { + "text": "Ntstatus Explicit Cast 2" + }, + "fullDescription": { + "text": "Cast between semantically different integer types (Boolean to NTSTATUS)." + }, + "defaultConfiguration": { + "enabled": true, + "level": "warning" + }, + "properties": { + "tags": [ + "correctness" + ], + "description": "Cast between semantically different integer types (Boolean to NTSTATUS).", + "feature.area": "Multiple", + "id": "cpp/drivers/ntstatus-explicit-cast2", + "impact": "Insecure Coding Practice", + "kind": "problem", + "name": "Ntstatus Explicit Cast 2", + "opaqueid": "CQLD-C28715", + "platform": "Desktop", + "precision": "medium", + "problem.severity": "warning", + "query-version": "v1", + "repro.text": "This warning indicates that a Boolean is being cast to NTSTATUS. This is likely to give undesirable results. For example, the typical failure value for functions that return a Boolean (FALSE) is a success status when tested as an NTSTATUS.", + "scope": "domainspecific" + } + } + ] + }, + "extensions": [ + { + "name": "microsoft/windows-drivers", + "semanticVersion": "1.1.0+ce7d70c32c8e0908d7c329389aa84ac3a89e7feb", + "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-21T06:06:26.075+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/ntstatus-explicit-cast2", + "ruleIndex": 0, + "rule": { + "id": "cpp/drivers/ntstatus-explicit-cast2", + "index": 0 + }, + "message": { + "text": "Cast between semantically different integer types: Boolean to NTSTATUS" + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "driver/driver_snippet.c", + "uriBaseId": "%SRCROOT%", + "index": 0 + }, + "region": { + "startLine": 30, + "startColumn": 9, + "endColumn": 35 + } + } + } + ], + "partialFingerprints": { + "primaryLocationLineHash": "bbf24f16ac513f29:1", + "primaryLocationStartColumnFingerprint": "4" + } + } + ], + "columnKind": "utf16CodeUnits", + "properties": { + "semmle.formatSpecifier": "sarifv2.1.0" + } + } + ] +} \ No newline at end of file diff --git a/src/drivers/general/queries/NtstatusExplicitCast2/driver_snippet.c b/src/drivers/general/queries/NtstatusExplicitCast2/driver_snippet.c new file mode 100644 index 00000000..9dce2224 --- /dev/null +++ b/src/drivers/general/queries/NtstatusExplicitCast2/driver_snippet.c @@ -0,0 +1,38 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +// Macros to enable or disable a code section that may or may not conflict with this test. +#define SET_DISPATCH 1 + +// Template function. Not used for this test. +void top_level_call() +{ +} + +BOOLEAN SomeFunction() +{ + return TRUE; +} +void test_good1() +{ + if (SomeFunction() == TRUE) + { + return 0; + } + else + { + return -1; + } +} + +void test_bad1() +{ + if (NT_SUCCESS(SomeFunction())) + { + return 0; + } + else + { + return -1; + } +} diff --git a/src/drivers/test/diff/NtstatusExplicitCast2.sarif b/src/drivers/test/diff/NtstatusExplicitCast2.sarif new file mode 100644 index 00000000..dea8275e --- /dev/null +++ b/src/drivers/test/diff/NtstatusExplicitCast2.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