Poking around MSVIDCTL.DLL

Hey all,

I have to admit that I did not follow the msvidctl.dll situation all that closely, except for my short tweet that this bug was apparently reported more than a year ago. Yesterday, my old friend Dennis Elser piqued my interest: He had isolated the bug down to a function called ATL::CComVariant::ReadFromStream. He had determined that the function in question made a rather strange mistake: Instead of passing a pointer to a data buffer to IStream::Read, it took the address of a (small) local variable, and passes this address as output buffer to IStream::Read, along with a length read from the stream previously.

Somebody clearly got confused.

So Dennis and me sat down tonight and did a bit of digging around and tried to clarify what was going on. So we dug in a bit deeper, and ended up with the following understanding of the situation:

1. If the stuff that is supposed to be deserialized does not contain the proper value in the first 2 bytes, 8 more bytes are read, and SafeArrayCreate is used to create a new array with a 4-byte size obtained from these bytes.
2. A pointer to the allocated memory is obtained by ways of calling SafeArrayAccessData. This function places a pointer to the memory in question into a memory cell pointed to by it's second argument.
   
3. Instead of passing the CONTENTS of the memory cell pointed to by the second argument to IStream::Read, the code in question passes the address of this variable in. This happens to be the re-used memory for the first argument of ReadFromStream, hence it is on the stack. Memory corruption hilarity ensues.

This is a cute little bug. First of all, it is a beautiful example of a single excess "&" in the source code. But what is most amusing about this bug is the centrality of it -- it is, after all, in a method of a class from the ATL.

Everybody loves bugs in libraries. Few things fill my heart with quite as much amusement as bugs in heavily-used, statically-linked libraries. OpenSSL (and libeay) was good for many laughs in the past, zlib was a favourite for a long time, too.

So what we have here is a bug in a component that is used fairly widely, and that has the property of being statically linked (yay templates !).

Now, a quick search of my harddisk turned out that a lot of third-party components (flash) contain the same function -- but in old and non-vulnerable versions (for an extra dash of irony, the function used to be safe before all this SafeArray-stuff was added). Only a small fraction of the files that use the ATL and contain this function appear to contain it in a vulnerable version.

We ended up building a really naive / stupid / false-negative-and-false-positive-prone regexp to scan for the vulnerable basic block. This is of course going to fail if anyone has tweaked their optimization settings etc., but it would still be interesting to find out how many files contain this "trivial" byte string.

So I searched my windows directory for the following regexp pattern:

pattern = "\x8B\x07\x6A\x00\xFF\x75\x2E\x8D\x4D\x2E\x51\x57\xFF\x50\x0C\x53"
r = re.compile( pattern, re.DOTALL )

There were a few files in which this pattern was found (XP):

Found pattern in file c:\Windows\system32\ieframe.dll
Found pattern in file c:\Windows\system32\mstscax.dll
Found pattern in file c:\Windows\system32\msvidctl.dll
Found pattern in file c:\Windows\system32\wmp.dll
Found pattern in file c:\Windows\system32\wmpdxm.dll

Dennis searched for the same pattern on his disk (Vista) and found:

c:\windows\system32\cic.dll
c:\windows\system32\comsnap.dll
c:\windows\system32\comsvcs.dll
c:\windows\system32\ieframe.dll
c:\windows\system32\mmcndmgr.dll
c:\windows\system32\mstscax.dll
c:\windows\system32\MSVidCtl.dll
c:\windows\system32\puiobj.dll
c:\windows\system32\rdpencom.dll
c:\windows\system32\shdocvw.dll
c:\windows\system32\wiaaut.dll
c:\windows\system32\wmp.dll
c:\windows\system32\wmpdxm.dll

Why the difference ? Well, amusingly, shdocvw.dll on my XP machine doesn't have this SafeArray-stuff in it yet -- it is probably compiled using an older, not vulnerable variant of the ATL -- wheras Dennis variant of the same DLL is much newer, compiled with the 'broken' variant of the ATL.

So, where does this leave us ?

1. The bug is actually much "deeper" than most people realize.
2. The killbit-fix is clearly insufficient, as there are bound to be many other ways of triggering the issue.
   
3. The bug might have weaseled it's way into third-party components, IF anyone outside of Microsoft had access to the broken ATL versions.
   
4. If this has happened, MS might have accidentally introduced security vulnerabilities into third-party products.
5. Depending on the optimization settings applied to the executables, it might require a bit of an effort to find out whether a vulnerable or non-vulnerable version of the code is present.
6. There might be a lot of recompiling next week.
7. IF this has gotten into third-party-products, I would bet that only a tiny fraction of software vendors will push out proper/timely updates.

It just seems that spending time to improve BinDiffs ability to find statically linked libraries might have been worth it :-)

Anyhow, I really need to get to sleep -- I have a train to catch in a bit more than 4 hours.

A lot of credit for this post has to go to Dennis Elser -- he did most of the hard work before we sat down.