Re: disabling FORTIFY_SOURCE
On 10/24/10 8:50 AM, Vitaliy Margolen wrote: On 10/24/2010 12:32 AM, Marcus Meissner wrote: Actually I would like to know if its just more than the dlls/shell32/pidl.c problem... Or just grep for '\[1\]' in include directory. Lots and lots of declarations in all different places. Stoopid question time. Is there a better method of declaring these than making them 2 char arrays? Can we 'assume' a maximum length and set them to that to prevent possible buffer overflows or is this something that we have to accept and work with? I understand that we want to keep the memory footprint for Wine as low as possible but we also don't want to introduce possible exploits in the process. James McKenzie
Re: disabling FORTIFY_SOURCE
On Sun, Oct 24, 2010 at 09:50:42AM -0600, Vitaliy Margolen wrote: > On 10/24/2010 12:32 AM, Marcus Meissner wrote: > >Actually I would like to know if its just more than the dlls/shell32/pidl.c > >problem... > If you take a look at winternl.h you'll see number of structures > there look like: > typedef struct _foo { > ULONG length; > WCHAR buffer[1]; > } foo, *pfoo; > > Or just grep for '\[1\]' in include directory. Lots and lots of > declarations in all different places. As I already wrote, this works. Here is a sample code which shows the problem dlls/shell32/pidl.c has: $ cat xx1.c #include #include struct foo { int x; char y[1]; }; union bar { struct foo fo; long y; float fl; }; struct berk { int t; union bar b; }; int main(int argc, char **argv) { struct berk *x1; struct foo *x2; x1 = malloc (sizeof(struct berk) + 5); x2 = malloc (sizeof(struct foo) + 5); strcpy(x1->b.fo.y, "hallo"); strcpy(x2->y, "hallo"); } $ gcc -O2 -Wall -D_FORTIFY_SOURCE=2 -g xx1.c -o xx1 xx1.c: In function ‘main’: xx1.c:28:1: warning: control reaches end of non-void function In file included from /usr/include/string.h:640:0, from xx1.c:1: In function ‘strcpy’, inlined from ‘main’ at xx1.c:26:8: /usr/include/bits/string3.h:107:3: warning: call to __builtin___strcpy_chk will always overflow destination buffer Only the "strcpy(x1->b.fo.y, "hallo");" with the nested struct is warned about, while the second strcpy works fine. It is just nested structs it does not like at this time. Ciao, Marcus
Re: disabling FORTIFY_SOURCE
On 10/24/2010 12:32 AM, Marcus Meissner wrote: Actually I would like to know if its just more than the dlls/shell32/pidl.c problem... If you take a look at winternl.h you'll see number of structures there look like: typedef struct _foo { ULONG length; WCHAR buffer[1]; } foo, *pfoo; Or just grep for '\[1\]' in include directory. Lots and lots of declarations in all different places. Vitaliy.
Re: disabling FORTIFY_SOURCE
Kees Cook writes: > It seems to me that disabling FORTIFY_SOURCE is a mistake. It offers > a great many protections, and virtually every distribution has very > intentionally turned on this compiler flag by default. Given Wine's > size[1], I would argue the benefits[2] outweigh the hassle of rearranging > the structures and accessors to not trick the compiler into allocating > memory beyond the end of the structure for incoming strings. > > It has found, at least in other projects, a lot of potential problems, > and better yet, has repeatedly turned exploitable vulnerabilities into > simple denial of services. So far in Wine, all it has done is repeatedly turn perfectly valid code into denial of service. Actually, even if Fortify worked correctly, the benefits would most likely be small, given that we make little use of the standard libc functions. Though given the trouble we've had so far, I shudder to think what would happen if we used libc functions all over the place. Fortify is a nice idea in theory, and I'd certainly encourage developers to enable it to see if it catches anything useful. But at this point it's not reliable enough to be forced upon end users. -- Alexandre Julliard julli...@winehq.org
Re: disabling FORTIFY_SOURCE
On Sun, Oct 24, 2010 at 03:44:35PM +0900, Dmitry Timoshkov wrote: > Marcus Meissner wrote: > > > Actually I would like to know if its just more than the dlls/shell32/pidl.c > > problem... > > It's the problem with any storage declared as something[1], there are plenty > of them in win32, and that's perfectly valid code. Actually FORTIFY knows those constructs in standalone structs and handles them correctly. It is only if they are embedded within another struct or unions when it has problems. Ciao, Marcus
Re: disabling FORTIFY_SOURCE
Marcus Meissner wrote: > Actually I would like to know if its just more than the dlls/shell32/pidl.c > problem... It's the problem with any storage declared as something[1], there are plenty of them in win32, and that's perfectly valid code. > (And of course also the stupid warn_unused_results warning spam, which we > disabled already.) It's also infamous "longjmp causes uninitialized stack frame": http://bugs.winehq.org/show_bug.cgi?id=21405 -- Dmitry.
Re: disabling FORTIFY_SOURCE
On Sat, Oct 23, 2010 at 11:54:27PM +, Dan Kegel wrote: > Hi Kees, > sure, supporting Fortify in Wine would be great, but it's not > clear how long it will take to fix Wine so it works with Fortify. > > Which would you prefer: > 1) have Wine broken for an unknown and possibly long time > or > 2) have Wine working, but without Fortify, until the bugs are fixed > ? Actually I would like to know if its just more than the dlls/shell32/pidl.c problem... (And of course also the stupid warn_unused_results warning spam, which we disabled already.) Ciao, Marcus
re: disabling FORTIFY_SOURCE
Hi Kees, sure, supporting Fortify in Wine would be great, but it's not clear how long it will take to fix Wine so it works with Fortify. Which would you prefer: 1) have Wine broken for an unknown and possibly long time or 2) have Wine working, but without Fortify, until the bugs are fixed ?