Hi project,
Following the two previous threads, I am posting here a draft patch implementing my proposal.
So, to begin with I will remind you the principle :
All function callable from outside wine, should be added sanity checks :
if safe_mode_on and (sanity_check1_failed or sanity_check2_failed [...] ) { ExitWineCleanlyAndAdvertiseUser; }
safe_mode_on value being read from registry (one and only time at wine startup).
So if safe_mode_on is set to true, sanity check are performed and in case of failure wine advertise user and terminate cleanly Else, sanity check aren't performed and on a bad input wine is crashing.
I've only produced a basic implementation, clearly marking as TODO items left to be done (by the way they are too much for me to handle, as I am no match for you when it comes to implementation). My goal here is firstly to demonstrate feasibility (hardly reached) and usefulness of the proposal. Secondly it is to provide something concrete.
Please note than "unsafe" mode behave exactly the same than latest git version of wine (some more operations occur, however, so is not strictly equal in term of performance)
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)
Results are as follow (test are wine_test calls wine on linux(1) platform only ) : 1 - all must-be-successful tests 100% succeed in the two modes 2 - in safe mode, the last two triggers the user message and wine exit 3 - in unsafe mode, the last two triggers a crash
Consequently, based on theses results no regression is identified.
As I received some more informations in last thread (thank you for enhancing my comprehension of the problem), I will again expose the problem, taking them into account :
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.
Personally, I don't mind that Microsoft is ignoring it and promote usage of early crash or whatever. You are providing a software, presenting security defects that you choose not to fix because they are compliant with your goals. Consequently, at the very least you should explicitly advertise your users, confronting them with their responsibilities.
I have read(3) on winehq a warning stating that wine is beta software, not ready for general use. I have read too about the "bug for bug" compatibility. So user's advertisement about security is rather thin and implicit.
Please consider the problem, you need to do something about it, it is your responsibility.
And continue the good work, you rocks ! Guillaume
(1) wine version : wine 1.1.14 with the attached patch applied on top of it OS : debian testing amd64 (2) http://cwe.mitre.org/data/definitions/391.html (3) http://www.winehq.org/about/
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".
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.
Hi Paul,
You asked me to actually describe the security I am concerned about, so I am going for it :
Imagine an ill-intentioned people, call it the attackers. By the mean of simply creating the following C application (based on classical "Hello word") :
#include needed header
int main (int argc, char * argv[]) { /* printf ( "Hello world!" ); */ GetOverlappedResult(0, NULL, NULL, FALSE);
return EXIT_SUCCESS; }
Running this application on wine, I get to have my crash, with the possibility of an exploit. So all I have to do know is to find a vector to make you and some other people willing to run my application.
I won't describe in detail the way to perform the exploit as : 1 - I don't know how to proceed and I don't want to 2 - It would be showing poor sense of responsibilities
Guillaume
---
2009/2/1 Paul TBBle Hampson Paul.Hampson@pobox.com
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.
--
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@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/
On Sun, Feb 01, 2009 at 10:41:25AM +0100, Guillaume SH wrote:
Hi Paul,
You asked me to actually describe the security I am concerned about, so I am going for it :
Imagine an ill-intentioned people, call it the attackers. By the mean of simply creating the following C application (based on classical "Hello word") :
#include needed header
int main (int argc, char * argv[]) { /* printf ( "Hello world!" ); */ GetOverlappedResult(0, NULL, NULL, FALSE);
return EXIT_SUCCESS;}
Running this application on wine, I get to have my crash, with the possibility of an exploit. So all I have to do know is to find a vector to make you and some other people willing to run my application.
I won't describe in detail the way to perform the exploit as : 1 - I don't know how to proceed and I don't want to 2 - It would be showing poor sense of responsibilities
If you can run an application ... it already can do everything!
No need to protect APIs against misuse, they run at the same privilege level as your code.
Ciao, Marcus
2009/2/1 Marcus Meissner marcus@jet.franken.de:
On Sun, Feb 01, 2009 at 10:41:25AM +0100, Guillaume SH wrote:
Running this application on wine, I get to have my crash, with the possibility of an exploit. So all I have to do know is to find a vector to make you and some other people willing to run my application.
I won't describe in detail the way to perform the exploit as : 1 - I don't know how to proceed and I don't want to 2 - It would be showing poor sense of responsibilities
If you can run an application ... it already can do everything!
No need to protect APIs against misuse, they run at the same privilege level as your code.
What exactly is being exploited?
Can you use this as a basis to inject your own code to change Wine's behaviour (e.g. using a buffer overflow exploit)? Can you use this to gain root privileges from a non-root user? Can you use it to gain access to or modify sensitive data?
What you have at the moment is an application that crashes. A crash could be: (a) because Wine is wrong and needs to be fixed (see the Coverity NULL pointer patches); (b) because the application is wrong - the application needs to be fixed here, not Wine; (c) someone has tampered with the system.
Taking your example of passing a NULL to GetOverlappedResult is that *even though the MSDN documentation does not mention that these parameters are optional (indicating they must be valid)* you are recommending that Wine makes these parameters optional. This has the effect of hiding bugs in the native application - for case (b) - or making them harder to track down. You are also making case (c) have potentially successful results as this is saying "not a problem, carry on".
If you report - as has been suggested - to the user, how is this useful to them? Can that be subverted to do malicious things? Why add to the code, providing something that has to be added to every API, that just increases the code size. How can you investigate and debug here?
If someone tampered this to say "do nothing, instead of reporting to the user" then you have lost this mechanism and you have turned a fail case into a success case and the application could then potentially do more damage.
There have been some attempts to add DrWatson functionality that will trap the exceptions (using a separate, overridable process), show the user it has crashed and gather information for a bug report. This is the same mechanism that winedbg uses to give the crash dump, and other debug tool. This is the best way to go for something like this - having a crash report program that displays a "report this to Wine developers" dialog. Microsoft, Mozilla and others use this approach.
- Reece
On Sun, Feb 01, 2009 at 10:41:25AM +0100, Guillaume SH wrote:
Imagine an ill-intentioned people, call it the attackers. By the mean of simply creating the following C application (based on classical "Hello word") :
#include needed header
int main (int argc, char * argv[]) { /* printf ( "Hello world!" ); */ GetOverlappedResult(0, NULL, NULL, FALSE);
return EXIT_SUCCESS;
Running this application on wine, I get to have my crash, with the possibility of an exploit.
A crash isn't magically a possibility of an exploit. Certain types of crashes (eg. user-supplied buffer overruns that hammer the return address on the stack) are vectors for security issues. Dereferencing a NULL isn't, off the top of my head.
A better exploit than GetOverlappedResult(0, NULL, NULL, FALSE) at that point is prolly to just do whatever your exploit's payload was going to be.
I won't describe in detail the way to perform the exploit as : 1 - I don't know how to proceed and I don't want to 2 - It would be showing poor sense of responsibilities
So you don't actually know what the exploit is you're trying to get us to break from the Win32 API to avoid, and you specifically refuse to describe it further?
Paul,
Basically, yes I don't know what the exploit is (there's no magic in there : possibility for an exploit is enough to justify action). But I don't ask for an API breakage, I propose wine to support two modes : one with API misuse checks and one strictly the same behaviour as Windows.
This leave the choice for users to use wine on the safe side or on the less safe side.
As I already answered to Marcus, I will go for some reflexion/documentation on the subject, Guillaume
2009/2/1 Paul TBBle Hampson Paul.Hampson@pobox.com
On Sun, Feb 01, 2009 at 10:41:25AM +0100, Guillaume SH wrote:
Imagine an ill-intentioned people, call it the attackers. By the mean of simply creating the following C application (based on classical "Hello word") :
#include needed header
int main (int argc, char * argv[]) { /* printf ( "Hello world!" ); */ GetOverlappedResult(0, NULL, NULL, FALSE);
return EXIT_SUCCESS;Running this application on wine, I get to have my crash, with the possibility of an exploit.
A crash isn't magically a possibility of an exploit. Certain types of crashes (eg. user-supplied buffer overruns that hammer the return address on the stack) are vectors for security issues. Dereferencing a NULL isn't, off the top of my head.
A better exploit than GetOverlappedResult(0, NULL, NULL, FALSE) at that point is prolly to just do whatever your exploit's payload was going to be.
I won't describe in detail the way to perform the exploit as : 1 - I don't know how to proceed and I don't want to 2 - It would be showing poor sense of responsibilities
So you don't actually know what the exploit is you're trying to get us to break from the Win32 API to avoid, and you specifically refuse to describe it further?
--
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@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/
On Sun, Feb 01, 2009 at 01:23:49PM +0100, Guillaume SH wrote:
Basically, yes I don't know what the exploit is (there's no magic in there : possibility for an exploit is enough to justify action).
So without the magic, there's no demonstrated possibility for an exploit, so the justification goes away...
Anyway, getting some unit tests written and committed is prolly your best way forward. Discussing behaviour is one thing, demonstrating that it's correct and sensible is another, more effective thing.
The Wine test suite, as I understand it, is considered part of Wine's Win32 API documentation. And it's the place to demonstrate that a given behaviour is correct and consistent.
-
Guillaume SH -
Marcus Meissner -
Paul TBBle Hampson -
Reece Dunn