February 2009 - Wine-Devel - list.winehq.org

Re: A basic implementation for increased security in wine proposal

by Reece Dunn

2009/2/1 Marcus Meissner <marcus(a)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

Feb. 1, 2009

Re: A basic implementation for increased security in wine proposal

by Marcus Meissner

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

Feb. 1, 2009

Re: A basic implementation for increased security in wine proposal

by Guillaume SH

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(a)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. > > > (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(a)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/ > ----------------------------------------------------------- > > > >

Feb. 1, 2009

Re: [patch] review requested - NamedPipe Message mode (was Reliable Datagram / Stream behaviour of NT Pipes)

by Alexandre Julliard

Luke Kenneth Casson Leighton <lkcl(a)lkcl.net> writes: >> You can't do that stuff on the client side. You either have to do all >> pipe I/O in the server, or add named pipe support in the kernel. The >> latter is harder, but would be much more useful. > > well, not entirely knowing the difference, i'm guessing that i'm > adding named pipe support in the kernel. starting from > SetNamedPipeHandleState and going from there. I mean the Linux kernel. > comparing what i've got so far in the improvements to ReadFile, > WriteFile, NtFsControlIo etc. with the way that it is believed that > this _should_ be done, what is right and what is wrong? It's all wrong, because this can't be done on the client side (kernel32 and ntdll are the client side, as opposed to the wineserver side). The client side is the equivalent of user space, with the wineserver being the equivalent of kernel space. You can't do this stuff in user space (hint: concurrency). -- Alexandre Julliard julliard(a)winehq.org

Feb. 1, 2009

Re: A basic implementation for increased security in wine proposal

by 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(a)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/ -----------------------------------------------------------

Feb. 1, 2009

A basic implementation for increased security in wine proposal

by Guillaume SH

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".

Feb. 1, 2009

Re: [patch] review requested - NamedPipe Message mode (was Reliable Datagram / Stream behaviour of NT Pipes)

by Luke Kenneth Casson Leighton

> You can't do that stuff on the client side. You either have to do all > pipe I/O in the server, or add named pipe support in the kernel. The > latter is harder, but would be much more useful. well, not entirely knowing the difference, i'm guessing that i'm adding named pipe support in the kernel. starting from SetNamedPipeHandleState and going from there. comparing what i've got so far in the improvements to ReadFile, WriteFile, NtFsControlIo etc. with the way that it is believed that this _should_ be done, what is right and what is wrong? l.

Feb. 1, 2009

Wine magazine articles?

by Dan Kegel

Hey everyone, does Wine have a gallery of press clippings anywhere? I couldn't find one, so I started one at http://wiki.winehq.org/Reviews just for long-form reviews. If you remember any good reviews of Wine (1.0 or later especially), please link them in there. - Dan

Feb. 1, 2009