Accidental Denial of Service through Inefficient Program Design Part 2 – Incorrectly Implementing Interfaces

Artikkeli
03/17/2015

There are few things that are more annoying as a user than to have the performance of a computer which they’re using grind to a halt. This series will outline program design flaws that I’ve run across, explain what they are, how they impact the system, example scenarios where the impact will be exacerbated, and safer alternatives to accomplish the same programming goal. In part two of this series, we’ll be exploring:

Incorrectly Implementing Interfaces

What It Is

There are a multitude of entry points in Windows where functionality can be added to, enhanced, or even replaced through the use of objects which implement interfaces. In many if not most of those cases, such entry points affect the user experience across many applications but are not implemented in their own processes. In other words, the system will load those objects in other processes to use them as those processes use system functionality that those objects integrate with. Some common examples of this would be user mode drivers and shell extensions.

How It Impacts the System

When an interface is documented, the expected behavior of each interface member is described as well as the expected behavior of the module function exports which will give access to initial objects. When those members or functions do not behave in ways the documentation states that they are required to behave, unexpected results can happen. Commonly these unexpected results take place in processes who haven’t done anything wrong, and it is difficult to figure out what the real root problem is for a veteran programmer while the average user or administrator incorrectly blames the application who was the victim of the bad code. It can lead to frustration with the system and even system restores and reinstalls by users who can’t resolve their issues.

Example Scenario of Exacerbation

I recently had a case where this exact kind of thing was happening; while I won’t name the specific parties involved, the scenario that I’m about to go over is in a commonly distributed component and has been known to affect the users of many applications. The interface in this scenario is IWiaTransferCallback, and the object implementing the interface which ultimately causes the problem is passed to the IWiaTransfer::Download method of a system provided object.

First, let’s go over what an object implementing IWiaTransferCallback, which inherits from IUnknown and is COM based, and the module which contains is required to do based on the documentation above and the content which it links to:

1. The object needs to implement the GetNextStream, TransferProgress, QueryInterface, AddRef, and Release methods.

2. The module needs to implement and export the DllCanUnloadNow and DllGetClassObject methods.

Due to the actual problem in this scenario, we’re going to ignore the DllGetClassObject, GetNextStream, TransferProgress, and QueryInterface and assume that they are all implemented correctly. This takes out all of the functionality specific to IWiaTransferCallback and leaves us with some basic COM interface implementation methods. The linked documentation on MSDN already does a great job of what each of these are supported to do, so I’ll only include a brief snippet here:

AddRef – increments the number of references to an object

Release – decrements the number of references to an object; also frees the object when there are no longer any references to the object

DllCanUnloadNow - determines whether the module from which it is exported is still in use; a module should be considered to be no longer in use when it is not managing any existing objects which should be determined by the reference count for all of its objects to be zero

What that boils down to is that each object should have its own reference counter so it knows whether or not it is safe to be freed, and that each object type should have its own reference counter so that the module knows whether or not an instance of each of its types is open or not. There is a caveat to this though; the DllCanUnloadNow function doesn’t have to be exported in some scenarios and in other scenarios it is an informative context instead of an inquisitive context. In the former case, the module won’t be unloaded until COM is uninitialized. In the latter, the function will be called before the module is unloaded but the result of the function will be ignored. In both cases, the module is still responsible for keeping objects and itself available while they are still in use. Whether or not these apply can be derived from the documentation for the specific interface(s) being implemented.

In this scenario, the implementer of IWiaTransferCallback, part of a scanner mini-driver, neglected to properly implement these functions. AddRef and Release always returned zero, it didn’t keep track of its references properly otherwise, and it allowed the module to be unloaded prematurely. When IWiaTransfer::Download was called, the system called AddRef more times than it called Release on the object before returning since it was still using that object in a RPC call. The module didn’t account for this this, the Release function pointed to an address within the module, and when the system later went to call Release on the object when it was done with it, the Release function no longer existed in memory. The result of that was an access violation which crashed whichever application was using the driver to retrieve a scan when the application called CoUninitialize to initiate cleanup on a thread. In the case of an application that only used COM from one thread, this resulted in the application crashing on exit. This turned out to be particular problematic for applications which implemented COM on multiple threads though as it caused the application to crash whenever it cleaned up the thread that was doing the scanning.

Luckily, there was an easy way to work around this particular problem. An application using this driver could call GetModuleHandleEx with the flags set to not unload the module before the module was unloaded. This came with the cost of having to leave the module in memory when it really shouldn’t be needed and wasn’t something that the application should have needed to do had the driver implemented things correctly. It did fix the problem for end users with a fairly trivial amount of code though.

Safer Alternatives

The simplest thing to do in a case like this would be to use a framework or language that already takes care of basic IUnknown implementation for you when implementing interfaces which inherit from IUnknown. A couple of examples of this would be ATL for C++ or exposing a COM Interface from a CLR binary. If you are going to implement IUnknown on an object from scratch, make sure you do it correctly. Returning the correct value for AddRef and Release and makes it substantially easier to verify behavior external to your object, but the more important thing is that your module doesn’t let itself be unloaded while its address space is being used.

Comments

Anonymous
March 17, 2015
Hi Chris Just a heads-up. Raymond Chen recently posted that there is no strict requirement for AddRef to return the new reference count. ie. The return value from AddRef is not actually meant to be a reliable indicator of whether it is actually incrementing the reference count or not. blogs.msdn.com/.../10599631.aspx .dan.g.
Anonymous
March 17, 2015
Yes, that is correct. I was trying to indicate here that while it's not required to be accurate, your module is required to keep track internally and keep itself loaded in memory while its objects are still in use. However, implementing the return values lets you verify counting behavior as well which can be useful for testing your code and debugging failures as in the scenario here.

Jaa