diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b64106f..a6345249 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,19 @@ # Change Log All notable changes to this project will be documented in this file. +## [1.9.0] - 2026-02-27 + +### Added + - Added five new queries in the Microsoft subfolder. These queries are now part of our recommended and must-run sets. + - ConditionallyUninitializedVariableAutomation.ql: Flags calls to initialization functions whose return status is not checked, potentially leaving a local variable uninitialized. + - UnprobedDereference.ql: Detects dereferences of user-provided pointers that haven't been probed first, which could cause access violations. + - UserModeMemoryOutsideTry.ql: Finds reads of user-mode memory that occur outside a try/catch block, where unexpected exceptions from changed memory protections could crash the kernel. + - UserModeMemoryReadMultipleTimes.ql: identifies double-fetch vulnerabilities where user-mode memory is read more than once without being copied to kernel memory first. + - UnguardedNullReturnDereference.ql: Reports dereferences of return values from calls that may return NULL (e.g. heap allocations) without a preceding null check. + +### Changed + - Standardized the rule ID of UninitializedPtrField.ql to "cpp/microsoft/public/likely-bugs/uninitializedptrfield" and updated accuracy. + ## [1.8.3] - 2026-02-25 ### Changed diff --git a/src/microsoft/Likely Bugs/Memory Management/ConditionallyUninitializedVariable.md b/src/microsoft/Likely Bugs/Memory Management/ConditionallyUninitializedVariable.md new file mode 100644 index 00000000..9b907df2 --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/ConditionallyUninitializedVariable.md @@ -0,0 +1,80 @@ +# Conditionally uninitialized variable +A common pattern is to initialize a local variable by calling another function (an "initialization" function) with the address of the local variable as a pointer argument. That function is then responsible for writing to the memory location referenced by the pointer. + +In some cases, the called function may not always write to the memory pointed to by the pointer argument. In such cases, the function will typically return a "status" code, informing the caller as to whether the initialization succeeded or not. If the caller does not check the status code before reading the local variable, it may read uninitialized memory, which can result in unexpected behavior. + + +## Recommendation +When using an initialization function that does not guarantee to initialize the memory pointed to by the passed pointer, and returns a status code to indicate whether such initialization occurred, the status code should be checked before reading from the local variable. + + +## Example +In this hypothetical example we have code for managing a series of devices. The code includes a `DeviceConfig` struct that can represent properties about each device. The `initDeviceConfig` function can be called to initialize one of these structures, by providing a "device number", which can be used to look up the appropriate properties in some data store. If an invalid device number is provided, the function returns a status code of `-1`, and does not initialize the provided pointer. + +In the first code sample below, the `notify` function calls the `initDeviceConfig` function with a pointer to the local variable `config`, which is then subsequently accessed to fetch properties of the device. However, the code does not check the return value from the function call to `initDeviceConfig`. If the device number passed to the `notify` function was invalid, the `initDeviceConfig` function will leave the `config` variable uninitialized, which will result in the `notify` function accessing uninitialized memory. + + +```c +struct DeviceConfig { + bool isEnabled; + int channel; +}; + +int initDeviceConfig(DeviceConfig *ref, int deviceNumber) { + if (deviceNumber >= getMaxDevices()) { + // No device with that number, return -1 to indicate failure + return -1; + } + // Device with that number, fetch parameters and initialize struct + ref->isEnabled = fetchIsDeviceEnabled(deviceNumber); + ref->channel = fetchDeviceChannel(deviceNumber); + // Return 0 to indicate success + return 0; +} + +int notify(int deviceNumber) { + DeviceConfig config; + initDeviceConfig(&config, deviceNumber); + // BAD: Using config without checking the status code that is returned + if (config.isEnabled) { + notifyChannel(config.channel); + } +} + +``` +To fix this, the code needs to check that the return value of the call to `initDeviceConfig` is zero. If that is true, then the calling code can safely assume that the local variable has been initialized. + + +```c +struct DeviceConfig { + bool isEnabled; + int channel; +}; + +int initDeviceConfig(DeviceConfig *ref, int deviceNumber) { + if (deviceNumber >= getMaxDevices()) { + // No device with that number, return -1 to indicate failure + return -1; + } + // Device with that number, fetch parameters and initialize struct + ref->isEnabled = fetchIsDeviceEnabled(deviceNumber); + ref->channel = fetchDeviceChannel(deviceNumber); + // Return 0 to indicate success + return 0; +} + +void notify(int deviceNumber) { + DeviceConfig config; + int statusCode = initDeviceConfig(&config, deviceNumber); + if (statusCode == 0) { + // GOOD: Status code returned by initialization function is checked, so this is safe + if (config.isEnabled) { + notifyChannel(config.channel); + } + } +} + +``` + +## References +* Wikipedia: [Uninitialized variable](https://en.wikipedia.org/wiki/Uninitialized_variable). diff --git a/src/microsoft/Likely Bugs/Memory Management/ConditionallyUninitializedVariable.qhelp b/src/microsoft/Likely Bugs/Memory Management/ConditionallyUninitializedVariable.qhelp new file mode 100644 index 00000000..73d07304 --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/ConditionallyUninitializedVariable.qhelp @@ -0,0 +1,59 @@ + + + + +

A common pattern is to initialize a local variable by calling another function (an +"initialization" function) with the address of the local variable as a pointer argument. That +function is then responsible for writing to the memory location referenced by the pointer.

+ +

In some cases, the called function may not always write to the memory pointed to by the +pointer argument. In such cases, the function will typically return a "status" code, informing the +caller as to whether the initialization succeeded or not. If the caller does not check the status +code before reading the local variable, it may read uninitialized memory, which can result in +unexpected behavior.

+ + + + +

When using an initialization function that does not guarantee to initialize the memory pointed to +by the passed pointer, and returns a status code to indicate whether such initialization occurred, +the status code should be checked before reading from the local variable.

+ + + + +

In this hypothetical example we have code for managing a series of devices. The code +includes a DeviceConfig struct that can represent properties about each device. +The initDeviceConfig function can be called to initialize one of these structures, by +providing a "device number", which can be used to look up the appropriate properties in some data +store. If an invalid device number is provided, the function returns a status code of +-1, and does not initialize the provided pointer.

+ +

In the first code sample below, the notify function calls the +initDeviceConfig function with a pointer to the local variable config, +which is then subsequently accessed to fetch properties of the device. However, the code does not +check the return value from the function call to initDeviceConfig. If the +device number passed to the notify function was invalid, the +initDeviceConfig function will leave the config variable uninitialized, +which will result in the notify function accessing uninitialized memory.

+ + + +

To fix this, the code needs to check that the return value of the call to +initDeviceConfig is zero. If that is true, then the calling code can safely assume +that the local variable has been initialized.

+ + + + + + +
  • + Wikipedia: + Uninitialized variable. +
    • + +     +     diff --git a/src/microsoft/Likely Bugs/Memory Management/ConditionallyUninitializedVariable.ql b/src/microsoft/Likely Bugs/Memory Management/ConditionallyUninitializedVariable.ql new file mode 100644 index 00000000..16bf4aff --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/ConditionallyUninitializedVariable.ql @@ -0,0 +1,30 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** +* @id cpp/microsoft/public/likely-bugs/memory-management/v2/conditionally-uninitialized-variable +* @name Conditionally uninitialized variable +* @description When an initialization function is used to initialize a local variable, +* but the returned status code is not checked, reading the variable may +* result in undefined behaviour. +* @platform Desktop +* @security.severity Low +* @impact Insecure Coding Practice +* @feature.area Multiple +* @repro.text The following code locations potentially contain uninitialized variables +* @owner.email sdat@microsoft.com +* @kind problem +* @problem.severity error +* @query-version 2.0 +**/ + +import cpp +private import UninitializedVariables + +from ConditionallyInitializedVariable v, ConditionalInitializationFunction f, ConditionalInitializationCall call, Evidence e +where exists(v.getARiskyAccess(f, call, e)) + and e = DefinitionInSnapshot() +select call, "$@: The status of this call to $@ is not checked, potentially leaving $@ uninitialized.", + call.getEnclosingFunction(), + call.getEnclosingFunction().toString(), + f, f.getName(), + v, v.getName() diff --git a/src/microsoft/Likely Bugs/Memory Management/ConditionallyUninitializedVariableBad.c b/src/microsoft/Likely Bugs/Memory Management/ConditionallyUninitializedVariableBad.c new file mode 100644 index 00000000..73a01c2d --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/ConditionallyUninitializedVariableBad.c @@ -0,0 +1,25 @@ +struct DeviceConfig { + bool isEnabled; + int channel; +}; + +int initDeviceConfig(DeviceConfig *ref, int deviceNumber) { + if (deviceNumber >= getMaxDevices()) { + // No device with that number, return -1 to indicate failure + return -1; + } + // Device with that number, fetch parameters and initialize struct + ref->isEnabled = fetchIsDeviceEnabled(deviceNumber); + ref->channel = fetchDeviceChannel(deviceNumber); + // Return 0 to indicate success + return 0; +} + +int notify(int deviceNumber) { + DeviceConfig config; + initDeviceConfig(&config, deviceNumber); + // BAD: Using config without checking the status code that is returned + if (config.isEnabled) { + notifyChannel(config.channel); + } +} diff --git a/src/microsoft/Likely Bugs/Memory Management/ConditionallyUninitializedVariableGood.c b/src/microsoft/Likely Bugs/Memory Management/ConditionallyUninitializedVariableGood.c new file mode 100644 index 00000000..ced43a66 --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/ConditionallyUninitializedVariableGood.c @@ -0,0 +1,27 @@ +struct DeviceConfig { + bool isEnabled; + int channel; +}; + +int initDeviceConfig(DeviceConfig *ref, int deviceNumber) { + if (deviceNumber >= getMaxDevices()) { + // No device with that number, return -1 to indicate failure + return -1; + } + // Device with that number, fetch parameters and initialize struct + ref->isEnabled = fetchIsDeviceEnabled(deviceNumber); + ref->channel = fetchDeviceChannel(deviceNumber); + // Return 0 to indicate success + return 0; +} + +void notify(int deviceNumber) { + DeviceConfig config; + int statusCode = initDeviceConfig(&config, deviceNumber); + if (statusCode == 0) { + // GOOD: Status code returned by initialization function is checked, so this is safe + if (config.isEnabled) { + notifyChannel(config.channel); + } + } +} diff --git a/src/microsoft/Likely Bugs/Memory Management/InitializationFunctions.qll b/src/microsoft/Likely Bugs/Memory Management/InitializationFunctions.qll new file mode 100644 index 00000000..8578ae04 --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/InitializationFunctions.qll @@ -0,0 +1,651 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** + * Provides classes and predicates for identifying functions that initialize their arguments. + */ +import cpp +import external.ExternalArtifact +private import microsoft.code.cpp.public.dispatch.VirtualDispatch +import microsoft.code.cpp.public.NestedFields +import drivers.libraries.SAL +import semmle.code.cpp.controlflow.Guards + +/** A context under which a function may be called. */ +private newtype TContext = + /** No specific call context. */ + NoContext() or + /** + * The call context is that the given other parameter is null. + * + * This context is created for all parameters that are null checked in the body of the function. + */ + ParamNull(Parameter p) { p = any(ParameterNullCheck pnc).getParameter() } or + /** + * The call context is that the given other parameter is not null. + * + * This context is created for all parameters that are null checked in the body of the function. + */ + ParamNotNull(Parameter p) { p = any(ParameterNullCheck pnc).getParameter() } + +/** + * A context under which a function may be called. + * + * Some functions may conditionally initialize a parameter depending on the value of another + * parameter. Consider: + * ``` + * int MyInitFunction(int* paramToBeInitialized, int* paramToCheck) { + * if (!paramToCheck) { + * // fail! + * return -1; + * } + * paramToBeInitialized = 0; + * } + * ``` + * In this case, whether `paramToBeInitialized` is initialized when this function call completes + * depends on whether `paramToCheck` is or is not null. A call-context insensitive analysis will + * determine that any call to this function may leave the parameter uninitialized, even if the + * argument to paramToCheck is guaranteed to be non-null (`&foo`, for example). + * + * This class models call contexts that can be considered when calculating whether a given parameter + * initializes or not. The supported contexts are: + * - `ParamNull(otherParam)` - the given `otherParam` is considered to be null. Applies when + * exactly one parameter other than this one is null checked. + * - `ParamNotNull(otherParam)` - the given `otherParam` is considered to be not null. Applies when + * exactly one parameter other than this one is null checked. + * - `NoContext()` - applies in all other circumstances. + */ +class Context extends TContext { + string toString() { + this = NoContext() and result = "NoContext" or + this = ParamNull(any(Parameter p | result = "ParamNull(" + p.getName() + ")")) or + this = ParamNotNull(any(Parameter p | result = "ParamNotNull(" + p.getName() + ")")) + } +} + +/** + * A check against a parameter. + */ +abstract class ParameterCheck extends Expr { + /** + * Gets a successor of this check that should be ignored for the given context. + */ + abstract ControlFlowNode getIgnoredSuccessorForContext(Context c); +} + +/** A null-check expression on a parameter. */ +class ParameterNullCheck extends ParameterCheck { + Parameter p; + ControlFlowNode nullSuccessor; + ControlFlowNode notNullSuccessor; + ParameterNullCheck() { + this.isCondition() and + p.getFunction() instanceof InitializationFunction and + p.getType().getUnspecifiedType() instanceof PointerType and + exists(VariableAccess va | + va = p.getAnAccess() | + ( + nullSuccessor = getATrueSuccessor() and notNullSuccessor = getAFalseSuccessor() and + ( + va = this.(NotExpr).getOperand() or + va = any(EQExpr eq | eq = this and eq.getAnOperand().getValue() = "0").getAnOperand() or + va = getAssertedFalseCondition(this) or + va = any(NEExpr eq | eq = getAssertedFalseCondition(this) and eq.getAnOperand().getValue() = "0").getAnOperand() + ) + ) or + ( + nullSuccessor = getAFalseSuccessor() and notNullSuccessor = getATrueSuccessor() and + ( + va = this or + va = any(NEExpr eq | eq = this and eq.getAnOperand().getValue() = "0").getAnOperand() or + va = any(EQExpr eq | eq = getAssertedFalseCondition(this) and eq.getAnOperand().getValue() = "0").getAnOperand() + ) + ) + ) + } + + /** The parameter being null-checked. */ + Parameter getParameter() { + result = p + } + + override ControlFlowNode getIgnoredSuccessorForContext(Context c) { + c = ParamNull(p) and result = notNullSuccessor or + c = ParamNotNull(p) and result = nullSuccessor + } + + /** The successor at which the parameter is confirmed to be null. */ + ControlFlowNode getNullSuccessor() { + result = nullSuccessor + } + + /** The successor at which the parameter is confirmed to be not-null. */ + ControlFlowNode getNotNullSuccessor() { + result = notNullSuccessor + } +} + +/** + * An entry in a CSV file in cond-init that contains externally defined functions that are + * conditional initializers. These files are typically produced by running the + * ConditionallyInitializedFunction companion query. + */ +class ValidatedExternalCondInitFunction extends ExternalData { + ValidatedExternalCondInitFunction() { this.getDataPath().matches("%cond-init%.csv") } + + predicate isExternallyVerified(Function f, int param) { + functionSignature(f, getField(1), getField(2)) and param = getFieldAsInt(3) + } +} + +/** + * The type of evidence used to determine whether a function initializes a parameter. + */ +newtype Evidence = + /** + * The function is defined in the snapshot, and the CFG has been analyzed to determine that the + * parameter is not initialized on at least one path to the exit. + */ + DefinitionInSnapshot() or + /** + * The function is externally defined, but the parameter has an `_out` SAL annotation which + * suggests that it is initialized in the function. + */ + SuggestiveSALAnnotation() or + /** + * We have been given a CSV file which indicates this parameter is conditionally initialized. + */ + ExternalEvidence() + +/** + * A call to an function which initializes one or more of its parameters. + */ +class InitializationFunctionCall extends FunctionCall { + Expr initializedArgument; + + InitializationFunctionCall() { + initializedArgument = getAnInitializedArgument(this) + } + + /** Gets a parameter that is initialized by this call. */ + Parameter getAnInitParameter() { + result.getAnAccess() = initializedArgument + } +} + + +/** + * A variable access which is dereferenced then assigned to. + */ +private predicate isPointerDereferenceAssignmentTarget(VariableAccess target) { + target.getParent().(PointerDereferenceExpr) = any(Assignment e).getLValue() +} + +/** + * A function which initializes one or more of its parameters. + */ +class InitializationFunction extends Function { + int i; + Evidence evidence; + + InitializationFunction() { + ( + evidence = DefinitionInSnapshot() and + ( + // Assignment by pointer dereferencing the parameter + isPointerDereferenceAssignmentTarget(this.getParameter(i).getAnAccess()) or + // Field wise assignment to the parameter + any(Assignment e).getLValue() = getAFieldAccess(this.getParameter(i)) or + i = this.(MemberFunction).getAnOverridingFunction+().(InitializationFunction).initializedParameter() or + getParameter(i) = any(InitializationFunctionCall c).getAnInitParameter() + ) + ) or + // If we have no definition, we look at SAL annotations + (not this.hasDefinition() and this.getParameter(i).(SALParameter).isOut() and evidence = SuggestiveSALAnnotation()) or + // We have some external information that this function conditionally initializes + (not this.hasDefinition() and any(ValidatedExternalCondInitFunction vc).isExternallyVerified(this, i) and evidence = ExternalEvidence()) + } + + /** Gets a parameter index which is initialized by this function. */ + int initializedParameter() { result = i } + + /** Gets a `ControlFlowNode` which assigns a new value to the parameter with the given index. */ + ControlFlowNode paramReassignment(int index) { + index = i and + ( + result = this.getParameter(i).getAnAccess() and + ( + result = any(Assignment a).getLValue().(PointerDereferenceExpr).getOperand() or + // Field wise assignment to the parameter + result = any(Assignment a).getLValue().(FieldAccess).getQualifier() or + // Assignment to a nested field of the parameter + result = any(Assignment a).getLValue().(NestedFieldAccess).getUltimateQualifier() or + result = getAnInitializedArgument(any(Call c)) or + exists(IfStmt check | result = check.getCondition().getAChild*() | + paramReassignmentCondition(check) + ) + ) or + result = any(AssumeExpr e | e.getEnclosingFunction() = this and e.getAChild().(Literal).getValue() = "0") + ) + } + + /** + * Helper predicate: holds if the `if` statement `check` contains a + * reassignment to the `i`th parameter within its `then` statement. + */ + pragma[noinline] + private predicate paramReassignmentCondition(IfStmt check) { + this.paramReassignment(i).getEnclosingStmt().getParentStmt*() = check.getThen() + } + + /** Holds if `n` can be reached without the parameter at `index` being reassigned. */ + predicate paramNotReassignedAt(ControlFlowNode n, int index, Context c) { + c = getAContext(index) and + ( + not exists(this.getEntryPoint()) and index = i and n = this or + n = this.getEntryPoint() and index = i or + exists(ControlFlowNode mid | paramNotReassignedAt(mid, index, c) | + n = mid.getASuccessor() and + not n = paramReassignment(index) and + /* + * Ignore successor edges where the parameter is null, because it is then confirmed to be + * initialized. + */ + not exists(ParameterNullCheck nullCheck | + nullCheck = mid and + nullCheck = getANullCheck(index) and + n = nullCheck.getNullSuccessor() + ) and + /* + * Ignore successor edges which are excluded by the given context + */ + not exists(ParameterCheck paramCheck | + paramCheck = mid | + n = paramCheck.getIgnoredSuccessorForContext(c) + ) + ) + ) + } + + /** Gets a null-check on the parameter at `index`. */ + private ParameterNullCheck getANullCheck(int index) { + getParameter(index) = result.getParameter() + } + + /** Gets a parameter which is not at the given index. */ + private Parameter getOtherParameter(int index) { + index = i and + result = getAParameter() and + not result.getIndex() = index + } + + /** + * Gets a call `Context` that is applicable when considering whether parameter at the `index` can + * be conditionally initialized. + */ + Context getAContext(int index) { + index = i and + /* + * If there is one and only one other parameter which is null checked in the body of the method, + * then we have two contexts to consider - that the other param is null, or that the other param + * is not null. + */ + if (strictcount(Parameter p | exists(Context c | c = ParamNull(p) or c = ParamNotNull(p)) and p = getOtherParameter(index)) = 1) then + exists(Parameter p | + p = getOtherParameter(index) | + result = ParamNull(p) or result = ParamNotNull(p) + ) + else + // Otherwise, only consider NoContext. + result = NoContext() + } + + /** + * Holds if this function should be whitelisted - that is, not considered as conditionally + * initializing its parameters. + */ + predicate whitelisted() { + exists(string name | this.hasName(name) | + // Return value is not a success code but the output functions never fail. + name.matches("_Interlocked%") or + // Functions that never fail, according to MSDN. + name = "QueryPerformanceCounter" or + name = "QueryPerformanceFrequency" or + // Functions that never fail post-Vista, according to MSDN. + name = "InitializeCriticalSectionAndSpinCount" or + // `rand_s` writes 0 to a non-null argument if it fails, according to MSDN. + name = "rand_s" or + // IntersectRect initializes the argument regardless of whether the input intersects + name = "IntersectRect" or + name = "SetRect" or + name = "UnionRect" or + // These functions appears to have an incorrect CFG, which leads to false positives + name = "PhysicalToLogicalDPIPoint" or + name = "LogicalToPhysicalDPIPoint" or + // Sets NtProductType to default on error + name = "RtlGetNtProductType" or + // Our CFG is not sophisticated enough to detect that the argument is always initialized + name = "StringCchLengthA" or + // All paths init the argument, and always returns SUCCESS. + name = "RtlUnicodeToMultiByteSize" or + // All paths init the argument, and always returns SUCCESS. + name = "RtlMultiByteToUnicodeSize" or + // All paths init the argument, and always returns SUCCESS. + name = "RtlUnicodeToMultiByteN" or + // Always initializes argument + name = "RtlGetFirstRange" or + // Destination range is zeroed out on failure, assuming first two parameters are valid + name = "memcpy_s" or + // This zeroes the memory unconditionally + name = "SeCreateAccessState" + ) + } +} + +/** + * A function which initializes one or more of its parameters, but not on all paths. + */ +class ConditionalInitializationFunction extends InitializationFunction { + Context c; + ConditionalInitializationFunction() { + c = this.getAContext(i) and + not this.whitelisted() and + exists(Type status | status = this.getType().getUnspecifiedType() | + status instanceof IntegralType or + status instanceof Enum + ) and + not this.getType().getName().toLowerCase() = "size_t" and + ( + /* + * If there is no definition, consider this to be conditionally initializing (based on either + * SAL or external data). + */ + not evidence = DefinitionInSnapshot() or + /* + * If this function is defined in this snapshot, then it conditionally initializes if there + * is at least one path through the function which doesn't initialize the parameter. + * + * Explicitly ignore pure virtual functions. + */ + this.hasDefinition() and this.paramNotReassignedAt(this, i, c) and not this instanceof PureVirtualFunction + ) + } + + /** Gets the evidence associated with the given parameter. */ + Evidence getEvidence(int param) { + /* + * Note: due to the way the predicate dispatch interacts with fields, this needs to be + * implemented on this class, not `InitializationFunction`. If implemented on the latter it + * can return evidence that does not result in conditional initialization. + */ + param = i and evidence = result + } + + /** Gets the index of a parameter which is conditionally initialized. */ + int conditionallyInitializedParameter(Context context) { result = i and context = c } +} + +/** + * More elaborate tracking, flagging cases where the status is checked after + * the potentially uninitialized variable has been used, and ignoring cases + * where the status is not checked but there is no use of the potentially + * uninitialized variable, may be obtained via `getARiskyAccess`. + */ +class ConditionalInitializationCall extends FunctionCall { + ConditionalInitializationFunction target; + ConditionalInitializationCall() { target = getTarget(this) } + + /** Gets the argument passed for the given parameter to this call. */ + Expr getArgumentForParameter(Parameter p) { + p = getTarget().getAParameter() and + result = getArgument(p.getIndex()) + } + + /** + * Gets an argument conditionally initialized by this call. + */ + Expr getAConditionallyInitializedArgument(ConditionalInitializationFunction condTarget, Evidence e) { + condTarget = target and + exists(Context context | + result = getAConditionallyInitializedArgument(this, condTarget, context, e) | + context = NoContext() or + exists(Parameter otherP, Expr otherArg | + context = ParamNotNull(otherP) or + context = ParamNull(otherP) | + otherArg = getArgumentForParameter(otherP) and + (otherArg instanceof AddressOfExpr implies context = ParamNotNull(otherP)) and + (otherArg.getType() instanceof ArrayType implies context = ParamNotNull(otherP)) and + (otherArg.getValue() = "0" implies context = ParamNull(otherP)) + ) + ) + } + + VariableAccess getAConditionallyInitializedVariable() { + not result.getTarget().getAnAssignedValue().getASuccessor+() = result and + // Should not be assigned field-wise prior to the call. + not exists(Assignment a, FieldAccess fa | + fa.getQualifier() = result.getTarget().getAnAccess() and + a.getLValue() = fa and + fa.getASuccessor+() = result + ) and + result = this.getArgument(getTarget(this).(ConditionalInitializationFunction).conditionallyInitializedParameter(_)).(AddressOfExpr).getOperand() + } + + Variable getStatusVariable() { + exists(AssignExpr a | a.getLValue() = result.getAnAccess() | + a.getRValue() = this + ) or + result.getInitializer().getExpr() = this + } + + Expr getSuccessCheck() { + exists(this.getAFalseSuccessor()) and result = this or + result = this.getParent() and ( + result instanceof NotExpr or + result.(EQExpr).getAnOperand().getValue() = "0" or + result.(GEExpr).getLesserOperand().getValue() = "0" + ) + } + + Expr getFailureCheck() { + result = this.getParent() and ( + result instanceof NotExpr or + result.(NEExpr).getAnOperand().getValue() = "0" or + result.(LTExpr).getLesserOperand().getValue() = "0" + ) + } + + private predicate inCheckedContext() { + exists(Call parent | this = parent.getAnArgument() | + parent.getTarget() instanceof Operator or + parent.getTarget().hasName("VerifyOkCatastrophic") + ) + } + + ControlFlowNode uncheckedReaches(LocalVariable var) { + ( + not exists(var.getInitializer()) and + var = this.getAConditionallyInitializedVariable().getTarget() and + if exists(this.getFailureCheck()) then + result = this.getFailureCheck().getATrueSuccessor() + else if exists(this.getSuccessCheck()) then + result = this.getSuccessCheck().getAFalseSuccessor() + else + (result = this.getASuccessor() and not this.inCheckedContext()) + ) or + exists(ControlFlowNode mid | mid = uncheckedReaches(var) | + not mid = getStatusVariable().getAnAccess() and + not mid = var.getAnAccess() and + not exists(VariableAccess write | result = write and write = var.getAnAccess() | + write = any(AssignExpr a).getLValue() or + write = any(AddressOfExpr a).getOperand() + ) and + result = mid.getASuccessor() + ) + } + + VariableAccess getARiskyRead(Function f) { + f = this.getTarget() and + exists(this.getFile().getRelativePath()) and + result = this.uncheckedReaches(result.getTarget()) and + not this.(GuardCondition).controls(result.getBasicBlock(), _) + } +} + +/** + * Holds if the call should not be considered as initializing a parameter. + */ +predicate isWhitelistedCall(Call c) { + /* + * RtlUnicodeStringToInteger only leaves the first argument uninitialized if the last argument is + * null. If the last argument is computed using the "address of" operator, it cannot be null. + */ + c.getTarget().getName().matches("RtlUnicodeStringToInteger") and + c.getArgument(2) instanceof AddressOfExpr or + /* + * RtlInitUnicodeStringEx only fails if the length of the string passed as the initializer is + * greater than the max length. If the argument is a string literal, this is assumed not to be + * true. + */ + c.getTarget().getName().matches("RtlInitUnicodeStringEx") and + c.getArgument(1) instanceof StringLiteral +} + +/** + * Gets the position of an argument to the call which is initialized by the call. + */ +pragma[nomagic] +int initializedArgument(Call call) { + exists(InitializationFunction target | + target = getTarget(call) and + result = target.initializedParameter() + ) +} + +/** + * Gets an argument which is initialized by the call. + */ +Expr getAnInitializedArgument(Call call) { + result = call.getArgument(initializedArgument(call)) +} + +/** + * Gets the position of an argument to the call to the target which is conditionally initialized by + * the call, under the given context and evidence. + */ +pragma[nomagic] +int conditionallyInitializedArgument(Call call, ConditionalInitializationFunction target, Context c, Evidence e) { + target = getTarget(call) and + c = target.getAContext(result) and + e = target.getEvidence(result) and + result = target.conditionallyInitializedParameter(c) +} + +/** + * Gets an argument which is conditionally initialized by the call to the given target under the given context and evidence. + */ +Expr getAConditionallyInitializedArgument(Call call, ConditionalInitializationFunction target, Context c, Evidence e) { + result = call.getArgument(conditionallyInitializedArgument(call, target, c, e)) +} + +/** + * Gets the type signature for the functions parameters. + */ +string typeSig(Function f) { + result = concat(int i, Type pt | pt = f.getParameter(i).getType() | pt.getUnspecifiedType().toString(), "," order by i) +} + +/** + * Holds where qualifiedName and typeSig make up the signature for the function. + */ +predicate functionSignature(Function f, string qualifiedName, string typeSig) { + qualifiedName = f.getQualifiedName() and + typeSig = typeSig(f) +} + +/** + * Gets a possible definition for the Call where the target is a defined function. + */ +Function getADefinition(Call c) { + result = VirtualDispatch::getAViableTarget(c) and + result.hasDefinition() +} + +/** + * Gets a possible definition for the Call where the target is an undefined function by matching + * the undefined function name and parameter arity with a defined function. + * + * This is useful for identifying call to target dependencies across libraries, where the libraries + * are never statically linked together. + */ +Function getAPossibleDefinition(Call c) { + exists(Function undefinedFunction | undefinedFunction = VirtualDispatch::getAViableTarget(c) | + not undefinedFunction.hasDefinition() and + exists(string qn, string typeSig | functionSignature(undefinedFunction, qn, typeSig) and functionSignature(result, qn, typeSig)) and + result.hasDefinition() + ) +} + +/** + * Gets a possible target for the Call, using the name and parameter matching if we did not associate + * this call with a specific definition at link or compile time, and performing simple virtual + * dispatch resolution. + */ +Function getTarget(Call c) { + if exists(getADefinition(c)) then + /* + * If there is at least one defined target after performing some simple virtual dispatch + * resolution, then the result is all the defined targets. + */ + result = getADefinition(c) + else + if exists(getAPossibleDefinition(c)) then + /* + * If we can use the heuristic matching of functions to find definitions for some of the viable + * targets, return those. + */ + result = getAPossibleDefinition(c) + else + // Otherwise, the result is the undefined `Function` instances + result = VirtualDispatch::getAViableTarget(c) +} + + +/** + * Get an access of a field on `Variable` v. + */ +FieldAccess getAFieldAccess(Variable v) { + exists(VariableAccess va, Expr qualifierExpr | + // Find an access of the variable, or an AddressOfExpr that has the access + va = v.getAnAccess() and ( + qualifierExpr = va or + qualifierExpr.(AddressOfExpr).getOperand() = va + ) | + // Direct field access + qualifierExpr = result.getQualifier() or + // Nested field access + qualifierExpr = result.(NestedFieldAccess).getUltimateQualifier() + ) +} + +/** + * Gets a condition which is asserted to be false by the given `ne` expression, according to this pattern: + * ``` + * int a = !!result; + * if (!a) { // <- ne + * .... + * } + * ``` + */ +Expr getAssertedFalseCondition(NotExpr ne) { + exists(LocalVariable v | + result = v.getInitializer().getExpr().(NotExpr).getOperand().(NotExpr).getOperand() and + ne.getOperand() = v.getAnAccess() and + nonAssignedVariable(v) + // and not passed by val? + ) +} + +pragma[noinline] +private predicate nonAssignedVariable(Variable v) { + not exists(v.getAnAssignment()) +} diff --git a/src/microsoft/Likely Bugs/Memory Management/UninitializedVariables.qll b/src/microsoft/Likely Bugs/Memory Management/UninitializedVariables.qll new file mode 100644 index 00000000..a12c700f --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/UninitializedVariables.qll @@ -0,0 +1,163 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** + * A module for identifying conditionally initialized variables. + */ + +import cpp +import InitializationFunctions + +// Optimised reachability predicates + +private predicate reaches(ControlFlowNode a, ControlFlowNode b) = fastTC(successor/2)(a,b) + +private predicate successor(ControlFlowNode a, ControlFlowNode b) { + b = a.getASuccessor() +} + +private predicate hasConditionalInitialization(ConditionalInitializationFunction f, ConditionalInitializationCall call, LocalVariable v, VariableAccess initAccess, Evidence e) { + // Ignore whitelisted calls + not isWhitelistedCall(call) and + f = getTarget(call) and + initAccess = v.getAnAccess() and + initAccess = call.getAConditionallyInitializedArgument(f, e).(AddressOfExpr).getOperand() +} + +/** + * A variable that can be conditionally initialized by a call. + */ +class ConditionallyInitializedVariable extends LocalVariable { + ConditionalInitializationCall call; + ConditionalInitializationFunction f; + VariableAccess initAccess; + Evidence e; + ConditionallyInitializedVariable() { + // Find a call that conditionally initializes this variable + hasConditionalInitialization(f, call, this, initAccess, e) and + // Ignore cases where the variable is assigned prior to the call + not reaches(getAnAssignedValue(), initAccess) and + // Ignore cases where the variable is assigned field-wise prior to the call. + not exists(FieldAccess fa | + exists(Assignment a | + fa = getAFieldAccess(this) and + a.getLValue() = fa + ) | + reaches(fa, initAccess) + ) and + // Ignore cases where the variable is assigned by a prior call to an initialization function + not exists(Call c | + getAnAccess() = getAnInitializedArgument(c).(AddressOfExpr).getOperand() and + reaches(c, initAccess) + ) and + /* + * Static local variables with constant initializers do not have the initializer expr as part of + * the CFG, but should always be considered as initialized, so exclude them. + */ + not exists(getInitializer().getExpr()) + } + + /** + * Gets an access of the variable `v` which is not used as an lvalue, and not used as an argument + * to an initialization function. + */ + private VariableAccess getAReadAccess() { + result = this.getAnAccess() and + // Not used as an lvalue + not result = any(AssignExpr a).getLValue() and + // Not passed to another initialization function + not exists(Call c, int j | j = c.getTarget().(InitializationFunction).initializedParameter() | + result = c.getArgument(j).(AddressOfExpr).getOperand() + ) and + // Not a pointless read + not result = any(ExprStmt es).getExpr() + } + + /** + * Gets a read access of variable `v` that occurs after the `initializingCall`. + */ + private VariableAccess getAReadAccessAfterCall(ConditionalInitializationCall initializingCall) { + // Variable associated with this particular call + call = initializingCall and + // Access is a meaningful read access + result = getAReadAccess() and + // Which occurs after the call + reaches(call, result) and + /* + * Ignore risky accesses which are arguments to calls which also include another parameter to + * the original call. This is an attempt to eliminate results where the "status" can be checked + * through another parameter that assigned as part of the original call. + */ + not exists(Call c | + c.getAnArgument() = result or + c.getAnArgument().(AddressOfExpr).getOperand() = result | + exists(LocalVariable lv | + call.getAnArgument().(AddressOfExpr).getOperand() = lv.getAnAccess() and + not lv = this | + c.getAnArgument() = lv.getAnAccess() + ) + ) + } + + /** + * Gets an access to the variable that is risky because the variable may not be initialized after + * the `call`, and the status of the call is never checked. + */ + VariableAccess getARiskyAccessWithNoStatusCheck(ConditionalInitializationFunction initializingFunction, ConditionalInitializationCall initializingCall, Evidence evidence) { + // Variable associated with this particular call + call = initializingCall and + initializingFunction = f and + e = evidence and + result = getAReadAccessAfterCall(initializingCall) and + ( + // Access is risky because status return code ignored completely + call instanceof ExprInVoidContext + or + // Access is risky because status return code ignored completely + exists(LocalVariable status | call = status.getAnAssignedValue() | not exists(status.getAnAccess())) + ) + } + + /** + * Gets an access to the variable that is risky because the variable may not be initialized after + * the `call`, and the status of the call is only checked after the risky access. + */ + VariableAccess getARiskyAccessBeforeStatusCheck(ConditionalInitializationFunction initializingFunction, ConditionalInitializationCall initializingCall, Evidence evidence) { + // Variable associated with this particular call + call = initializingCall and + initializingFunction = f and + e = evidence and + result = getAReadAccessAfterCall(initializingCall) and + exists(LocalVariable status, Assignment a | + a.getRValue() = call and + call = status.getAnAssignedValue() and + // There exists a check of the status code + definitionUsePair(status, a, _) and + // And the check of the status code does not occur before the risky access + not exists(VariableAccess statusAccess | + definitionUsePair(status, a, statusAccess) and + reaches(statusAccess, result) + ) and + // Ignore cases where the assignment to the status code is used directly + a instanceof ExprInVoidContext and + /* + * Ignore risky accesses which are arguments to calls which also include the status code. + * If both the risky value and status code are passed to a different function, that + * function is responsible for checking the status code. + */ + not exists(Call c | + c.getAnArgument() = result or + c.getAnArgument().(AddressOfExpr).getOperand() = result | + definitionUsePair(status, a, c.getAnArgument()) + ) + ) + } + + /** + * Gets an access to the variable that is risky because the variable may not be initialized after + * the `call`. + */ + VariableAccess getARiskyAccess(ConditionalInitializationFunction initializingFunction, ConditionalInitializationCall initializingCall, Evidence evidence) { + result = getARiskyAccessBeforeStatusCheck(initializingFunction, initializingCall, evidence) or + result = getARiskyAccessWithNoStatusCheck(initializingFunction, initializingCall, evidence) + } +} diff --git a/src/microsoft/Likely Bugs/Memory Management/UnprobedDereference.md b/src/microsoft/Likely Bugs/Memory Management/UnprobedDereference.md new file mode 100644 index 00000000..3dfa226a --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/UnprobedDereference.md @@ -0,0 +1,12 @@ +# User provided pointer dereferenced without a probe. +In low-level Windows kernel code, dereferencing a pointer originating from user mode is dangerous if the pointer has not been validated (probed). An unvalidated pointer may point to privileged kernel memory or invalid addresses. Using proper user-mode accessors such as `ReadULongFromUser` or probing via APIs such as `ProbeForRead` ensures that invalid or malicious user-supplied addresses do not compromise the kernel. + + +## Recommendation +Before dereferencing pointers from user memory in kernel-mode code, always validate them. + + +## References +* Microsoft Learn: [User-mode accessors](https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/user-mode-accessors). +* Microsoft Learn: [ProbeForRead](https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-probeforread). +* Common Weakness Enumeration: [CWE-668](https://cwe.mitre.org/data/definitions/668.html). diff --git a/src/microsoft/Likely Bugs/Memory Management/UnprobedDereference.qhelp b/src/microsoft/Likely Bugs/Memory Management/UnprobedDereference.qhelp new file mode 100644 index 00000000..05219342 --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/UnprobedDereference.qhelp @@ -0,0 +1,24 @@ + + + +

    In low-level Windows kernel code, dereferencing a pointer originating from user mode is dangerous if the pointer has not been validated (probed). + An unvalidated pointer may point to privileged kernel memory or invalid addresses. + Using proper user-mode accessors such as ReadULongFromUser or probing via APIs such as ProbeForRead ensures that invalid or malicious user-supplied addresses do not compromise the kernel.

    +     + + +

    Before dereferencing pointers from user memory in kernel-mode code, always validate them.

    +     + +
  • + Microsoft Learn: + User-mode accessors. +
    • +
  • + Microsoft Learn: + ProbeForRead. +
    • +     +     diff --git a/src/microsoft/Likely Bugs/Memory Management/UnprobedDereference.ql b/src/microsoft/Likely Bugs/Memory Management/UnprobedDereference.ql new file mode 100644 index 00000000..2482434b --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/UnprobedDereference.ql @@ -0,0 +1,46 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** + * @name User provided pointer dereferenced without a probe. + * @description If a user provided pointer is dereferenced without first being + * probed, it could point to kernel mode memory, opening up a + * serious security hole + * @kind problem + * @problem.severity error + * @tags security + * external/cwe/cwe-668 + * @owner.email sdat@microsoft.com + * @id cpp/microsoft/public/likely-bugs/memory-management/unprobeddereference + */ +import microsoft.code.cpp.public.windows.kernel.SystemCalls +import microsoft.code.cpp.public.windows.kernel.MemoryOriginDereferences +import microsoft.code.cpp.public.controlflow.Reachability + +/** + * A dereferencing access that should be ignored because it is either within a probe call, or is + * preceded by one. + */ +class ProbedAccess extends IgnoredAccess { + ProbedAccess() { + exists(ProbeCallAccess probeAccess | probeAccess = this.getTarget().getAnAccess() and reaches(probeAccess, this)) or + this instanceof ProbeCallAccess + } +} + +/** + * A dereferencing access that should be ignored because it is either within a UMA call, or + * preceded by one. + */ +class UMAGuardedAccess extends IgnoredAccess { + UMAGuardedAccess() { + exists(UMAAccess umaAccess | umaAccess = this.getTarget().getAnAccess() and reaches(umaAccess, this)) or + this instanceof UMAAccess + } +} + +from MemoryOrigin o, VariableAccess va +where o.originCanUnmap() and + va = getANestedDereference(o) and + // And the dereferencing expression is not contained in an undefined call + (va.getParent() instanceof Call implies va.getParent().(Call).getTarget().hasDefinition()) +select va, "Variable dereferenced without probe accessing $@", o, o.toString() diff --git a/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryOutsideTry.md b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryOutsideTry.md new file mode 100644 index 00000000..718fc3ea --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryOutsideTry.md @@ -0,0 +1,306 @@ +# User mode memory read outside a try block +Reading user memory outside a try/catch block is discouraged, as unexpected exceptions can occur if untrusted code changes memory protections. + + +## Recommendation +Move all reads to user memory to inside a try-catch block. + + +## Example +The following examples show scenarios where uncaught exceptions can occur. + + +```c +typedef unsigned char UCHAR, *PUCHAR; +typedef int SIZE_T; +typedef UCHAR BOOLEAN; // winnt +#define FALSE 0 +#define TRUE 1 + +#define NULL (0) +typedef unsigned long ULONG; +typedef long LONG; + +typedef void * PVOID; + +typedef long NTSTATUS; + +typedef struct _IO_STATUS_BLOCK { + union { + NTSTATUS Status; + PVOID Pointer; + } DUMMYUNIONNAME; + + ULONG* Information; +} IO_STATUS_BLOCK, *PIO_STATUS_BLOCK; + +typedef struct _IRP { + + union { + struct _IRP *MasterIrp; + LONG IrpCount; + PVOID SystemBuffer; + } AssociatedIrp; + + // + // I/O status - final status of operation. + // + + IO_STATUS_BLOCK IoStatus; + + // + // Note that the UserBuffer parameter is outside of the stack so that I/O + // completion can copy data back into the user's address space without + // having to know exactly which service was being invoked. The length + // of the copy is stored in the second half of the I/O status block. If + // the UserBuffer field is NULL, then no copy is performed. + // + + PVOID UserBuffer; +} IRP; + +typedef IRP *PIRP; + +typedef struct _IO_STACK_LOCATION { + union { + // + // System service parameters for: NtDeviceIoControlFile + // + // Note that the user's output buffer is stored in the UserBuffer field + // and the user's input buffer is stored in the SystemBuffer field. + // + + struct { + ULONG OutputBufferLength; + ULONG InputBufferLength; + ULONG IoControlCode; + PVOID Type3InputBuffer; + } DeviceIoControl; + + } Parameters; + +} IO_STACK_LOCATION, *PIO_STACK_LOCATION; + +PIO_STACK_LOCATION +IoGetCurrentIrpStackLocation( + PIRP Irp +); + +#define EXCEPTION_EXECUTE_HANDLER 1 + +void Read(PVOID buf) { *((PIRP)buf); } + +void ExRaiseAccessViolation(); + +void +TestIrp( + PIRP Irp + ) +{ + PIO_STACK_LOCATION IrpStack = IoGetCurrentIrpStackLocation(Irp); + PVOID buf = IrpStack->Parameters.DeviceIoControl.Type3InputBuffer; + Read(buf); // BAD + *((PIRP)buf); // BAD + + buf = Irp->UserBuffer; + Read(buf); // BAD + *((PIRP)buf); // BAD +} + +typedef struct _MYSTRUCT { + ULONG Length; + PUCHAR Data; +} MYSTRUCT, *PMYSTRUCT; + +void +TestNestedDereference( + PIRP Irp + ) +{ + PIO_STACK_LOCATION IrpStack = IoGetCurrentIrpStackLocation(Irp); + PVOID Buf = IrpStack->Parameters.DeviceIoControl.Type3InputBuffer; + PMYSTRUCT MyStruct = NULL; + PUCHAR Data = NULL; + + *Data; // BAD +} + + +void +TestSystemBufferNestedDereference( + PIRP Irp + ) +{ + PVOID Buf = Irp->AssociatedIrp.SystemBuffer; + *Data; // BAD +} + +__kernel_entry void TestSystemCall(int *buf) +{ + Read(buf); // BAD + *((PIRP)buf); // BAD +} + +``` +To correct the problem, transfer user mode memory use to inside a try-catch block. + + +```c +typedef unsigned char UCHAR, *PUCHAR; +typedef int SIZE_T; +typedef UCHAR BOOLEAN; // winnt +#define FALSE 0 +#define TRUE 1 + +#define NULL (0) +typedef unsigned long ULONG; +typedef long LONG; + +typedef void * PVOID; + +typedef long NTSTATUS; + +typedef struct _IO_STATUS_BLOCK { + union { + NTSTATUS Status; + PVOID Pointer; + } DUMMYUNIONNAME; + + ULONG* Information; +} IO_STATUS_BLOCK, *PIO_STATUS_BLOCK; + +typedef struct _IRP { + + union { + struct _IRP *MasterIrp; + LONG IrpCount; + PVOID SystemBuffer; + } AssociatedIrp; + + // + // I/O status - final status of operation. + // + + IO_STATUS_BLOCK IoStatus; + + // + // Note that the UserBuffer parameter is outside of the stack so that I/O + // completion can copy data back into the user's address space without + // having to know exactly which service was being invoked. The length + // of the copy is stored in the second half of the I/O status block. If + // the UserBuffer field is NULL, then no copy is performed. + // + + PVOID UserBuffer; +} IRP; + +typedef IRP *PIRP; + +typedef struct _IO_STACK_LOCATION { + union { + // + // System service parameters for: NtDeviceIoControlFile + // + // Note that the user's output buffer is stored in the UserBuffer field + // and the user's input buffer is stored in the SystemBuffer field. + // + + struct { + ULONG OutputBufferLength; + ULONG InputBufferLength; + ULONG IoControlCode; + PVOID Type3InputBuffer; + } DeviceIoControl; + + } Parameters; + +} IO_STACK_LOCATION, *PIO_STACK_LOCATION; + +PIO_STACK_LOCATION +IoGetCurrentIrpStackLocation( + PIRP Irp +); + +#define EXCEPTION_EXECUTE_HANDLER 1 + +void Read(PVOID buf) { *((PIRP)buf); } + +void ExRaiseAccessViolation(); + +void +TestIrp( + PIRP Irp + ) +{ + PIO_STACK_LOCATION IrpStack = IoGetCurrentIrpStackLocation(Irp); + PVOID buf = IrpStack->Parameters.DeviceIoControl.Type3InputBuffer; + + __try{ + *((PIRP)buf); // GOOD + } __except(EXCEPTION_EXECUTE_HANDLER) { + ExRaiseAccessViolation(); + } + + buf = Irp->UserBuffer; + + __try{ + Read(buf); // GOOD + } __except(EXCEPTION_EXECUTE_HANDLER) { + ExRaiseAccessViolation(); + } +} + +void +TestSystemBuffer( + PIRP Irp + ) +{ + PVOID buf = Irp->AssociatedIrp.SystemBuffer; + Read(buf); // GOOD + *((PIRP)buf); // GOOD +} + +typedef struct _MYSTRUCT { + ULONG Length; + PUCHAR Data; +} MYSTRUCT, *PMYSTRUCT; + +void +TestNestedDereference( + PIRP Irp + ) +{ + PIO_STACK_LOCATION IrpStack = IoGetCurrentIrpStackLocation(Irp); + PVOID Buf = IrpStack->Parameters.DeviceIoControl.Type3InputBuffer; + PMYSTRUCT MyStruct = NULL; + PUCHAR Data = NULL; + + __try{ + MyStruct = (PMYSTRUCT)Buf; // GOOD + Data = MyStruct->Data; // GOOD + } __except(EXCEPTION_EXECUTE_HANDLER) { + ExRaiseAccessViolation(); + } +} + + +void +TestSystemBufferNestedDereference( + PIRP Irp + ) +{ + PVOID Buf = Irp->AssociatedIrp.SystemBuffer; + PMYSTRUCT MyStruct = (PMYSTRUCT)Buf; // GOOD + PUCHAR Data = MyStruct->Data; // GOOD +} + +__kernel_entry void TestSystemCall(int *buf) +{ + __try{ + *((PIRP)buf); // GOOD + } __except(EXCEPTION_EXECUTE_HANDLER) { + ExRaiseAccessViolation(); + } +} + +``` diff --git a/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryOutsideTry.qhelp b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryOutsideTry.qhelp new file mode 100644 index 00000000..d955dfbf --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryOutsideTry.qhelp @@ -0,0 +1,21 @@ + + + +

    Reading user memory outside a try/catch block is discouraged, as unexpected exceptions can occur if untrusted code changes memory protections.

    +     + + +

    Move all reads to user memory to inside a try-catch block.

    +     + + +

    The following examples show scenarios where uncaught exceptions can occur.

    + + +

    To correct the problem, transfer user mode memory use to inside a try-catch block.

    + +     + +     diff --git a/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryOutsideTry.ql b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryOutsideTry.ql new file mode 100644 index 00000000..607842c8 --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryOutsideTry.ql @@ -0,0 +1,39 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** + * @name User mode memory read outside a try block + * @description Reading user memory outside a try/catch block is discouraged, as + * unexpected exceptions can occur if untrusted code changes memory + * protections. + * @kind problem + * @problem.severity warning + * @tags security + * external/cwe/cwe-755 + * @owner.email sdat@microsoft.com + * @id cpp/microsoft/public/likely-bugs/memory-management/usermodememoryoutsidetry + */ + +import cpp +import microsoft.code.cpp.public.windows.kernel.SystemCalls +import microsoft.code.cpp.public.windows.kernel.MemoryOriginDereferences + +/** + * Ignore dereferencing accesses that occur within Microsoft try-except blocks. + */ +class InsideMicrosoftTry extends IgnoredAccess { + InsideMicrosoftTry() { + isInMicrosoftTry(this) + } +} + +from MemoryOrigin o, Expr e +where + // Memory can be unmapped from user space + o.originCanUnmap() and + // e dereferences the pointer p, outside of a try block + e = getANestedDereference(o) and + // And the dereferencing expression is not contained in an undefined call + (e.getParent() instanceof Call implies e.getParent().(Call).getTarget().hasDefinition()) +select + o,"$@: User mode memory dereferenced $@ outside of a try-catch.", + e.getEnclosingFunction() as enclosingFunction, enclosingFunction.toString(), e, "here" diff --git a/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryOutsideTry1.c b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryOutsideTry1.c new file mode 100644 index 00000000..e2990d73 --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryOutsideTry1.c @@ -0,0 +1,129 @@ +typedef unsigned char UCHAR, *PUCHAR; +typedef int SIZE_T; +typedef UCHAR BOOLEAN; // winnt +#define FALSE 0 +#define TRUE 1 + +#define NULL (0) +typedef unsigned long ULONG; +typedef long LONG; + +typedef void * PVOID; + +typedef long NTSTATUS; + +typedef struct _IO_STATUS_BLOCK { + union { + NTSTATUS Status; + PVOID Pointer; + } DUMMYUNIONNAME; + + ULONG* Information; +} IO_STATUS_BLOCK, *PIO_STATUS_BLOCK; + +typedef struct _IRP { + + union { + struct _IRP *MasterIrp; + LONG IrpCount; + PVOID SystemBuffer; + } AssociatedIrp; + + // + // I/O status - final status of operation. + // + + IO_STATUS_BLOCK IoStatus; + + // + // Note that the UserBuffer parameter is outside of the stack so that I/O + // completion can copy data back into the user's address space without + // having to know exactly which service was being invoked. The length + // of the copy is stored in the second half of the I/O status block. If + // the UserBuffer field is NULL, then no copy is performed. + // + + PVOID UserBuffer; +} IRP; + +typedef IRP *PIRP; + +typedef struct _IO_STACK_LOCATION { + union { + // + // System service parameters for: NtDeviceIoControlFile + // + // Note that the user's output buffer is stored in the UserBuffer field + // and the user's input buffer is stored in the SystemBuffer field. + // + + struct { + ULONG OutputBufferLength; + ULONG InputBufferLength; + ULONG IoControlCode; + PVOID Type3InputBuffer; + } DeviceIoControl; + + } Parameters; + +} IO_STACK_LOCATION, *PIO_STACK_LOCATION; + +PIO_STACK_LOCATION +IoGetCurrentIrpStackLocation( + PIRP Irp +); + +#define EXCEPTION_EXECUTE_HANDLER 1 + +void Read(PVOID buf) { *((PIRP)buf); } + +void ExRaiseAccessViolation(); + +void +TestIrp( + PIRP Irp + ) +{ + PIO_STACK_LOCATION IrpStack = IoGetCurrentIrpStackLocation(Irp); + PVOID buf = IrpStack->Parameters.DeviceIoControl.Type3InputBuffer; + Read(buf); // BAD + *((PIRP)buf); // BAD + + buf = Irp->UserBuffer; + Read(buf); // BAD + *((PIRP)buf); // BAD +} + +typedef struct _MYSTRUCT { + ULONG Length; + PUCHAR Data; +} MYSTRUCT, *PMYSTRUCT; + +void +TestNestedDereference( + PIRP Irp + ) +{ + PIO_STACK_LOCATION IrpStack = IoGetCurrentIrpStackLocation(Irp); + PVOID Buf = IrpStack->Parameters.DeviceIoControl.Type3InputBuffer; + PMYSTRUCT MyStruct = NULL; + PUCHAR Data = NULL; + + *Data; // BAD +} + + +void +TestSystemBufferNestedDereference( + PIRP Irp + ) +{ + PVOID Buf = Irp->AssociatedIrp.SystemBuffer; + *Data; // BAD +} + +__kernel_entry void TestSystemCall(int *buf) +{ + Read(buf); // BAD + *((PIRP)buf); // BAD +} diff --git a/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryOutsideTry2.c b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryOutsideTry2.c new file mode 100644 index 00000000..d38a4a2c --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryOutsideTry2.c @@ -0,0 +1,156 @@ +typedef unsigned char UCHAR, *PUCHAR; +typedef int SIZE_T; +typedef UCHAR BOOLEAN; // winnt +#define FALSE 0 +#define TRUE 1 + +#define NULL (0) +typedef unsigned long ULONG; +typedef long LONG; + +typedef void * PVOID; + +typedef long NTSTATUS; + +typedef struct _IO_STATUS_BLOCK { + union { + NTSTATUS Status; + PVOID Pointer; + } DUMMYUNIONNAME; + + ULONG* Information; +} IO_STATUS_BLOCK, *PIO_STATUS_BLOCK; + +typedef struct _IRP { + + union { + struct _IRP *MasterIrp; + LONG IrpCount; + PVOID SystemBuffer; + } AssociatedIrp; + + // + // I/O status - final status of operation. + // + + IO_STATUS_BLOCK IoStatus; + + // + // Note that the UserBuffer parameter is outside of the stack so that I/O + // completion can copy data back into the user's address space without + // having to know exactly which service was being invoked. The length + // of the copy is stored in the second half of the I/O status block. If + // the UserBuffer field is NULL, then no copy is performed. + // + + PVOID UserBuffer; +} IRP; + +typedef IRP *PIRP; + +typedef struct _IO_STACK_LOCATION { + union { + // + // System service parameters for: NtDeviceIoControlFile + // + // Note that the user's output buffer is stored in the UserBuffer field + // and the user's input buffer is stored in the SystemBuffer field. + // + + struct { + ULONG OutputBufferLength; + ULONG InputBufferLength; + ULONG IoControlCode; + PVOID Type3InputBuffer; + } DeviceIoControl; + + } Parameters; + +} IO_STACK_LOCATION, *PIO_STACK_LOCATION; + +PIO_STACK_LOCATION +IoGetCurrentIrpStackLocation( + PIRP Irp +); + +#define EXCEPTION_EXECUTE_HANDLER 1 + +void Read(PVOID buf) { *((PIRP)buf); } + +void ExRaiseAccessViolation(); + +void +TestIrp( + PIRP Irp + ) +{ + PIO_STACK_LOCATION IrpStack = IoGetCurrentIrpStackLocation(Irp); + PVOID buf = IrpStack->Parameters.DeviceIoControl.Type3InputBuffer; + + __try{ + *((PIRP)buf); // GOOD + } __except(EXCEPTION_EXECUTE_HANDLER) { + ExRaiseAccessViolation(); + } + + buf = Irp->UserBuffer; + + __try{ + Read(buf); // GOOD + } __except(EXCEPTION_EXECUTE_HANDLER) { + ExRaiseAccessViolation(); + } +} + +void +TestSystemBuffer( + PIRP Irp + ) +{ + PVOID buf = Irp->AssociatedIrp.SystemBuffer; + Read(buf); // GOOD + *((PIRP)buf); // GOOD +} + +typedef struct _MYSTRUCT { + ULONG Length; + PUCHAR Data; +} MYSTRUCT, *PMYSTRUCT; + +void +TestNestedDereference( + PIRP Irp + ) +{ + PIO_STACK_LOCATION IrpStack = IoGetCurrentIrpStackLocation(Irp); + PVOID Buf = IrpStack->Parameters.DeviceIoControl.Type3InputBuffer; + PMYSTRUCT MyStruct = NULL; + PUCHAR Data = NULL; + + __try{ + MyStruct = (PMYSTRUCT)Buf; // GOOD + Data = MyStruct->Data; // GOOD + } __except(EXCEPTION_EXECUTE_HANDLER) { + ExRaiseAccessViolation(); + } +} + + +void +TestSystemBufferNestedDereference( + PIRP Irp + ) +{ + PVOID Buf = Irp->AssociatedIrp.SystemBuffer; + PMYSTRUCT MyStruct = (PMYSTRUCT)Buf; // GOOD + PUCHAR Data = MyStruct->Data; // GOOD +} + +__kernel_entry void TestSystemCall(int *buf) +{ + __try{ + *((PIRP)buf); // GOOD + } __except(EXCEPTION_EXECUTE_HANDLER) { + ExRaiseAccessViolation(); + } +} diff --git a/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryReadMultipleTimes.md b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryReadMultipleTimes.md new file mode 100644 index 00000000..f7bd90db --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryReadMultipleTimes.md @@ -0,0 +1,260 @@ +# Double fetch of user memory +User mode memory should always be transferred to kernel memory before being read repeatedly, to avoid its content changing based on user actions ("double fetch" vulnerability). + + +## Recommendation +Transfer user mode memory to kernel memory. + + +## Example +The following examples show scenarios where double fetch may occur. + + +```c +typedef unsigned char UCHAR; +typedef int SIZE_T; +typedef short SHORT; +typedef UCHAR BOOLEAN; // winnt +#define FALSE 0 +#define TRUE 1 + +#define NULL (0) +typedef unsigned long ULONG; +typedef long LONG; + +typedef void * PVOID; + +typedef long NTSTATUS; + +typedef struct _IO_STATUS_BLOCK { + union { + NTSTATUS Status; + PVOID Pointer; + } DUMMYUNIONNAME; + + ULONG* Information; +} IO_STATUS_BLOCK, *PIO_STATUS_BLOCK; + +typedef struct _MDL { + SHORT Size; +} MDL, *PMDL; + +typedef struct _IRP { + + union { + struct _IRP *MasterIrp; + LONG IrpCount; + PVOID SystemBuffer; + } AssociatedIrp; + + // + // I/O status - final status of operation. + // + + IO_STATUS_BLOCK IoStatus; + + // + // Note that the UserBuffer parameter is outside of the stack so that I/O + // completion can copy data back into the user's address space without + // having to know exactly which service was being invoked. The length + // of the copy is stored in the second half of the I/O status block. If + // the UserBuffer field is NULL, then no copy is performed. + // + + PVOID UserBuffer; + + PMDL MdlAddress; +} IRP; + +typedef IRP *PIRP; + +typedef struct _IO_STACK_LOCATION { + union { + // + // System service parameters for: NtDeviceIoControlFile + // + // Note that the user's output buffer is stored in the UserBuffer field + // and the user's input buffer is stored in the SystemBuffer field. + // + + struct { + ULONG OutputBufferLength; + ULONG InputBufferLength; + ULONG IoControlCode; + PVOID Type3InputBuffer; + } DeviceIoControl; + + } Parameters; + +} IO_STACK_LOCATION, *PIO_STACK_LOCATION; + +PIO_STACK_LOCATION +IoGetCurrentIrpStackLocation( + PIRP Irp +); + +void Read(PVOID buf) { *((PIRP)buf); } + +typedef struct _MyStruct { + PVOID Field; +} MyStruct, *PMyStruct; + +void DoDoubleFetch(PVOID buf) +{ + Read(buf); + *((PIRP)buf); +} + +void TestIrp(PIRP Irp) +{ + // IRP: Type3InputBuffer double fetch + PIO_STACK_LOCATION IrpStack = IoGetCurrentIrpStackLocation(Irp); + PVOID buf = IrpStack->Parameters.DeviceIoControl.Type3InputBuffer; + DoDoubleFetch(buf); + + // IRP: UserBuffer double fetch + buf = Irp->UserBuffer; + DoDoubleFetch(buf); + + // IRP: Access to user buffer via IO manager copied SystemBuffer + PMyStruct s = (PMyStruct)Irp->AssociatedIrp.SystemBuffer; + DoDoubleFetch(s->Field); + + // MDL: MmGetSystemAddressForMdlSafe + SIZE_T *input = (SIZE_T*) MmGetSystemAddressForMdlSafe(Irp->MdlAddress); + DoDoubleFetch(input); + + // IRP: Access to SystemBuffer, no warning expected + SIZE_T *sb = (SIZE_T*) Irp->AssociatedIrp.SystemBuffer; + DoDoubleFetch(sb); +} +``` +To correct the problem, be sure to read from user memory only once. + + +```c +typedef unsigned char UCHAR; +typedef int SIZE_T; +typedef short SHORT; +typedef UCHAR BOOLEAN; // winnt +#define FALSE 0 +#define TRUE 1 + +#define NULL (0) +typedef unsigned long ULONG; +typedef long LONG; + +typedef void * PVOID; + +typedef long NTSTATUS; + +typedef struct _IO_STATUS_BLOCK { + union { + NTSTATUS Status; + PVOID Pointer; + } DUMMYUNIONNAME; + + ULONG* Information; +} IO_STATUS_BLOCK, *PIO_STATUS_BLOCK; + +typedef struct _MDL { + SHORT Size; +} MDL, *PMDL; + +typedef struct _IRP { + + union { + struct _IRP *MasterIrp; + LONG IrpCount; + PVOID SystemBuffer; + } AssociatedIrp; + + // + // I/O status - final status of operation. + // + + IO_STATUS_BLOCK IoStatus; + + // + // Note that the UserBuffer parameter is outside of the stack so that I/O + // completion can copy data back into the user's address space without + // having to know exactly which service was being invoked. The length + // of the copy is stored in the second half of the I/O status block. If + // the UserBuffer field is NULL, then no copy is performed. + // + + PVOID UserBuffer; + + PMDL MdlAddress; +} IRP; + +typedef IRP *PIRP; + +typedef struct _IO_STACK_LOCATION { + union { + // + // System service parameters for: NtDeviceIoControlFile + // + // Note that the user's output buffer is stored in the UserBuffer field + // and the user's input buffer is stored in the SystemBuffer field. + // + + struct { + ULONG OutputBufferLength; + ULONG InputBufferLength; + ULONG IoControlCode; + PVOID Type3InputBuffer; + } DeviceIoControl; + + } Parameters; + +} IO_STACK_LOCATION, *PIO_STACK_LOCATION; + +PIO_STACK_LOCATION +IoGetCurrentIrpStackLocation( + PIRP Irp +); + +#define EXCEPTION_EXECUTE_HANDLER 1 + +void Read(PVOID buf) { *((PIRP)buf); } + +void ExRaiseAccessViolation(); + +PVOID MmGetSystemAddressForMdlSafe(PMDL MdlAddress); + +typedef int UINT32; + +typedef struct _MyStruct { + PVOID Field; +} MyStruct, *PMyStruct; + +void TestIrp(PIRP Irp) +{ + // IRP: Type3InputBuffer + PIO_STACK_LOCATION IrpStack = IoGetCurrentIrpStackLocation(Irp); + PVOID buf = IrpStack->Parameters.DeviceIoControl.Type3InputBuffer; + Read(buf); + + // IRP: UserBuffer + buf = Irp->UserBuffer; + Read(buf); + + // IRP: Access to user buffer via IO manager copied SystemBuffer + PMyStruct s = (PMyStruct)Irp->AssociatedIrp.SystemBuffer; + Read(s->Field); + + // MDL: MmGetSystemAddressForMdlSafe + SIZE_T *input = (SIZE_T*) MmGetSystemAddressForMdlSafe(Irp->MdlAddress); + Read(input); + + // IRP: Access to SystemBuffer, no warning expected + SIZE_T *sb = (SIZE_T*) Irp->AssociatedIrp.SystemBuffer; + Read(sb); +} + +``` + +## References +* MSRC: [MS08-061: The case of the kernel mode double-fetch](https://msrc-blog.microsoft.com/2008/10/14/ms08-061-the-case-of-the-kernel-mode-double-fetch/#:~:text=When%20accessing%20user%20mode%20data%2C%20kernel%20mode%20code,type%20of%20problem%20known%20as%20a%20%E2%80%9Cdouble%20fetch%E2%80%9D.). +* Common Weakness Enumeration: [CWE-672](https://cwe.mitre.org/data/definitions/672.html). diff --git a/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryReadMultipleTimes.qhelp b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryReadMultipleTimes.qhelp new file mode 100644 index 00000000..40d8d154 --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryReadMultipleTimes.qhelp @@ -0,0 +1,26 @@ + + + +

    User mode memory should always be transferred to kernel memory before being read repeatedly, to avoid its content changing based on user actions ("double fetch" vulnerability).

    +     + + +

    Transfer user mode memory to kernel memory.

    +     + + +

    The following examples show scenarios where double fetch may occur.

    + + +

    To correct the problem, be sure to read from user memory only once.

    + + +     + + +
  • MSRC: MS08-061: The case of the kernel mode double-fetch.
    • +     + +     diff --git a/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryReadMultipleTimes.ql b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryReadMultipleTimes.ql new file mode 100644 index 00000000..7beff699 --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryReadMultipleTimes.ql @@ -0,0 +1,137 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** + * @name Double fetch of user memory + * @description User mode memory should always be transferred to kernel memory before + * being read repeatedly, to avoid its content changing based on user + * actions ("double fetch" vulnerability). + * @kind problem + * @problem.severity error + * @tags security + * external/cwe/cwe-672 + * @owner.email sdat@microsoft.com + * @id cpp/microsoft/public/likely-bugs/memory-management/usermodememoryreadmultipletimes + */ + +import cpp +import UserModeMemoryReadMultipleTimes + + +/** + * A memory origin relevant for this query. Either an MDL parameter, + * an IRP parameter or a parameter of a system call function (a + * function with SAL annotation `__kernel_entry` or `_Kernel_entry_` or + * `_Kernel_entry_always_` or `__control_entrypoint`). + */ +class RelevantMemoryOrigin extends Element { + RelevantMemoryOrigin() { + this.(MemoryOrigin).originCanWrite() + } + + /** + * Gets an expression that accesses this memory origin, either directly or + * indirectly as determined by interprocedural data flow analysis. + */ + LocalAccess getAnAccess() { + memoryOriginDataFlow(this, result) + } +} + +/** + * Holds if there is a use-use relationship from `access1` to `access2` or from + * `access2` to `access1`. + */ +predicate symmetricUseUse(LocalAccess access1, LocalAccess access2) { + useUsePair(_, access1, access2) + or + useUsePair(_, access2, access1) +} + +/** + * A `LocalAccess` that is most likely not filtered out in the later stages of + * this query. This class exists for performance, so that earlier stages of the + * query can start from a smaller set of accesses. + */ +class AccessCandidate extends LocalAccess { + AccessCandidate() { + exists(RelevantMemoryOrigin origin | this = origin.getAnAccess()) and + dereferenceDataFlow(this, _) and + symmetricUseUse(this, _) + } +} + +/** + * Holds if some memory origin flows (inter-procedurally) to both + * `access1` and `access2`. The two accesses form a use-use pair + * (either direction), `access[1|2]` flows (inter-procedurally) to + * `deref[1|2]`, and the two dereferences may potentially dereference + * overlapping memory. + */ +predicate flowsToDereferencingAccessPair(AccessCandidate access1, UserModeDereferencingOperation deref1, AccessCandidate access2, UserModeDereferencingOperation deref2) { + symmetricUseUse(access1, access2) and + dereferenceDataFlow(access1, deref1) and + flowsToDereferencingAccess(access2, deref2, deref1) +} + +// This predicate is factored out for performance +pragma[noinline] +predicate flowsToDereferencingAccess(AccessCandidate access2, UserModeDereferencingOperation deref2, UserModeDereferencingOperation deref1) { + dereferenceDataFlow(access2, deref2) and + deref1 = deref2.getAnEquivalent() +} + +predicate alertCandidate( + LocalAccess access1, + UserModeDereferencingOperation deref1, + UserModeDereferencingOperation deref2) +{ + flowsToDereferencingAccessPair(access1, deref1, _, deref2) +} + +// deref1 and deref2 read the same bits if in a bit read expr. +predicate sameBitsIfBitRead(UserModeDereferencingOperation deref1, UserModeDereferencingOperation deref2) +{ + alertCandidate(_, deref1, deref2) and + ( + not exists(BitReadExpr r1 | deref1 = r1.getOperand()) + or + not exists(BitReadExpr r2 | deref2 = r2.getOperand()) + or + exists(BitReadExpr r1, BitReadExpr r2 | + deref1 = r1.getOperand() and + deref2 = r2.getOperand() + | r1.isOverlapping(r2) + ) + ) +} + +// This predicate is too slow when the QL optimizer chooses the join order. +// The `noopt` pragma here means that the join order is left to right as +// written in the source. +pragma[noopt] +predicate alert( + RelevantMemoryOrigin origin, + LocalAccess access1, + UserModeDereferencingOperation deref1) +{ + exists(UserModeDereferencingOperation deref2 | + alertCandidate(access1, deref1, deref2) and + sameBitsIfBitRead(deref1, deref2) and + // check for mitigating circumstances + not isMitigated(deref1, deref2) and + access1 = origin.getAnAccess() + ) +} + +from RelevantMemoryOrigin origin, + LocalAccess access1, + UserModeDereferencingOperation deref1 +where alert(origin, access1, deref1) +select origin, + "Buffer from " + origin.(MemoryOrigin).getADirectBufferAccess().getEnclosingFunction() + + " line " +origin.getLocation().getStartLine() + + " is dereferenced in '$@', which may be one of a pair of double fetches", + access1, + access1.getEnclosingFunction().getName() + + " line " + access1.(Expr).getLocation().getStartLine() + + " (" + access1 + ")" diff --git a/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryReadMultipleTimes.qll b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryReadMultipleTimes.qll new file mode 100644 index 00000000..c8b20fef --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryReadMultipleTimes.qll @@ -0,0 +1,165 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** + * Provides classes for identifying double fetch problems, and common mitigations. + */ +import cpp +import microsoft.code.cpp.public.windows.kernel.MemoryOrigins +import microsoft.code.cpp.public.windows.kernel.MemoryOriginDereferences +import microsoft.code.cpp.public.windows.kernel.KernelMode +import microsoft.code.cpp.public.controlflow.Reachability +import microsoft.code.cpp.public.controlflow.Dereferences + +/** + * An expression that dereferences a pointer that may point to + * user-mode memory. + */ +class UserModeDereferencingOperation extends DefaultDereferencingAccess { + UserModeDereferencingOperation() { + this = getANestedDereference(_) and + isReadExpr(this) + } + + MemoryOrigin getAnOrigin() { + this = getANestedDereference(result) + } + + /** + * Gets a dereference that accesses some of the same memory as this dereference, for the + * same reason (either reading or writing). + */ + UserModeDereferencingOperation getAnEquivalent() { + result = this.getAnEquivalentCandidate() + and + // We are only interested in expressions that both read or both write to the same address. + (isWriteExprTarget(this) implies isWriteExprTarget(result)) + and + (isWriteExprTarget(result) implies isWriteExprTarget(this)) + and + /* + * Are the two dereferences "overlapping"? Specifically, can they potentially read the same + * location in memory. + */ + ( + // `this` is not a more specific type + (not this.getParent() instanceof FieldAccess and + not this.getParent().(ArrayExpr).getParent() instanceof FieldAccess) or + // `result` is not a more specific type + (not result.getParent() instanceof FieldAccess and + not result.getParent().(ArrayExpr).getParent() instanceof FieldAccess) or + // ptr->Field.field and ptr->Field.field should be the same + this.getParent().(FieldAccess).getParent().(FieldAccess).getTarget().getName() = result.getParent().(FieldAccess).getParent().(FieldAccess).getTarget().getName() or + // ptr[_].field and ptr[_].field are the same + this.getParent().(ArrayExpr).getParent().(FieldAccess).getTarget().getName() = result.getParent().(ArrayExpr).getParent().(FieldAccess).getTarget().getName() or + // ptr->Field are equivalent to the same fields + (not this.getParent().(FieldAccess).getParent() instanceof FieldAccess and + this.getParent().(FieldAccess).getTarget().getName() = result.getParent().(FieldAccess).getTarget().getName() and exists(this.getParent().(FieldAccess).getQualifier())) + ) + } + + private UserModeDereferencingOperation getAnEquivalentCandidate() { + result = getAnEquivalentCandidateInSameFunction() or + result = getAnEquivalentCandidateInOtherFunction() + } + + private UserModeDereferencingOperation getAnEquivalentCandidateInSameFunction() { + // There must exist an origin that may ultimately be dereferenced + // by both this and the result. + exists(MemoryOrigin mo | + this = getANestedDereference(mo) and + result = getANestedDereference(mo) + ) + and + this.getEnclosingFunction() = result.getEnclosingFunction() + and + ( + /* + * Either the dereferences form a use-use pair, or they dereference different + * variables, in which case they must be able to occur in the same program + * execution ("reachability" through the control flow graph). + */ + useUsePair(_, this, result) + or + useUsePair(_, result, this) + or + not this.(VariableAccess).getTarget().getAnAccess() = result and + (reaches(this, result) or reaches(result, this)) + ) + } + + UserModeDereferencingOperation getAnEquivalentCandidateInOtherFunction() { + // There must exist an origin that may ultimately be dereference by both this and the result. + exists(MemoryOrigin mo | + this = getANestedDereference(mo) and + result = getANestedDereference(mo) + ) and + not this.getEnclosingFunction() = result.getEnclosingFunction() + } +} + +/** + * Whether the dereference of user mode data at `deref2` is overwritten with the user mode data read + * at `deref1`. + * + * The pattern here is: + * ``` + * size = input->size; + * s = ProbeAndReadStructure(input, size); + * s->size = size; + * ``` + * + * This is a genuine case of double fetch, but is mitigated because the second fetch of + * `input->size` is overwritten without being read after the copy. + */ +predicate isMitigated(UserModeDereferencingOperation deref1, UserModeDereferencingOperation deref2) { + exists(string fieldName, Variable tempVariable | + isMitigated1(deref1, fieldName, tempVariable) and + isMitigated2(fieldName, tempVariable, deref2) + ) +} + +private predicate isMitigated1(UserModeDereferencingOperation deref1, string fieldName, Variable tempVariable) { + /* + * `deref1` is a read of `fieldName` to `tempVariable`. + * `deref2` is involved in copying the whole struct to `copiedStruct`. + * `tempVariable is written back to the `copiedStruct`. + */ + // Identify the assignment of something like a size field to the tempVariable + exists(AssignExpr ae, FieldAccess fa | + ae.getRValue() = fa or + ae.getRValue().(FunctionCall).getArgument(0).getAChild+() = fa | + tempVariable = ae.getLValue().(VariableAccess).getTarget() and + fa.getQualifier() = deref1 and + fieldName = fa.getTarget().getName() + ) +} + +private predicate isMitigated2(string fieldName, Variable tempVariable, UserModeDereferencingOperation deref2) { + exists(Variable copiedStruct | + // Identify the variable which the second deref is copied to + copiedStruct = any(MemoryCopy copy | copy.getCopySourceExpr() = deref2).getCopyTargetVariable() and + // Identify the assignment of the temporary variable back to the struct field + exists(AssignExpr ae, FieldAccess fa | + ae.getLValue() = fa and + copiedStruct = fa.getQualifier().(VariableAccess).getTarget() and + fa.getTarget().getName() = fieldName and + tempVariable = ae.getRValue().(VariableAccess).getTarget() + ) + ) +} + +/** + * A read of a pointer variable which is immediately combined in a bitwise and operation + * with a hex literal. + */ +class BitReadExpr extends BitwiseAndExpr { + BitReadExpr() { + getAnOperand().(PointerDereferenceExpr).getOperand() instanceof VariableAccess and + getAnOperand() instanceof HexLiteral + } + VariableAccess getOperand() { result = getAnOperand().(PointerDereferenceExpr).getOperand() } + int getLiteral() { result = getAnOperand().(HexLiteral).getValue().toInt() } + predicate isOverlapping(BitReadExpr other) { + 0 != getLiteral().bitAnd(other.getLiteral()) + } +} diff --git a/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryReadMultipleTimes1.c b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryReadMultipleTimes1.c new file mode 100644 index 00000000..3cd9d722 --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryReadMultipleTimes1.c @@ -0,0 +1,117 @@ +typedef unsigned char UCHAR; +typedef int SIZE_T; +typedef short SHORT; +typedef UCHAR BOOLEAN; // winnt +#define FALSE 0 +#define TRUE 1 + +#define NULL (0) +typedef unsigned long ULONG; +typedef long LONG; + +typedef void * PVOID; + +typedef long NTSTATUS; + +typedef struct _IO_STATUS_BLOCK { + union { + NTSTATUS Status; + PVOID Pointer; + } DUMMYUNIONNAME; + + ULONG* Information; +} IO_STATUS_BLOCK, *PIO_STATUS_BLOCK; + +typedef struct _MDL { + SHORT Size; +} MDL, *PMDL; + +typedef struct _IRP { + + union { + struct _IRP *MasterIrp; + LONG IrpCount; + PVOID SystemBuffer; + } AssociatedIrp; + + // + // I/O status - final status of operation. + // + + IO_STATUS_BLOCK IoStatus; + + // + // Note that the UserBuffer parameter is outside of the stack so that I/O + // completion can copy data back into the user's address space without + // having to know exactly which service was being invoked. The length + // of the copy is stored in the second half of the I/O status block. If + // the UserBuffer field is NULL, then no copy is performed. + // + + PVOID UserBuffer; + + PMDL MdlAddress; +} IRP; + +typedef IRP *PIRP; + +typedef struct _IO_STACK_LOCATION { + union { + // + // System service parameters for: NtDeviceIoControlFile + // + // Note that the user's output buffer is stored in the UserBuffer field + // and the user's input buffer is stored in the SystemBuffer field. + // + + struct { + ULONG OutputBufferLength; + ULONG InputBufferLength; + ULONG IoControlCode; + PVOID Type3InputBuffer; + } DeviceIoControl; + + } Parameters; + +} IO_STACK_LOCATION, *PIO_STACK_LOCATION; + +PIO_STACK_LOCATION +IoGetCurrentIrpStackLocation( + PIRP Irp +); + +void Read(PVOID buf) { *((PIRP)buf); } + +typedef struct _MyStruct { + PVOID Field; +} MyStruct, *PMyStruct; + +void DoDoubleFetch(PVOID buf) +{ + Read(buf); + *((PIRP)buf); +} + +void TestIrp(PIRP Irp) +{ + // IRP: Type3InputBuffer double fetch + PIO_STACK_LOCATION IrpStack = IoGetCurrentIrpStackLocation(Irp); + PVOID buf = IrpStack->Parameters.DeviceIoControl.Type3InputBuffer; + DoDoubleFetch(buf); + + // IRP: UserBuffer double fetch + buf = Irp->UserBuffer; + DoDoubleFetch(buf); + + // IRP: Access to user buffer via IO manager copied SystemBuffer + PMyStruct s = (PMyStruct)Irp->AssociatedIrp.SystemBuffer; + DoDoubleFetch(s->Field); + + // MDL: MmGetSystemAddressForMdlSafe + SIZE_T *input = (SIZE_T*) MmGetSystemAddressForMdlSafe(Irp->MdlAddress); + DoDoubleFetch(input); + + // IRP: Access to SystemBuffer, no warning expected + SIZE_T *sb = (SIZE_T*) Irp->AssociatedIrp.SystemBuffer; + DoDoubleFetch(sb); +} \ No newline at end of file diff --git a/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryReadMultipleTimes2.c b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryReadMultipleTimes2.c new file mode 100644 index 00000000..82c9b908 --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/UserModeMemoryReadMultipleTimes2.c @@ -0,0 +1,111 @@ +typedef unsigned char UCHAR; +typedef int SIZE_T; +typedef short SHORT; +typedef UCHAR BOOLEAN; // winnt +#define FALSE 0 +#define TRUE 1 + +#define NULL (0) +typedef unsigned long ULONG; +typedef long LONG; + +typedef void * PVOID; + +typedef long NTSTATUS; + +typedef struct _IO_STATUS_BLOCK { + union { + NTSTATUS Status; + PVOID Pointer; + } DUMMYUNIONNAME; + + ULONG* Information; +} IO_STATUS_BLOCK, *PIO_STATUS_BLOCK; + +typedef struct _MDL { + SHORT Size; +} MDL, *PMDL; + +typedef struct _IRP { + + union { + struct _IRP *MasterIrp; + LONG IrpCount; + PVOID SystemBuffer; + } AssociatedIrp; + + // + // I/O status - final status of operation. + // + + IO_STATUS_BLOCK IoStatus; + + // + // Note that the UserBuffer parameter is outside of the stack so that I/O + // completion can copy data back into the user's address space without + // having to know exactly which service was being invoked. The length + // of the copy is stored in the second half of the I/O status block. If + // the UserBuffer field is NULL, then no copy is performed. + // + + PVOID UserBuffer; + + PMDL MdlAddress; +} IRP; + +typedef IRP *PIRP; + +typedef struct _IO_STACK_LOCATION { + union { + // + // System service parameters for: NtDeviceIoControlFile + // + // Note that the user's output buffer is stored in the UserBuffer field + // and the user's input buffer is stored in the SystemBuffer field. + // + + struct { + ULONG OutputBufferLength; + ULONG InputBufferLength; + ULONG IoControlCode; + PVOID Type3InputBuffer; + } DeviceIoControl; + + } Parameters; + +} IO_STACK_LOCATION, *PIO_STACK_LOCATION; + +PIO_STACK_LOCATION +IoGetCurrentIrpStackLocation( + PIRP Irp +); + +void Read(PVOID buf) { *((PIRP)buf); } + +typedef struct _MyStruct { + PVOID Field; +} MyStruct, *PMyStruct; + +void TestIrp(PIRP Irp) +{ + // IRP: Type3InputBuffer + PIO_STACK_LOCATION IrpStack = IoGetCurrentIrpStackLocation(Irp); + PVOID buf = IrpStack->Parameters.DeviceIoControl.Type3InputBuffer; + Read(buf); + + // IRP: UserBuffer + buf = Irp->UserBuffer; + Read(buf); + + // IRP: Access to user buffer via IO manager copied SystemBuffer + PMyStruct s = (PMyStruct)Irp->AssociatedIrp.SystemBuffer; + Read(s->Field); + + // MDL: MmGetSystemAddressForMdlSafe + SIZE_T *input = (SIZE_T*) MmGetSystemAddressForMdlSafe(Irp->MdlAddress); + Read(input); + + // IRP: Access to SystemBuffer, no warning expected + SIZE_T *sb = (SIZE_T*) Irp->AssociatedIrp.SystemBuffer; + Read(sb); +} diff --git a/src/microsoft/Likely Bugs/UnguardedNullReturnDereference.md b/src/microsoft/Likely Bugs/UnguardedNullReturnDereference.md new file mode 100644 index 00000000..6e2b0350 --- /dev/null +++ b/src/microsoft/Likely Bugs/UnguardedNullReturnDereference.md @@ -0,0 +1,105 @@ +# Result of call that may return NULL dereferenced unconditionally +A null pointer dereference can cause program crash or exit. + + +## Recommendation +Make sure to initialize all pointer fields before usage. + + +## Example +The following examples show scenarios where a pointer is dereferenced without a null check. + + +```c +#define NULL (0) +typedef void * PVOID; +typedef enum _POOL_TYPE { + NonPagedPool + // ... +} POOL_TYPE; +typedef int SIZE_T; +typedef unsigned long ULONG; + +PVOID ExAllocatePoolWithTag(POOL_TYPE, SIZE_T, ULONG); + +void use(char* data); + +void free(char* data); + +char* malloc(int size); + +void test(bool b) { + char* data; + data = (char*) ExAllocatePoolWithTag(NonPagedPool, sizeof(*data), 'tag'); + // BAD: 'data' is not null-checked + use(data); + + char* data2 = (char*) ExAllocatePoolWithTag(NonPagedPool, sizeof(*data2), 'tag'); + // BAD: 'data2' is not null-checked + use(data2); + + if (b) + { + // BAD + char* data6 = (char*)malloc(12); + use(data6); + } +} + +``` +To correct the problem, check the pointer before dereferencing. + + +```c +#define NULL (0) +typedef void * PVOID; +typedef enum _POOL_TYPE { + NonPagedPool + // ... +} POOL_TYPE; +typedef int SIZE_T; +typedef unsigned long ULONG; + +PVOID ExAllocatePoolWithTag(POOL_TYPE, SIZE_T, ULONG); + +void use(char* data); + +void free(char* data); + +char* malloc(int size); + +void test(bool b) { + char* data; + + data = (char*) ExAllocatePoolWithTag(NonPagedPool, sizeof(*data), 'tag'); + if (data != NULL) + // GOOD: 'data' is null-checked + use(data); + + char* data3 = (char*) ExAllocatePoolWithTag(NonPagedPool, sizeof(*data3), 'tag'); + if (data3 != NULL) + // GOOD: 'data' is null-checked + use(data3); + + char *data4 = malloc(12); + // GOOD: 'free' is null-safe + free(data4); + + // GOOD + if (char* data5 = (char*)malloc(12)) + { + use(data5); + } + + // GOOD + if (data4 = (char*)malloc(12)) + { + use(data4); + } +} + +``` + +## References +* CWE: [CWE-476: NULL Pointer Dereference](https://cwe.mitre.org/data/definitions/476.html). +* Common Weakness Enumeration: [CWE-476](https://cwe.mitre.org/data/definitions/476.html). diff --git a/src/microsoft/Likely Bugs/UnguardedNullReturnDereference.qhelp b/src/microsoft/Likely Bugs/UnguardedNullReturnDereference.qhelp new file mode 100644 index 00000000..95c97135 --- /dev/null +++ b/src/microsoft/Likely Bugs/UnguardedNullReturnDereference.qhelp @@ -0,0 +1,24 @@ + + + +

    A null pointer dereference can cause program crash or exit.

    +     + + +

    Make sure to initialize all pointer fields before usage.

    +     + + +

    The following examples show scenarios where a pointer is dereferenced without a null check.

    + + +

    To correct the problem, check the pointer before dereferencing.

    + +     + +
  • CWE: CWE-476: NULL Pointer Dereference.
    • +     + +     diff --git a/src/microsoft/Likely Bugs/UnguardedNullReturnDereference.ql b/src/microsoft/Likely Bugs/UnguardedNullReturnDereference.ql new file mode 100644 index 00000000..1f3ecacd --- /dev/null +++ b/src/microsoft/Likely Bugs/UnguardedNullReturnDereference.ql @@ -0,0 +1,146 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** + * @name Result of call that may return NULL dereferenced unconditionally + * @description If a call is known to return NULL, the result should be checked + * before dereferencing. + * @kind problem + * @problem.severity error + * @tags security + * external/cwe/cwe-476 + * @owner.email sdat@microsoft.com + * @id cpp/microsoft/public/likely-bugs/unguardednullreturndereference + */ + +import microsoft.code.cpp.public.commons.Literals +import microsoft.code.cpp.public.commons.MemoryAllocation +import microsoft.code.cpp.public.controlflow.Dereferences +import microsoft.code.cpp.public.controlflow.Reachability +import drivers.libraries.SAL +import semmle.code.cpp.controlflow.StackVariableReachability + +/** + * Holds if `call` may return `null`. + */ +predicate possiblyNull(Call call) { + exists(Function f | + f = call.getTarget() and + // NdisGetDataBuffer only returns null if the size parameter is not statically assigned + ( + call.getTarget().hasName("NdisGetDataBuffer") + implies + not call.getArgument(1) instanceof StaticValue + ) + | + mayReturnNull(f) or + // SAL annotation indicates the result should be checked. + any(SALAnnotation a | a.getMacroName() = "_Must_inspect_result_").getDeclaration() = f or + any(SALAnnotation a | a.getMacroName() = "_Result_nullonfailure_").getDeclaration() = f + ) + or + // Heap allocations should be checked for null. + call instanceof HeapAllocation +} + +/** + * Holds if `call` is assigned to variable `v` in control flow node + * `cfn`, where `call` may return `null`. + */ +predicate possiblyNullAssignment(ControlFlowNode cfn, Call call, StackVariable v) { + possiblyNull(call) and + v.getType().getUnspecifiedType() instanceof PointerType and + ( + cfn = + any(AssignExpr ae | + ae.getLValue() = v.getAnAccess() and + ae.getRValue() = call + ) + or + exists(Initializer i | + i = v.getInitializer() and + i.getExpr() = call and + cfn = call + ) + ) and + not exists(ConditionDeclExpr cde | cde.getVariable() = v) +} + +class UnguardedNullReturnDereferenceReachability extends StackVariableReachability { + UnguardedNullReturnDereferenceReachability() { this = "UnguardedNullReturnDereference" } + + override predicate isSource(ControlFlowNode node, StackVariable v) { + possiblyNullAssignment(node, _, v) + } + + override predicate isSink(ControlFlowNode node, StackVariable v) { + node instanceof Dereference and + node = v.getAnAccess() and + not node.(Dereference) + .getDereferencingOperation() + .(FunctionCall) + .getTarget() + .hasGlobalName("free") + } + + override predicate isBarrier(ControlFlowNode node, StackVariable v) { + // Variable is redefined + definitionBarrier(v, node) + or + // Only report the first dereference on any path + isSink(node, v) + or + // Any indication that this variable is checked + nullCheckExpr(node, v) + or + validCheckExpr(node, v) + or + exists(VariableAccess va | + va = node and + va = v.getAnAccess() + | + va = + any(EqualityOperation o | + o.getAnOperand() instanceof NullValue + ).getAnOperand() or + va = any(IfStmt i).getCondition() or + va.getParent() instanceof LogicalAndExpr or + va.getParent() instanceof NotExpr or + va.getParent() = any(ConditionalExpr c).getCondition() + // TODO: Handle asserts? + ) + or + // Eliminating FP - the variable is initialized within the `IfStmt` + exists(IfStmt ifs, Initializer init | + ifs = node.getEnclosingStmt() and + v = init.getDeclaration() and + ifs = init.getEnclosingStmt() + ) + or + // Elminating FP - the variable is assigned within the `IfStmt` + exists(IfStmt ifs, AssignExpr ae | + v.getAnAssignment() = ae and + ifs.getAChild*() = node + | + ae.getEnclosingStmt() = ifs + ) + or + // TODO: this is a workaround to address the inability to detect when a pointer check guards an exit condition. + // Currently using specific check function names, but this should be replaced with a more general mechanism. + // We are currently working on a generic query but it is on hold. + exists(Call c | + c.getTarget().getName() = ["_checked_malloc_impl", "_checked_realloc_impl", "_checked_calloc_impl", + "_checked_pointer_impl", "_fail_on_unexpected_null_pointer", "_fail_on_memory_op"] + and c.getAnArgument().getAChild*() = v.getAnAccess() + and c = node) + } +} + +from + UnguardedNullReturnDereferenceReachability r, StackVariable v, ControlFlowNode source, Call call, + Expr use +where + r.reaches(source, v, use) and + possiblyNullAssignment(source, call, v) +select use, + "In " + use.getEnclosingFunction() + " result of $@ to $@ is dereferenced here and may be null.", + call, "call", call.getTarget(), call.getTarget().toString() diff --git a/src/microsoft/Likely Bugs/UnguardedNullReturnDereferenceBad.c b/src/microsoft/Likely Bugs/UnguardedNullReturnDereferenceBad.c new file mode 100644 index 00000000..8b4acd9c --- /dev/null +++ b/src/microsoft/Likely Bugs/UnguardedNullReturnDereferenceBad.c @@ -0,0 +1,34 @@ +#define NULL (0) +typedef void * PVOID; +typedef enum _POOL_TYPE { + NonPagedPool + // ... +} POOL_TYPE; +typedef int SIZE_T; +typedef unsigned long ULONG; + +PVOID ExAllocatePoolWithTag(POOL_TYPE, SIZE_T, ULONG); + +void use(char* data); + +void free(char* data); + +char* malloc(int size); + +void test(bool b) { + char* data; + data = (char*) ExAllocatePoolWithTag(NonPagedPool, sizeof(*data), 'tag'); + // BAD: 'data' is not null-checked + use(data); + + char* data2 = (char*) ExAllocatePoolWithTag(NonPagedPool, sizeof(*data2), 'tag'); + // BAD: 'data2' is not null-checked + use(data2); + + if (b) + { + // BAD + char* data6 = (char*)malloc(12); + use(data6); + } +} diff --git a/src/microsoft/Likely Bugs/UnguardedNullReturnDereferenceGood.c b/src/microsoft/Likely Bugs/UnguardedNullReturnDereferenceGood.c new file mode 100644 index 00000000..5798c4f5 --- /dev/null +++ b/src/microsoft/Likely Bugs/UnguardedNullReturnDereferenceGood.c @@ -0,0 +1,46 @@ +#define NULL (0) +typedef void * PVOID; +typedef enum _POOL_TYPE { + NonPagedPool + // ... +} POOL_TYPE; +typedef int SIZE_T; +typedef unsigned long ULONG; + +PVOID ExAllocatePoolWithTag(POOL_TYPE, SIZE_T, ULONG); + +void use(char* data); + +void free(char* data); + +char* malloc(int size); + +void test(bool b) { + char* data; + + data = (char*) ExAllocatePoolWithTag(NonPagedPool, sizeof(*data), 'tag'); + if (data != NULL) + // GOOD: 'data' is null-checked + use(data); + + char* data3 = (char*) ExAllocatePoolWithTag(NonPagedPool, sizeof(*data3), 'tag'); + if (data3 != NULL) + // GOOD: 'data' is null-checked + use(data3); + + char *data4 = malloc(12); + // GOOD: 'free' is null-safe + free(data4); + + // GOOD + if (char* data5 = (char*)malloc(12)) + { + use(data5); + } + + // GOOD + if (data4 = (char*)malloc(12)) + { + use(data4); + } +} diff --git a/src/microsoft/Likely Bugs/UninitializedPtrField.ql b/src/microsoft/Likely Bugs/UninitializedPtrField.ql index af48b37d..3e959fa3 100644 --- a/src/microsoft/Likely Bugs/UninitializedPtrField.ql +++ b/src/microsoft/Likely Bugs/UninitializedPtrField.ql @@ -1,44 +1,45 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT license. - /** - * @id cpp/uninitializedptrfield + * @id cpp/microsoft/public/likely-bugs/uninitializedptrfield * @name Dereference of potentially uninitialized pointer field - * @description A pointer field which was not initialized during or since class + * @description A pointer field which was not initialized during or since class * construction will cause a null pointer dereference. * @kind problem * @problem.severity warning * @tags security * external/cwe/cwe-476 * @opaqueid SM02310 + * @owner.email sdat@microsoft.com * @microsoft.severity Important */ - + import cpp import semmle.code.cpp.controlflow.StackVariableReachability +import semmle.code.cpp.valuenumbering.HashCons /* * Utilities */ - + /** * Go through user-specified implicit conversions. It's a bug that the library * doesn't handle this */ Expr afterUserConversions(Expr source) { - if result.(Call).getTarget() instanceof ImplicitConversionFunction - then result.(Call).getQualifier() = afterUserConversions(source) - else result = source + if result.(Call).getTarget() instanceof ImplicitConversionFunction + then result.(Call).getQualifier() = afterUserConversions(source) + else result = source } /** * An expression that may be returned by f. */ Expr mayReturn(Function f) { - exists(ReturnStmt s | - s.getExpr() = result and - s.getEnclosingFunction() = f - ) + exists(ReturnStmt s | + s.getExpr() = result and + s.getEnclosingFunction() = f + ) } /** @@ -46,10 +47,10 @@ Expr mayReturn(Function f) { */ class OverloadedAddressOfExpr extends FunctionCall { OverloadedAddressOfExpr() { - getTarget().hasName("operator&") - and getTarget().getEffectiveNumberOfParameters() = 1 + getTarget().hasName("operator&") and + getTarget().getEffectiveNumberOfParameters() = 1 } - + /** * Gets the expression this operator & applies to. */ @@ -63,99 +64,160 @@ class OverloadedAddressOfExpr extends FunctionCall { * An address of v. */ Expr addressOf(Expr value) { - exists(Expr e | result = afterUserConversions(e) | - e.(AddressOfExpr).getOperand() = value - or - e.(OverloadedAddressOfExpr).getExpr() = value - ) + exists(Expr e | result = afterUserConversions(e) | + e.(AddressOfExpr).getOperand() = value + or + e.(OverloadedAddressOfExpr).getExpr() = value + ) } Field assignedNonNull(Function f) { - exists(Function sub, Assignment a, Expr init | - f.calls*(sub) and - a.getEnclosingFunction() = sub and - a.getLValue() = result.getAnAccess() and - a.getRValue() = init | - not init instanceof NullValue - ) + exists(Function sub, Assignment a, Expr init | + f.calls*(sub) and + a.getEnclosingFunction() = sub and + a.getLValue() = result.getAnAccess() and + a.getRValue() = init + | + not init instanceof NullValue + ) } /* * Main analysis */ - + /** - * A constructor which does not initialize the given field. + * A constructor which does not initialize the given pointer field. */ Constructor unsafeConstructor(Field uninitialized) { - // defined in the same type - result.getDeclaringType() = uninitialized.getDeclaringType() - // all initializations in transitively called functions are null - and not uninitialized = assignedNonNull(result) - // all constructor initializations are null - and forall(ConstructorFieldInit i | - result.getAnInitializer() = i and i.getTarget() = uninitialized | - i.getExpr() instanceof NullValue - ) + // If no definition for the constructor, we can either assume dangerous or safe + // to avoid false positives, we err on the side of assuming safe at least for + // SDL-required queries + result.hasDefinition() and + // defined in the same type + result.getDeclaringType() = uninitialized.getDeclaringType() and + // all initializations in transitively called functions are null + not uninitialized = assignedNonNull(result) and + // all constructor initializations are null + forall(ConstructorFieldInit i | result.getAnInitializer() = i and i.getTarget() = uninitialized | + i.getExpr() instanceof NullValue + ) and + uninitialized.getUnderlyingType() instanceof PointerType } predicate fieldDeref(Field f, Expr qualifier, Expr deref) { - exists(Expr ptrExpr | - // the expression corresponding to the pointer is... - ( - // either a call to a method that may return the field - exists(FunctionCall c | c = ptrExpr | - mayReturn(c.getTarget()).(FieldAccess).getTarget() = f - and qualifier = c.getQualifier() - ) - or - // or a direct access to the field - exists(FieldAccess fa | fa = ptrExpr | - fa.getTarget() = f - and qualifier = fa.getQualifier() - ) - ) - and - // the pointer expression is dereferenced by deref - dereferencedByOperation(deref, ptrExpr) - ) + exists(Expr ptrExpr | + // the expression corresponding to the pointer is... + ( + // either a call to a method that may return the field + exists(FunctionCall c | c = ptrExpr | + mayReturn(c.getTarget()).(FieldAccess).getTarget() = f and + qualifier = c.getQualifier() + ) + or + // or a direct access to the field + exists(FieldAccess fa | fa = ptrExpr | + fa.getTarget() = f and + qualifier = fa.getQualifier() + ) + ) and + // the pointer expression is dereferenced by deref + dereferencedByOperation(deref, ptrExpr) + ) } -class UninitializedFieldReachability extends StackVariableReachability { - UninitializedFieldReachability() { - this = "UninitializedFieldReachability" - } - - override predicate isSource(ControlFlowNode node, StackVariable v) { - definition(v, node) - and node = any(Class t | | t.getAConstructor().getACallToThisFunction()) - } - - override predicate isSink(ControlFlowNode node, StackVariable v) { - node = v.getAnAccess() - } - - override predicate isBarrier(ControlFlowNode node, StackVariable v) { - exists(Call c | c = node | - // methods can modify fields - c.getQualifier() = v.getAnAccess() - or - // if we pass in a reference then assume that anything can happen - c.getAnArgument() = addressOf(v.getAnAccess()) - or - // similarly for references to an actual field - c.getAnArgument() = addressOf(any(FieldAccess f | f.getQualifier() = v.getAnAccess())) - ) - or - node.(Assignment).getLValue().(FieldAccess).getQualifier() = v.getAnAccess() - } +Expr breakExpr(Expr e) { + if e instanceof BinaryLogicalOperation + then result = breakExpr(e.(BinaryLogicalOperation).getAnOperand()) + else + if e instanceof UnaryLogicalOperation + then result = breakExpr(e.(UnaryLogicalOperation).getAnOperand()) + else result = e +} + +/** + * Evaluates for any expression `e`, if `cond` is a condition that encloses `e`. + */ +bindingset[e] +Expr getEnclosingCondition(Expr e) { + exists(IfStmt i | i.getCondition() = result | + i.getChildStmt().getAChild*() = e.getEnclosingStmt() + ) + or + exists(Loop l | l.getCondition() = result | l.getChildStmt().getAChild*() = e.getEnclosingStmt()) +} + +predicate isInitializer(ControlFlowNode node, StackVariable v) { + exists(Call c | c = node | + // methods can modify fields + c.getQualifier() = v.getAnAccess() + or + // if we pass in a reference then assume that anything can happen + c.getAnArgument() = addressOf(v.getAnAccess()) + or + // Including if pass a reference implicitly (call by reference) + exists(Expr arg | arg = c.getAnArgument() and arg = v.getAnAccess() | + arg.getFullyConverted() instanceof ReferenceToExpr + ) + or + // similarly for references to an actual field + c.getAnArgument() = addressOf(any(FieldAccess f | f.getQualifier() = v.getAnAccess())) + ) + or + // FALSE NEGATIVE CAVEAT: any field access assignment is a barrier under this logic, leading to false negatives + node.(Assignment).getLValue().(FieldAccess).getQualifier() = v.getAnAccess() } +/** + * Holds if for a given `node` representing a variable access of `v`, + * if there is a predecessory ControlFlowNode that initializes fields + * of `v` under the same/similar guarding conditions as `node`. + */ +bindingset[node, v] +predicate hasCheckedInitialize(ControlFlowNode node, StackVariable v) { + node = v.getAnAccess() and + exists(ControlFlowNode pred | + //TODO: found I could not enforce a successor relationship of pred + // cases exist in real-world code that were still false positives + // As such, query is extra conservative for now + // pred.getASuccessor+() = node | + isInitializer(pred, v) and + exists(Expr cond1, Expr cond2 | + cond1 = getEnclosingCondition(node) and + cond2 = getEnclosingCondition(pred) and + hashCons(breakExpr(cond1)) = hashCons(breakExpr(cond2)) and + // Explicitly looking for the conditions to not be the same + // exact expression in this case + cond1 != cond2 + ) + ) +} + +class UninitializedFieldReachability extends StackVariableReachability { + UninitializedFieldReachability() { this = "UninitializedFieldReachability" } + + override predicate isSource(ControlFlowNode node, StackVariable v) { + definition(v, node) and + node = unsafeConstructor(_).getACallToThisFunction() and + node = any(Class t | | t.getAConstructor().getACallToThisFunction()) + } + + override predicate isSink(ControlFlowNode node, StackVariable v) { + node = v.getAnAccess() and + // There must not exist an initialing expression anywhere prior to the sink + // where the initialilzation is performed in a guarding condition + // that appears to be related to the guarding condition of the sink + // e.g., if(A) initialize ... if(A) use + not hasCheckedInitialize(node, v) + } + + override predicate isBarrier(ControlFlowNode node, StackVariable v) { isInitializer(node, v) } +} from Variable v, Field f, Expr def, Expr use, Expr deref -where - any(UninitializedFieldReachability r).reaches(def, v, use) - and def = unsafeConstructor(f).getACallToThisFunction() - and f.getType() instanceof PointerType - and fieldDeref(f, use, deref) -select deref, "Dereference of $@ which may not have been initialized since $@.", f, "field", def, "construction" +where + any(UninitializedFieldReachability r).reaches(def, v, use) and + def = unsafeConstructor(f).getACallToThisFunction() and + fieldDeref(f, use, deref) +select deref, "Dereference of $@ which may not have been initialized since $@.", f, "field", def, + "construction" diff --git a/src/microsoft/code/cpp/public/NestedFields.qll b/src/microsoft/code/cpp/public/NestedFields.qll new file mode 100644 index 00000000..3183cef4 --- /dev/null +++ b/src/microsoft/code/cpp/public/NestedFields.qll @@ -0,0 +1,43 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +import cpp + +/** + * Gets a `Field` that is nested within the given `Struct`. + * + * This identifies `Field`s which are located in the same memory + */ +private Field getANestedField(Struct s) { + result = s.getAField() or + exists(NestedStruct ns | + s = ns.getDeclaringType() and + result = getANestedField(ns) + ) +} + +/** + * Unwraps a series of field accesses to determine the inner-most qualifier. + */ +private Expr getUltimateQualifier(FieldAccess fa) { + exists(Expr qualifier | + qualifier = fa.getQualifier() | + result = getUltimateQualifier(qualifier) or + not qualifier instanceof FieldAccess and result = qualifier + ) +} + +/** + * Accesses to nested fields. + */ +class NestedFieldAccess extends FieldAccess { + Expr ultimateQualifier; + NestedFieldAccess() { + ultimateQualifier = getUltimateQualifier(this) and + getTarget() = getANestedField(ultimateQualifier.getType().stripType()) + } + + /** Gets the ultimate qualifier of this nested field access. */ + Expr getUltimateQualifier() { + result = ultimateQualifier + } +} diff --git a/src/microsoft/code/cpp/public/commons/Literals.qll b/src/microsoft/code/cpp/public/commons/Literals.qll new file mode 100644 index 00000000..4c7d6a0e --- /dev/null +++ b/src/microsoft/code/cpp/public/commons/Literals.qll @@ -0,0 +1,88 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** + * Provides classes and predicates for identifying variables and expression that have a static + * value. + */ +import cpp + +/** + * A local variable which is only assigned literal values. + */ +class LiteralLocalVariable extends LocalVariable { + LiteralLocalVariable() { + // All assigned values are constant, or accesses to literal local variables + forex(Expr assigned | + assigned = this.getAnAssignedValue() | + assigned.isConstant() + ) and + // No sneaky assignments + not exists(CrementOperation co | co.getOperand() = this.getAnAccess()) and + not exists(AddressOfExpr ao | ao.getOperand() = this.getAnAccess()) and + not exists(AssignOperation ae | ae.getLValue() = this.getAnAccess()) + } + + /** + * Gets a literal expr assigned to this variable + */ + Expr getALiteralExpr() { + result = getAnAssignedValue() + } +} + +/** + * An access of a variable which is only assigned literal values. + */ +class LiteralAccess extends VariableAccess { + LiteralAccess() { + getTarget() instanceof LiteralLocalVariable + } + + /** + * Gets an expr representing one of the literal values. + */ + Expr getALiteralExpr() { + exists(Expr def, Assignment a | + def = getTarget().(LiteralLocalVariable).getALiteralExpr() and + definitionUsePair(_, a, this) and + a.getRValue() = def | + def.isConstant() and result = def + ) + } + + /** + * Gets a string representing one of the possible literal values for this access. + */ + string getALiteralValue() { + result = getALiteralExpr().getValue() + } + + /** + * Gets an int representing one of the possible literal values for this access. + */ + int getALiteralInt() { + result = getALiteralValue().toInt() + } +} + +/** + * An expression whose purpose is to find the offset of a field. + * + * This expression has this structure `&(0->field)` + */ +class OffsetOf extends AddressOfExpr { + OffsetOf() { + getAnOperand().(FieldAccess).getQualifier().getValue() = "0" + } +} + +/** + * A value that is statically assigned at compile time. + */ +class StaticValue extends Expr { + StaticValue() { + this instanceof Literal or + this instanceof SizeofOperator or + this instanceof OffsetOf + } +} diff --git a/src/microsoft/code/cpp/public/commons/MemoryAllocation.qll b/src/microsoft/code/cpp/public/commons/MemoryAllocation.qll new file mode 100644 index 00000000..44a1ec30 --- /dev/null +++ b/src/microsoft/code/cpp/public/commons/MemoryAllocation.qll @@ -0,0 +1,67 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** + * Provides classes for identifying expressions that allocate memory. + */ +import cpp + +/** + * An allocation of either stack or heap memory. + */ +abstract class Allocation extends Expr { } + +/** + * An allocation on the stack. + */ +class StackAllocation extends Allocation { + StackAllocation() { + exists(StackVariable var | + var.getType().getUnspecifiedType() instanceof Struct | + var.getInitializer().getExpr() = this + ) + } + + /** Gets the variable which points to the allocated memory. */ + StackVariable getAllocationVariable() { + result.getInitializer().getExpr() = this + } +} + +/** + * An allocation on the heap. + */ +abstract class HeapAllocation extends Allocation, FunctionCall { + /** + * Gets the expression representing the allocation size in this call. + */ + abstract Expr getAllocatedSize(); + override string toString() { result = FunctionCall.super.toString() } +} + +/** + * A heap allocation using an `ExAllocatePool*` function. + */ +class ExAllocatePoolAllocation extends HeapAllocation { + ExAllocatePoolAllocation() { + this.getTarget().getName().matches("ExAllocatePool%") + } + + override + Expr getAllocatedSize() { + result = this.getArgument(1) + } +} + +/** + * A heap allocation using `malloc`. + */ +class Malloc extends HeapAllocation { + Malloc() { + this.getTarget().getName().matches("malloc%") + } + + override + Expr getAllocatedSize() { + result = this.getArgument(0) + } +} diff --git a/src/microsoft/code/cpp/public/controlflow/Dereferences.qll b/src/microsoft/code/cpp/public/controlflow/Dereferences.qll new file mode 100644 index 00000000..2e159e90 --- /dev/null +++ b/src/microsoft/code/cpp/public/controlflow/Dereferences.qll @@ -0,0 +1,111 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** + * Provides classes and predicates for identifying expressions that are dereferences of a pointer. + * + * This library detects dereferences that occur locally - for example, in a dereferencing + * expression, or as an argument to a library call such as memcpy - as well as dereferences that + * occur by passing pointer arguments to calls that ultimately dereference the argument. + */ +import cpp +import semmle.code.cpp.controlflow.Dereferenced + +/** + * A pointer expression `e` that is dereferenced directly by `op`. + */ +predicate localDereference(Expr op, Expr e) { + e.getActualType().getUnspecifiedType() instanceof PointerType and + not op = any(SizeofExprOperator sizeof).getAChild*() and( + e = op.(PointerDereferenceExpr).getOperand() or + e = op.(FieldAccess).getQualifier() or + e = op.(Call).getQualifier() or + e = op.(ArrayExpr).getArrayBase() or + e = op.(ArrayExpr).getArrayOffset() or + localDereference(op, any(PointerArithmeticOperation pOp | e = pOp.getAnOperand())) or + localDereference(op, any(ConditionalExpr c | e = c.getThen() or e = c.getElse())) or + exists(int i | op.(Call).getArgument(i) = e | + callDereferences(op, i) + ) + ) +} + +/** + * A pointer expression `e` that is dereferenced directly or indirectly by `op`. + * + * Indirect dereferences occur when pointer arguments are passed to functions which ultimately + * dereference the argument (all calls by value to library functions are assumed to dereference + * their argument). + */ +predicate opDereferences(Expr op, Expr e) { + not op = any(SizeofExprOperator sizeof).getAChild*() and( + localDereference(op, e) or + exists(Call call, int i | + call = op and + not call.passesByReference(i, e) and + call.getArgument(i) = e | + indirectDereference(call, e) + or + not call.getTarget().hasEntryPoint() // library call + ) + ) + or + dereferencedByOperation(op, e) +} + +predicate indirectDereference(Call call, Expr e) { + not call = any(SizeofExprOperator sizeof).getAChild*() and( + exists(int i | + not call.passesByReference(i, e) and + call.getArgument(i) = e | + exists(Expr paramUse | parameterUsePair(call.getTarget().getParameter(i), paramUse) | + opDereferences(_, paramUse) or + exists(Expr step | definitionUsePair(_, paramUse, step) and opDereferences(_, step)) + ) + ) + ) +} + +/** + * A pointer expression that is dereferenced, directly or indirectly. + * Indirect dereferences occur when pointer arguments are passed to functions which ultimately + * dereference the argument. + */ +class Dereference extends Expr { + Dereference() { + opDereferences(_, this) + } + + /** Gets the operation that is performing the dereference of this pointer expression. */ + Expr getDereferencingOperation() { + opDereferences(result, this) + } +} + +/** + * A pointer expression that is dereferenced indirectly within this function. + */ +class IndirectDereference extends Dereference { + IndirectDereference() { indirectDereference(_, this) } + + override Expr getDereferencingOperation() { + indirectDereference(result, this) + } +} + +/** + * A pointer expression that is dereferenced directly within this function. + */ +class DirectDereference extends Dereference { + DirectDereference() { not this instanceof IndirectDereference } +} + +/** + * A pointer expression that is dereferenced directly within this function. + */ +class LocalDereference extends DirectDereference { + LocalDereference() { localDereference(_, this) } + + override Expr getDereferencingOperation() { + localDereference(result, this) + } +} diff --git a/src/microsoft/code/cpp/public/controlflow/Reachability.qll b/src/microsoft/code/cpp/public/controlflow/Reachability.qll new file mode 100644 index 00000000..c143a415 --- /dev/null +++ b/src/microsoft/code/cpp/public/controlflow/Reachability.qll @@ -0,0 +1,14 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** + * Provides a reachability predicate API. + */ +import cpp + +/** + * Holds if `a` has `b` as a successor. + */ +cached +predicate reaches(ControlFlowNode a, ControlFlowNode b) { + b = a.getASuccessor+() +} diff --git a/src/microsoft/code/cpp/public/dispatch/VirtualDispatch.qll b/src/microsoft/code/cpp/public/dispatch/VirtualDispatch.qll new file mode 100644 index 00000000..62d3a1f5 --- /dev/null +++ b/src/microsoft/code/cpp/public/dispatch/VirtualDispatch.qll @@ -0,0 +1,82 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +import cpp + +/** + * A module for performing simple virtual dispatch analysis. + */ +module VirtualDispatch { + /** + * Gets a possible implementation target when the given function is the static target of a virtual call. + */ + private MemberFunction getAPossibleImplementation(MemberFunction staticTarget) { + /* + * `IUnknown` is a COM interface with many sub-types, and many overrides (tens of thousands on + * some databases), so we ignore any member functions defined within that interface. + */ + not staticTarget.getDeclaringType().hasName("IUnknown") and + result = staticTarget.getAnOverridingFunction*() + } + + /** Gets the static type of the qualifier expression for the given call. */ + private Class getCallQualifierType(FunctionCall c) { + result = c.getQualifier().getType().stripType() and + /* + * `IUnknown` is a COM interface with many sub-types (tens of thousands on some databases), so + * we ignore any cases where the qualifier type is that interface. + */ + not result.hasName("IUnknown") + } + + /** + * Gets a viable target for the given function call. + * + * If `c` is a virtual call, then we will perform a simple virtual dispatch analysis to return + * the `Function` instances which might be a viable target, based on an analysis of the declared + * type of the qualifier expression. + * + * (This analysis is imprecise: it looks for subtypes of the declared type of the qualifier expression + * and the possible implementations of `c.getTarget()` that are declared or inherited by those subtypes. + * This does not account for virtual inheritance and the ways this affects dispatch.) + * + * If `c` is not a virtual call, the result will be `c.getTarget()`. + */ + Function getAViableTarget(Call c) { + exists(Function staticTarget | staticTarget = c.getTarget() | + if c.(FunctionCall).isVirtual() and staticTarget instanceof MemberFunction + then exists(Class qualifierType, Class qualifierSubType | + result = getAPossibleImplementation(staticTarget) and + qualifierType = getCallQualifierType(c) and + qualifierType = qualifierSubType.getABaseClass*() and + mayInherit(qualifierSubType, result) and + not cannotInherit(qualifierSubType, result) + ) + else result = staticTarget + ) + } + + /** Holds if `f` is declared in `c` or a transitive base class of `c`. */ + private predicate mayInherit(Class c, MemberFunction f) { + f.getDeclaringType() = c.getABaseClass*() + } + + /** + * Holds if `c` cannot inherit the member function `f`, + * i.e. `c` or one of its supertypes overrides `f`. + */ + private predicate cannotInherit(Class c, MemberFunction f) { + exists(Class overridingType, MemberFunction override | + cannotInheritHelper(c, f, overridingType, override) and + override.overrides+(f) + ) + } + + pragma[nomagic] + private predicate cannotInheritHelper( + Class c, MemberFunction f, + Class overridingType, MemberFunction override) { + c.getABaseClass*() = overridingType and + override.getDeclaringType() = overridingType and + overridingType.getABaseClass+() = f.getDeclaringType() + } +} diff --git a/src/microsoft/code/cpp/public/windows/kernel/IRP.qll b/src/microsoft/code/cpp/public/windows/kernel/IRP.qll new file mode 100644 index 00000000..f4ebe9d0 --- /dev/null +++ b/src/microsoft/code/cpp/public/windows/kernel/IRP.qll @@ -0,0 +1,228 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** + * Provides classes relevant for IRP. + */ + +module IRP { + private import cpp as CPP + private import semmle.code.cpp.dataflow.new.DataFlow::DataFlow as DataFlow + private import semmle.code.cpp.controlflow.DefinitionsAndUses as DefUse + + /** An `IRP` parameter. */ + class Parameter extends CPP::Parameter { + Parameter() { + exists(CPP::PointerType pt | + pt = this.getType().getUnspecifiedType() and + pt.getBaseType().(CPP::Struct).hasName("_IRP") + ) + } + + /** Gets a use of this `IRP` parameter. */ + CPP::VariableAccess getAUse() { + DefUse::parameterUsePair(this, result) + } + + private DeviceIoControlAccess getADeviceIoControlAccess() { + this = result.getParameter() + } + + private CPP::FieldAccess getADeviceIoControlQualifiedAccess() { + getASource(result.getQualifier()) = this.getADeviceIoControlAccess() + } + + /** + * Gets an access to the input buffer length of this `IRP` parameter, for + * example + * + * ``` + * Stack = IoGetCurrentIrpStackLocation(Irp); + * ... Stack->Parameters.DeviceIoControl.InputBufferLength ... + * ``` + */ + CPP::FieldAccess getAnInputBufferLengthAccess() { + result = this.getADeviceIoControlQualifiedAccess() + and result.getTarget().hasName("InputBufferLength") + } + + /** + * Gets an access to the output buffer length of this `IRP` parameter, for + * example + * + * ``` + * Stack = IoGetCurrentIrpStackLocation(Irp); + * ... Stack->Parameters.DeviceIoControl.OutputBufferLength ... + * ``` + */ + CPP::FieldAccess getAnOutputBufferLengthAccess() { + result = this.getADeviceIoControlQualifiedAccess() + and result.getTarget().hasName("OutputBufferLength") + } + } + + /** + * A field access using an `IRP` parameter. + */ + abstract class FieldAccess extends CPP::FieldAccess { + /** Gets the `IRP` parameter belonging to this access. */ + abstract Parameter getParameter(); + } + + /** Gets a possible source for expression `e`. */ + private CPP::Expr getASource(CPP::Expr e) { + DataFlow::localFlow(DataFlow::exprNode(result), DataFlow::exprNode(e)) + } + + /** + * An access to an `IRP` user buffer, for example + * + * ``` + * p->UserBuffer + * ``` + */ + class UserBufferAccess extends FieldAccess { + UserBufferAccess() { + userBufferAcces(_, this) + } + + override Parameter getParameter() { + userBufferAcces(result, this) + } + } + + private predicate userBufferAcces(Parameter p, CPP::FieldAccess fa) { + p.getAUse() = fa.getQualifier() and + fa.getTarget().hasName("UserBuffer") + } + + /** + * A call to `IoGetCurrentIrpStackLocation()`. + */ + class IoGetCurrentIrpStackLocationCall extends CPP::FunctionCall { + IoGetCurrentIrpStackLocationCall() { + ioGetCurrentIrpStackLocationCall(_, this) + } + + Parameter getParameter() { + ioGetCurrentIrpStackLocationCall(result, this) + } + } + + private predicate ioGetCurrentIrpStackLocationCall(Parameter p, CPP::FunctionCall fc) { + fc.getAnArgument() = p.getAnAccess() and + fc.getTarget().hasName("IoGetCurrentIrpStackLocation") + } + + /** + * An access to an `IRP` device IO control, for example + * + * ``` + * IrpStack = IoGetCurrentIrpStackLocation(Irp); + * ... IrpStack->Parameters.DeviceIoControl ... + * ``` + */ + class DeviceIoControlAccess extends FieldAccess { + DeviceIoControlAccess() { + deviceIoControlAccess(_, this) + } + + override Parameter getParameter() { + deviceIoControlAccess(result, this) + } + } + + private predicate deviceIoControlAccess(Parameter p, CPP::FieldAccess fa) { + exists(IoGetCurrentIrpStackLocationCall fc, CPP::FieldAccess fa1 | + fc.getParameter() = p and + getASource(fa1.getQualifier()) = fc and + fa1.getTarget().hasName("Parameters") and + getASource(fa.getQualifier()) = fa1 and + fa.getTarget().hasName("DeviceIoControl") + ) + } + + /** + * An access to an `IRP` system buffer, for example + * + * ``` + * Irp->AssociatedIrp.SystemBuffer + * ``` + */ + class SystemBufferAccess extends FieldAccess { + SystemBufferAccess() { + systemBufferAccess(_, this) + } + + override Parameter getParameter() { + systemBufferAccess(result, this) + } + } + + private predicate systemBufferAccess(Parameter p, CPP::FieldAccess fa) { + exists(CPP::FieldAccess qual | + p.getAUse() = qual.getQualifier() and + qual.getTarget().hasName("AssociatedIrp") and + fa.getQualifier() = getASource(qual) and + fa.getTarget().hasName("SystemBuffer") + ) + } + + /** + * An access to an `IRP` type 3 input buffer, for example + * + * ``` + * IrpStack = IoGetCurrentIrpStackLocation(Irp); + * ... IrpStack->Parameters.DeviceIoControl.Type3InputBuffer ... + * ``` + */ + class Type3InputBufferAccess extends FieldAccess { + Type3InputBufferAccess() { + exists(DeviceIoControlAccess a | + getASource(this.getQualifier()) = a and + this.getTarget().hasName("Type3InputBuffer") + ) + } + + override Parameter getParameter() { + deviceIoControlAccess(result, this.getQualifier()) + } + } + + /** + * An access to the input buffer length of an `IRP` device IO control, for example + * + * ``` + * IrpStack = IoGetCurrentIrpStackLocation(Irp); + * ... IrpStack->Parameters.DeviceIoControl.InputBufferLength ... + * ``` + */ + class InputBufferLengthAccess extends FieldAccess { + private Parameter parameter; + InputBufferLengthAccess() { + this = parameter.getAnInputBufferLengthAccess() + } + + override Parameter getParameter() { + result = parameter + } + } + + /** + * An access to the output buffer length of an `IRP` device IO control, for example + * + * ``` + * IrpStack = IoGetCurrentIrpStackLocation(Irp); + * ... IrpStack->Parameters.DeviceIoControl.OutputBufferLength ... + * ``` + */ + class OutputBufferLengthAccess extends FieldAccess { + private Parameter parameter; + OutputBufferLengthAccess() { + this = parameter.getAnOutputBufferLengthAccess() + } + + override Parameter getParameter() { + result = parameter + } + } +} diff --git a/src/microsoft/code/cpp/public/windows/kernel/KernelMode.qll b/src/microsoft/code/cpp/public/windows/kernel/KernelMode.qll new file mode 100644 index 00000000..0a0d98e6 --- /dev/null +++ b/src/microsoft/code/cpp/public/windows/kernel/KernelMode.qll @@ -0,0 +1,154 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** + * Provides classes and predicates for Windows kernel mode features. + */ +import cpp +import microsoft.code.cpp.public.controlflow.Dereferences + +/** + * A call that frees memory. + */ +class FreeCall extends FunctionCall { + FreeCall() { + exists(string name | + name = this.getTarget().getName() | + name.matches("ExFree%") or + name.matches("delete%") + ) + } + + /** Gets the access for the variable that is freed. */ + VariableAccess getFreedVariableAccess() { + result = getArgument(0) + } +} + +/** + * An access within a Probe call. + */ +class ProbeCallAccess extends VariableAccess { + ProbeCallAccess() { + this = any(MacroInvocation e | e.getMacroName().matches("%Probe%")).getAnExpandedElement() or + this = any(Call c | c.getTarget().getName().matches("%Probe%")).getAnArgument().getAChild*() + } +} + +/** + * An access within a user-mode accessor. + */ +class UMAAccess extends VariableAccess { + UMAAccess() { + this = any(MacroInvocation e | e.getMacro().getFile().getBaseName().matches("usermode_accessors.h")).getAnExpandedElement() or + this = any(Call c | c.getTarget().getFile().getBaseName().matches("usermode_accessors.h")).getAnArgument().getAChild*() + } +} + +/** + * Holds if `e` occurs in a branch of a conditional that implies that it is not in user mode. + */ +predicate inKernelModeIfStmt(Expr e) { + exists(IfStmt ifStmt, EqualityOperation eq | + eq = ifStmt.getCondition() and + eq.getAnOperand().(EnumConstantAccess).getTarget().hasName("UserMode") | + if eq instanceof EQExpr then + ifStmt.getElse() = e.getEnclosingStmt().getParent+() + else + ifStmt.getThen() = e.getEnclosingStmt().getParent+() + ) or + exists(IfStmt ifStmt, EqualityOperation eq | + eq = ifStmt.getCondition() and + eq.getAnOperand().(EnumConstantAccess).getTarget().hasName("KernelMode") | + if eq instanceof EQExpr then + ifStmt.getThen() = e.getEnclosingStmt().getParent+() + else + ifStmt.getElse() = e.getEnclosingStmt().getParent+() + ) +} + +/** Holds if `va` occurs within a Microsoft style try-except block. */ +predicate isInMicrosoftTry(VariableAccess va) { + exists(MicrosoftTryStmt ts | + ts.getStmt().getAChild+() = va.getEnclosingStmt() + ) +} + +/* + * Memory operations. + */ + +/** A call to memset. */ +class Memset extends FunctionCall { + Memset() { + getTarget().hasName("memset") or + getTarget().hasName("RtlSetUserMemory") or + getTarget().hasName("RtlFillVolatileMemory") or + getTarget().hasName("RtlSetVolatileMemory") + } + + /** Gets the expr describing the target of the write. */ + Expr getWriteTargetExpr() { + result = getArgument(0) + } +} + +/** A call to a function that copies memory from one buffer to another. */ +class MemoryCopy extends FunctionCall { + MemoryCopy() { + getTarget().hasName("memcpy") or + getTarget().hasName("ProbeAndReadBuffer") or + getTarget().hasName("RtlRead%FromUser") or // UMA read + getTarget().hasName("RtlCopyFromUser%") or // UMA copy + getTarget().hasName("RtlCopyToUser%") or // UMA copy + getTarget().hasName("RtlWrite%ToUser%") // UMA copy + } + + /** Gets the expr describing the target of the copy. */ + Expr getCopyTargetExpr() { + result = getArgument(0) + } + + /** Gets the expr describing the source of the copy. */ + Expr getCopySourceExpr() { + result = getArgument(1) + } + + /** Gets the variable which is the target of the copy, if any. */ + Variable getCopyTargetVariable() { + result = getCopyTargetExpr().(VariableAccess).getTarget() or + result = getCopyTargetExpr().(AddressOfExpr).getAddressable() + } +} + +/** + * Is this dereference in a read or write. + */ +predicate isInMemReadWriteCall(Dereference va) { + exists(FunctionCall fc | + fc.getAnArgument() = va | + fc.getTarget().getName().matches("%Probe%Write%") or + fc.getTarget().getName().matches("%Probe%Read%") + ) +} + +/** + * Whether `e` is the target location of a write operation. + */ +predicate isWriteExprTarget(Expr e) { + e = any(Assignment a).getLValue().getAChild*() or + e = any(FunctionCall fc | fc.getTarget().getName().matches("%Probe%Write%")).getAnArgument().getAChild*() or + e = any(Memset m).getWriteTargetExpr().getAChild*() or + e = any(MemoryCopy mc).getCopyTargetExpr().getAChild*() +} + +/** An variable access that looks like a read, but isn't. */ +predicate isSpuriousRead(VariableAccess e) { + e = any(FunctionCall fc | fc.getTarget().getName().matches("%ProbeFor%")).getAnArgument().getAChild*() or + e = any(MacroInvocation m | m.getMacroName().matches("%ProbeFor%")).getAnExpandedElement().(Expr).getAChild*() or + e = any(FunctionCall fc | fc.getTarget().getName().matches("Mm%ecureVirtualMemory")).getAnArgument().getAChild*() or + e = any(FunctionCall fc | fc.getTarget().getName().matches("ZwUnmapViewOfSection")).getAnArgument().getAChild*() +} + +predicate isReadExpr(Expr e) { + not isWriteExprTarget(e) and not isSpuriousRead(e) +} diff --git a/src/microsoft/code/cpp/public/windows/kernel/MemoryOriginDereferences.qll b/src/microsoft/code/cpp/public/windows/kernel/MemoryOriginDereferences.qll new file mode 100644 index 00000000..d204aa77 --- /dev/null +++ b/src/microsoft/code/cpp/public/windows/kernel/MemoryOriginDereferences.qll @@ -0,0 +1,128 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** + * Provides classes for identifying dereferences of data + * flowing from memory origins. + */ + +private import microsoft.code.cpp.public.controlflow.Dereferences +private import semmle.code.cpp.dataflow.new.DataFlow +private import microsoft.code.cpp.public.windows.kernel.SystemCalls as SystemCalls +import MemoryOrigins +private import KernelMode + +/** A dereference or an argument of a call. */ +class LocalAccess extends Expr { + LocalAccess() { + /* + * Dereferences that only occur when the memory is guaranteed to be in kernel mode + * can be ignored. + */ + not inKernelModeIfStmt(this) and + not this instanceof IgnoredAccess and + ( + this instanceof Dereference + or + exists(Function f | + f.hasEntryPoint() and + this = f.getACallToThisFunction().getAnArgument() + ) + ) + } +} + +private module AccessDataFlowConfig implements DataFlow::ConfigSig { + + + predicate isSource(DataFlow::Node source) { + source.asExpr() = any(MemoryOrigin origin).getADirectBufferAccess() + } + + predicate isSink(DataFlow::Node sink) { + sink.asExpr() instanceof LocalAccess + } +} + +private module AccessDataFlow = DataFlow::Global; + +/** + * Holds if data flows (inter-procedurally) from memory origin `origin` + * to local access `access`. + */ +predicate memoryOriginDataFlow(MemoryOrigin origin, LocalAccess access) { + AccessDataFlow::flow(DataFlow::exprNode(origin.getADirectBufferAccess()), DataFlow::exprNode(access)) +} + +private module DereferenceDataFlowConfig implements DataFlow::ConfigSig { + + predicate isSource(DataFlow::Node source) { + memoryOriginDataFlow(_, source.asExpr()) + } + + predicate isSink(DataFlow::Node sink) { + sink.asExpr() instanceof LocalDereference + } + + predicate isBarrier(DataFlow::Node barrier) { + isSpuriousRead(barrier.asExpr()) + } +} + +private module DereferenceDataFlow = DataFlow::Global; + +/** + * Holds if data flows (inter-procedurally) from `access` to the + * direct dereference `dereference`. + */ +predicate dereferenceDataFlow(LocalAccess access, DirectDereference dereference) { + DereferenceDataFlow::flow(DataFlow::exprNode(access), DataFlow::exprNode(dereference)) +} + +/** + * Gets an operation that dereferences `origin`, identifying the intermediate + * `access` that leads to the dereference. + */ +DirectDereference getANestedDereference(MemoryOrigin origin, Expr access) { + memoryOriginDataFlow(origin, access) and + dereferenceDataFlow(access, result) +} + +/** + * Gets an operation that dereferences `origin`. + */ +DirectDereference getANestedDereference(MemoryOrigin origin) { + result = getANestedDereference(origin, _) +} + +/** + * An access that should be ignored for the purposes of determining dereferences of + * `PointerToUserModeParameter`. + */ +abstract class IgnoredAccess extends VariableAccess { } + +/** + * An access that is considered as directly dereferencing for the purposes of determining + * dereferences of `PointerToUserModeParameter`. + */ +abstract class LocalDereferencingAccess extends VariableAccess { } + +/** + * An access that is considered as directly dereferencing by default. + */ +class DefaultDereferencingAccess extends LocalDereferencingAccess { + DefaultDereferencingAccess() { + this instanceof LocalDereference or + isInUndefinedCall(this) or + isInMemReadWriteCall(this) + } +} + +/** + * Whether `va` is an argument to a call to an undefined function. + */ +private predicate isInUndefinedCall(VariableAccess va) { + exists(FunctionCall fc | + fc.getAnArgument() = va | + not fc.getTarget().hasDefinition() + ) +} diff --git a/src/microsoft/code/cpp/public/windows/kernel/MemoryOrigins.qll b/src/microsoft/code/cpp/public/windows/kernel/MemoryOrigins.qll new file mode 100644 index 00000000..e34a65ee --- /dev/null +++ b/src/microsoft/code/cpp/public/windows/kernel/MemoryOrigins.qll @@ -0,0 +1,292 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** + * Provides classes for identifying memory origins. + */ + +import cpp +private import semmle.code.cpp.dataflow.new.DataFlow +private import microsoft.code.cpp.public.windows.kernel.IRP +private import microsoft.code.cpp.public.windows.kernel.SystemCalls + +private module NestedBufferDataFlowTrace = DataFlow::Global; + + +/** + * A "handle" to an element that in some sense is the origin of a buffer. This + * handle is used to link buffers together when they are known to be the same + * and to report helpful alert messages. + * + * This class is indirectly extensible. To add a new kind of memory origin, + * extend the abstract class `DirectMemoryOrigin`. + */ +class MemoryOrigin extends Element { + MemoryOrigin() { + this instanceof DirectMemoryOrigin + or + nestedMemoryOrigin(this) + } + + /** Gets an access to this buffer. */ + final Expr getADirectBufferAccess() { + result = this.(DirectMemoryOrigin).getADirectBufferAccess() + or + nestedMemoryOrigin(this) and + result = this + } + + /** Gets an expression that holds the size of this buffer. */ + final Expr getABufferSizeExpression() { + result = this.(DirectMemoryOrigin).getABufferSizeExpression() + } + + /** + * Holds if the contents of this buffer can be initialized from + * the origin (for example, a "user-mode buffer"). + */ + predicate originCanInitialize() { + this.(DirectMemoryOrigin).originCanInitialize() + or + nestedMemoryOrigin(this) + } + + /** + * Holds if the contents of this buffer can be read from the origin. This + * includes both the case where the buffer can be read concurrently while + * trusted code is writing it, and the case where the buffer contents will + * eventually be copied back to the origin. + * + * This means in practice that this origin is vulnerable to information + * leaks if confidential data is written to it. + */ + predicate originCanRead() { + this.(DirectMemoryOrigin).originCanRead() + or + nestedMemoryOrigin(this) + } + + /** + * Holds if the contents of this buffer can be updated from the origin after + * trusted code has started reading it. + * + * This means in practice that this origin is vulnerable to "double fetch" + * issues if trusted code assumes that repeating a read will give the same + * result. + */ + predicate originCanWrite() { + this.(DirectMemoryOrigin).originCanWrite() + or + nestedMemoryOrigin(this) + } + + /** + * Holds if this buffer points to virtual memory that can be unmapped from + * the origin while trusted code is reading or writing it. + * + * This means in practice that this origin is vulnerable to denial of service + * if trusted code accesses it without properly handling errors in a `__try` + * block. + */ + predicate originCanUnmap() { + this.(DirectMemoryOrigin).originCanUnmap() + or + nestedMemoryOrigin(this) + } +} + +/** + * A memory origin that is not defined by data flow analysis from other memory + * origins. This is the class to extend when adding a new memory origin. + */ +abstract class DirectMemoryOrigin extends Element { + // TODO: should there be both a getADirectBufferSource and a + // getADirectBufferSink? A function that copies a kernel buffer to a user + // buffer is more like a sink. + /** Gets an access to this buffer. */ + abstract Expr getADirectBufferAccess(); + + /** Gets an expression that holds the size of this buffer. */ + abstract Expr getABufferSizeExpression(); + + /** + * Holds if the contents of this buffer can be initialized from + * the origin (for example, a "user-mode buffer"). + */ + predicate originCanInitialize() { any() } + + /** + * Holds if the contents of this buffer can be read from the origin. This + * includes both the case where the buffer can be read concurrently while + * trusted code is writing it, and the case where the buffer contents will + * eventually be copied back to the origin. + * + * This means in practice that this origin is vulnerable to information + * leaks if confidential data is written to it. + */ + predicate originCanRead() { any() } + + /** + * Holds if the contents of this buffer can be updated from the origin after + * trusted code has started reading it. + * + * This means in practice that this origin is vulnerable to "double fetch" + * issues if trusted code assumes that repeating a read will give the same + * result. + */ + predicate originCanWrite() { none() } + + /** + * Holds if this buffer points to virtual memory that can be unmapped from + * the origin while trusted code is reading or writing it. + * + * This means in practice that this origin is vulnerable to denial of service + * if trusted code accesses it without properly handling errors in a `__try` + * block. + */ + predicate originCanUnmap() { none() } + + /** + * Holds if this buffer may contain pointers that a reader of this buffer + * might dereference. Such pointers are sometimes used for sharing memory + * directly between user mode and kernel mode in Windows, and they must only + * be accessed with the proper pattern of probing and exception handling. + * + * Note that a nested origin may be vulnerable to types of attack that its + * parent origin is not vulnerable to. + */ + predicate mayContainNestedOrigins() { any() } +} + +/** + * An MDL is a mechanism for mapping a shared range of physical memory into more + * than one virtual address space. Typically, this is used for sharing memory + * between user mode and kernel mode, or between a virtualized guest and its + * host. + * + * MDL-mapped memory does not require probing or installing an exception handler + * before access, but since MDLs are typically used across a trust boundary, it + * is important to avoid double fetches and information leaks. + */ +class MdlOrigin extends DirectMemoryOrigin, FunctionCall { + MdlOrigin() { + this.getTarget().getName() = "MmGetSystemAddressForMdlSafe" + } + + override Expr getADirectBufferAccess() { + result = this + } + + override Expr getABufferSizeExpression() { none() } + + override predicate originCanWrite() { any() } +} + +/** + * A parameter pointing directly to user-mode memory. + */ +class AbstractPointerToUserModeSysCallParameterOrigin extends Parameter, DirectMemoryOrigin { + AbstractPointerToUserModeSysCallParameterOrigin() { + this instanceof AbstractPointerToUserModeSysCallParameter + } + + override Expr getADirectBufferAccess() { + parameterUsePair(this, result) + } + + override Expr getABufferSizeExpression() { none() } + + override predicate originCanWrite() { + this = any(PointerToUserModeSysCallParameter p | + not p.isOutParameter() + ) + } + + override predicate originCanUnmap() { + this instanceof PointerToUserModeSysCallParameter + } + + // This should technically be `any()`, but tracking nested origins within + // syscall parameters will generate too many results for the subsequent + // analyses to be feasible. + override predicate mayContainNestedOrigins() { none() } +} + +/** + * An `IRP` memory origin. + */ +class IRPOrigin extends DirectMemoryOrigin { + /** Whether this memory origin refers to a system buffer. */ + private boolean isSystemBuffer; + /** The `IRP` parameter this memory origin comes from. */ + private IRP::Parameter parameter; + + IRPOrigin() { + ( + ( + parameter = this.(IRP::Type3InputBufferAccess).getParameter() and + isSystemBuffer = false + ) + or + ( + parameter = this.(IRP::UserBufferAccess).getParameter() and + isSystemBuffer = false + ) + or + ( + parameter = this.(IRP::SystemBufferAccess).getParameter() and + isSystemBuffer = true + ) + ) + } + + override Access getADirectBufferAccess() { + result = this + } + + override Expr getABufferSizeExpression() { + // the buffer size must come from the same IRP parameter + result.(IRP::FieldAccess).getParameter() = parameter and + ( + (this instanceof IRP::Type3InputBufferAccess and result instanceof IRP::InputBufferLengthAccess) or + (this instanceof IRP::UserBufferAccess and result instanceof IRP::OutputBufferLengthAccess) or + ( + this instanceof IRP::SystemBufferAccess and + (result instanceof IRP::InputBufferLengthAccess or result instanceof IRP::OutputBufferLengthAccess) + ) + ) + } + + override predicate originCanRead() { isSystemBuffer = false } + + override predicate originCanWrite() { isSystemBuffer = false } + + override predicate originCanUnmap() { isSystemBuffer = false } +} + +/** + * Holds if `access` could be a pointer value read from a `MemoryOrigin`, + * meaning it might be user-controlled. + */ +private predicate nestedMemoryOrigin(PointerFieldAccess access) { + NestedBufferDataFlowTrace::flowToExpr(access) +} + +private predicate pointerFieldAccess(Expr object, PointerFieldAccess access) { + access.getQualifier() = object and + access.getActualType().getUnspecifiedType() instanceof PointerType +} + +private module NestedBufferDataFlowConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { + source.asExpr() = any(DirectMemoryOrigin mo | mo.mayContainNestedOrigins()).getADirectBufferAccess() + } + + // This lets us find origins nested in _nested_ origins. + predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) { + pointerFieldAccess(n1.asExpr(), n2.asExpr()) + } + + predicate isSink(DataFlow::Node sink) { + pointerFieldAccess(_, sink.asExpr()) + } +} diff --git a/src/microsoft/code/cpp/public/windows/kernel/SystemCalls.qll b/src/microsoft/code/cpp/public/windows/kernel/SystemCalls.qll new file mode 100644 index 00000000..de8b08d6 --- /dev/null +++ b/src/microsoft/code/cpp/public/windows/kernel/SystemCalls.qll @@ -0,0 +1,135 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** + * Provides classes representing system calls and their parameters. + * + * System calls are identified by SAL annotation. For system call parameters that are pointers to + * user mode memory, operations are provided to identify the places in the program where they are + * dereferenced. + */ +import cpp +import drivers.libraries.SAL +import microsoft.code.cpp.public.windows.kernel.KernelMode +import microsoft.code.cpp.public.controlflow.Dereferences + +/** + * A function that can be invoked by user mode through a system call. + */ +class SystemCallFunction extends Function { + SystemCallFunction() { + this = any(SALAnnotation a | a.getMacro().hasName("__kernel_entry") or + a.getMacro().hasName("_Kernel_entry_") or + a.getMacro().hasName("_Kernel_entry_always_") or + a.getMacro().hasName("__control_entrypoint")).getDeclaration() and + hasDefinition() + } +} + +/** + * An abstract class representing all the syscall pointer parameters. + */ +abstract class AbstractPointerToUserModeSysCallParameter extends Parameter { + AbstractPointerToUserModeSysCallParameter() { + // Only in defined functions - we are not interested in library functions + getFunction().hasDefinition() and + // Ignore cases where the parameter is redefined. + not exists(this.getAnAssignedValue()) and + not exists(AddressOfExpr addr | addr.getAddressable() = this) and + // Look for pointer type parameters to system calls. + this = any(SystemCallFunction scf).getAParameter() and + this.getType().getUnspecifiedType() instanceof PointerType + } + + /** + * Whether this parameter is marked as an `out` parameter. + */ + predicate isOutParameter() { + exists(SALAnnotation a | a.getMacroName().matches("__out%") | a.getDeclaration() = this) + } +} + +/** + * A `Parameter` on a system call that represents a pointer to user mode memory. + */ +class PointerToUserModeSysCallParameter extends AbstractPointerToUserModeSysCallParameter { + PointerToUserModeSysCallParameter() { + not this.getType().getName().matches("H%") + } +} + +/** + * A `Parameter` on a system call that represents a handle to user mode memory. + */ +class PointerToUserModeHandleParameter extends AbstractPointerToUserModeSysCallParameter { + PointerToUserModeHandleParameter() { + this.getType().getName().matches("H%") + } +} + +/** + * A hook for custom notions of validating user-mode data so that it + * is no longer tracked. + */ +abstract class ValidatingAccess extends VariableAccess {} + +/** + * A hook for custom notions of functions that do not return interesting user-mode data. + */ +abstract class SafeFunction extends Function {} + +/** + * An internal class adding the `comesFrom(Parameter p)` predicate which + * holds if this expression may point to user memory stemming from that + * parameter. + */ +library class MaybeUserMemory extends Expr { + final predicate comesFrom(Parameter p) { + not this instanceof ValidatedUse and ( + // Pointer arguments to system calls. + exists(SystemCallFunction syscall | parameterUsePair(p, this) | + p = syscall.getAParameter() and + p.getType().getUnspecifiedType() instanceof PointerType + ) or + // Member variables of things that may point to user memory. + any(MaybeUserMemory m | m.comesFrom(p)) = this.(VariableAccess).getQualifier() or + // A function that is passed potential memory has potential memory. + exists(Function f, Parameter param, int i | parameterUsePair(param, this) and + param = f.getParameter(i) | + f.getACallToThisFunction().getArgument(i).(MaybeUserMemory).comesFrom(p) + ) or + // Pointer-typed values returned from calls on user memory. + exists(Call c, Function f | c.getQualifier().(MaybeUserMemory).comesFrom(p) | + this = c and + c.getTarget() = f and + c.getType().getUnspecifiedType() instanceof PointerType and + not f instanceof SafeFunction + ) or + // A read of a variable where the associated write had user memory. + exists(MaybeUserMemory def | definitionUsePair(_, def, this) | + def.comesFrom(p) + ) or + // An object constructed from a user-mode handle. + this.(ConstructorCall).getAnArgument() = any(MaybeUserMemory m | + m.getType().getName().matches("H%") and + m.comesFrom(p) + ) + ) + } +} + +/** + * A use of a variable which has been validated by some `ValidatingAccess`. + */ +class ValidatedUse extends VariableAccess { + ValidatedUse() { + this instanceof ValidatingAccess or + useUsePair(_, any(ValidatedUse v), this) + } +} + +/** + * An expression that may point to user memory. + */ +class PotentialUserMemory extends MaybeUserMemory { + PotentialUserMemory() { this.comesFrom(_) } +} diff --git a/src/qlpack.yml b/src/qlpack.yml index b96e514a..ca6b3e74 100644 --- a/src/qlpack.yml +++ b/src/qlpack.yml @@ -5,7 +5,7 @@ library: false warnOnImplicitThis: false compileForOverlayEval: false name: microsoft/windows-drivers -version: 1.8.3 +version: 1.9.0 description: CodeQL queries designed for Windows device driver development. dependencies: codeql/cpp-all: ^7.0.0 diff --git a/src/windows-driver-suites/recommended.qls b/src/windows-driver-suites/recommended.qls index 22a8649b..ecadd949 100644 --- a/src/windows-driver-suites/recommended.qls +++ b/src/windows-driver-suites/recommended.qls @@ -54,7 +54,12 @@ - microsoft/Likely Bugs/Boundary Violations/PaddingByteInformationDisclosure.ql - microsoft/Likely Bugs/Conversion/BadOverflowGuard.ql - microsoft/Likely Bugs/Conversion/InfiniteLoop.ql + - microsoft/Likely Bugs/Memory Management/ConditionallyUninitializedVariable.ql + - microsoft/Likely Bugs/Memory Management/UnprobedDereference.ql + - microsoft/Likely Bugs/Memory Management/UserModeMemoryOutsideTry.ql + - microsoft/Likely Bugs/Memory Management/UserModeMemoryReadMultipleTimes.ql - microsoft/Likely Bugs/Memory Management/UseAfterFree/UseAfterFree.ql + - microsoft/Likely Bugs/UnguardedNullReturnDereference.ql - microsoft/Likely Bugs/UninitializedPtrField.ql - microsoft/Security/Crytpography/HardcodedIVCNG.ql - queries: .