Vulnerability Patching: How Microsoft recently screwed up

I am one to give Microsoft credit when it's due for a lot of the secure programming initiatives that have taken place over the last few years. I have been impressed with much of their efforts, which I have blogged about before.

Today I want to turn the tables and show why I also say that the weakest link in security is STILL the human factor. Even with the new secure programming paradigm at Microsoft, a recent set of patches show that the process is not perfect. Humans are fallible. Even the ones at Microsoft.

Back in April Microsoft released MS05-018, a patch for vulnerabilities in the Windows Kernel that could allow elevation of privilege and denial of service. One part of this patch dealt with issues relating to overflows on how csrss.exe processes fonts.

Basically a vulnerability existed in which a buffer holding a Unicode string for the face name of a font is copied without validating it's
size, causing a potential buffer overflow. This was done by the DoFontEnum() function of winsrv.dll.

The vulnerable code looks like:

push [ebp+arg_4] ; pointer to FaceName Unicode string.

lea eax,[ebp+var_70.lfFaceName]

push eax 	 ; pointer to buffer were string will be copied.

call wcscpy 	 ; No length check... it could crash here

It looks like the developers at Microsoft looked at this and decided that the best way to handle it was to validate the values beforehand. This would have the side effect of also fixing potential issues with other data structures that were also not being validated. So they added a validation function called ValidateConsoleProperties() before DoFontEnum() is ever called. Walla. The reported vulnerability was fixed.

But did they do it right? NOPE.

There is a problem with this. You have to ALWAYS check the values of data as it crosses from an untrusted to trusted boundary before you use it. Since DoFontEnum() could be called elsewhere, all they did was patch against this specific code execution path. And it ends up that they missed the fact that DoFontEnum() could be called elsewhere. And it was, causing the real bug to have to be fixed again in a follow up patch in MS05-049 this month.

But this also showed me a bigger problem. I have a lot of respect for Michael Howard (Security guru working on training developers to write secure code at Microsoft as part of SWI) and can only chuckle at how these bugs must have made him roll in his chair. In his work on writing the 19 deadly Sins of Software Security, there is a chapter on Buffer Overflows, and a section pointing out that you need to "Replace Dangerous String Handling Functions". His writings in "Writing Secure Code" does one better and shows how the string copy functions can be handled better. Yet these principles seem to have been missed by the developers working on the patch. When auditing the code in the kernel, I would imagine that one of the first things they would have done was scan for the likes of wcscpy, and replace them with the kernel safe string functions that already exist such as RtlStringCbCopy. Heck, Microsoft even has an entire page talking about using safe string functions for kernel developers.

It looks like in MS05-049 the developers fixed this properly. The code now looks like this:

push [ebp+arg_4] ; pointer to FaceName Unicode string.

lea eax,[ebp+var_74.lfFaceName]

push 20h         ; very important:size !!

push eax 	 ; pointer to buffer were string will be copied.

call StringCopyWorkerW ; this new function will only copy the specified bytes

That looks better. And fixes the bug properly, directly in DoFontEnum() as the untrusted data is being used.

Cesar Cerrudo wrote up a nice little paper on the topic called "Story of a Dumb Patch". Not sure I particularly like the title of it, but it's rather fitting. You can read his paper if you want more in depth coverage on the matter.

Moral of the story: Always validate you data when crossing from an untrusted to a trusted boundary. If you have data coming into a function, ALWAYS assume it's hostile until proven otherwise.