A basic implementation for increased security in wine proposal

[Paul TBBle Hampson]

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.

> (2) http://cwe.mitre.org/data/definitions/391.html

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.

-- 
-----------------------------------------------------------
Paul "TBBle" Hampson, B.Sc, LPI, MCSE
Very-later-year Asian Studies student, ANU
The Boss, Bubblesworth Pty Ltd (ABN: 51 095 284 361)
Paul.Hampson at Pobox.com

Of course Pacman didn't influence us as kids. If it did,
we'd be running around in darkened rooms, popping pills and
listening to repetitive music.
 -- Kristian Wilson, Nintendo, Inc, 1989

License: http://creativecommons.org/licenses/by/2.5/au/
-----------------------------------------------------------
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
Url : http://www.winehq.org/pipermail/wine-devel/attachments/20090201/8e8c07c7/attachment-0001.pgp