On Sun, Feb 01, 2009 at 09:11:29AM +0100, Guillaume SH wrote:
I tested the two modes with the help of wine test suite, restricted to kernel/file.c, test_overlapped and I considered only : all must-be-successful tests GetOverlappedResult(0, NULL, &result, FALSE); GetOverlappedResult(0, &ov, NULL, FALSE); (these last two cannot be used in the same run, in either mode)
This is potentially a flaw in the tests. The NULL-passing-in tests should have an exception handler wrapped around them if they are expected to throw an exception, an example of such was posted to this mailing list during an earlier discussion of this method's failure cases.
If your automated test suite itself is crashing, you're not actually getting useful results.
The problem I am considering (and I may be mistaken here as I am not an expert) is called "Unchecked Error Condition"(2) and it is a referenced weakness (CWE id 391), rated "Medium" in likelihood of exploit, by serious people.
This isn't an Unchecked Error Condition in GetOverlappedResult. It's a known and understood invalid input (NULL for something that the API specifies cannot be NULL) with an empirically determined failure action (throw a NULL-reference exception).
An Unchecked Error Condition would occur if someone specifically wrote code around a GetOverlappedResult to catch the NULL-reference, and then ignored it and preceded on as if it hadn't given a NULL-reference.
It would (at least in according to my understanding of the reasoning behind the Win32 API) be much more common for people to accidentally produce the Unchecked Error Condition flaw if a NULL going into these inputs (which the API specifies is invalid) were to produce an error code.
Exceptions catch programmer's attention, they have to work to ignore them. They are "exceptional" events.
Error codes get ignored left, right and centre.
I can't see how GetOverlappedResult could be a security weakness.
Program does dumb thing -> program crashes. It's not executing user-provided code or jumping about the stack in any way. It just bubbles straight up to the structured exception handler if one exists, or away into the night if one doesn't. Either way the code flow is interrupted, not redirected.
If an attacker is limited to zeroing out a chunk of your stack, such that an otherwise-valid pointer becomes NULL and gets passed into GetOverlappedResult, you don't want to keep processing your error-handling path. Who know what other data the attacker was able to get onto your stack or heap? Instead, throw an exception and let the exception handling code (which should make significantly fewer assumptions about what state is valid) clean up for you.
And in that case, the security issue is whatever the attacker used to zero out chunks of your stack.
Can you actually please describe the security issue you're trying to fix here?
PS : I have observed that you are very active about fixing other security issues : + buffer overflow + NULL pointer dereference + variable signedness + ... I seems to me that hopefully, you performs those fixes without consideration for "Is windows doing the same ?". I will be sad the day I will see a commit "Remove resource release because Windows is doing the same" or "Remove buffer overflow fix because app A (2 million copies sold) rely on it".
Have a look at the ddrawex implementation. It's basically ddraw except it doesn't release resources as soon as they're released by the caller due to the way Internet Explorer and its plugins interact with the library. (Or at least that's the understanding I have from skimming the submitted patch)
It'd be a sad day when we start sacrificing Win32 API implementation accuracy just because we think the Win32 API is wrong.