Skip to content

Commit

Permalink
IRPStackEntryCopy: CodeQL port of c28114 (#120)
Browse files Browse the repository at this point in the history
* port of C28114, Copying a whole IRP stack entry

* update unit tests

* fix ql name

* update id and move out of experimental

* add query to ported ca checks suite

* fix sarif file

---------

Signed-off-by: Jacob Ronstadt <147542405+jacob-ronstadt@users.noreply.github.com>
  • Loading branch information
jacob-ronstadt authored Aug 23, 2024
1 parent 53bb6fe commit 0488826
Show file tree
Hide file tree
Showing 6 changed files with 511 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
<qhelp>
<overview>
<p>
Copying a whole IRP stack entry leaves certain fields initialized that should be cleared or updated
</p>
</overview>
<recommendation>
<p>
The driver is copying an IRP improperly. Improperly copying an IRP can cause serious problems with a driver, including loss of data and system crashes. If an IRP must be copied and IoCopyCurrentIrpStackLocationToNext does not suffice, then certain members of the IRP should not be copied or should be zeroed after copying.
</p>
</recommendation>
<example>
<sample src="driver_snippet.c" />
</example>
<references>
<li>
<a href="https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28114-improper-irp-stack-copy">
C28114
</a>
</li>
</references>
</qhelp>
48 changes: 48 additions & 0 deletions src/drivers/general/queries/IRPStackEntryCopy/IRPStackEntryCopy.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
/**
* @id cpp/drivers/irp-stack-entry-copy
* @name Irp stack entry copy
* @description C28114: Copying a whole IRP stack entry leaves certain fields initialized that should be cleared or updated.
* @platform Desktop
* @security.severity Medium
* @feature.area Multiple
* @impact Exploitable Design
* @repro.text
* @owner.email sdat@microsoft.com
* @opaqueid CQLD-C28114
* @kind problem
* @problem.severity warning
* @precision medium
* @tags correctness
* wddst
* @scope domainspecific
* @query-version v1
*/

import cpp

class PIO_STACK_LOCATION extends TypedefType {
PIO_STACK_LOCATION() {
this instanceof TypedefType and
this.getName() = "PIO_STACK_LOCATION"
}
}

from FunctionCall mem_copy_func, Expr io_stack_param, string size
where
(
mem_copy_func.getTarget().getName() = "RtlMoveMemory" or
mem_copy_func.getTarget().getName() = "RtlCopyMemory" or
mem_copy_func.getTarget().getName() = "RtlCopyBytes" or
mem_copy_func.getTarget().getName() = "RtlCopyMemory32" or
mem_copy_func.getTarget().getName() = "memmove" or
mem_copy_func.getTarget().getName() = "memcpy"
) and
// All of these memory copy functions have source as the second parameter and size as the third parameter
io_stack_param = mem_copy_func.getArgument(1) and
io_stack_param.getType() instanceof PIO_STACK_LOCATION and
size = mem_copy_func.getArgument(2).getValue() and
size = "36" // Size of the IRP stack entry in bytes in decimal form. (0x24 hex)
select mem_copy_func, "Possible copy of a whole IRP stack entry $@ at $@", io_stack_param,
io_stack_param.toString(), mem_copy_func, mem_copy_func.toString()
Loading

0 comments on commit 0488826

Please sign in to comment.