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
Marcus Meissner mar...@jet.franken.de 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 Sun, Oct 24, 2010 at 03:44:35PM +0900, Dmitry Timoshkov wrote: Marcus Meissner mar...@jet.franken.de 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
Kees Cook k...@ubuntu.com 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 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
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 string.h
#include stdlib.h
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/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
disabling FORTIFY_SOURCE
Hi, 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. I realize it's a bit of a pain to work with given how you're building some of the structures, but I'd like to ask that it not be globally disabled. Thanks, -Kees [1] $ find . -type f -name '*.[ch]' | xargs wc -l | grep total 2678911 total [2] Some details at https://wiki.ubuntu.com/CompilerFlags#-D_FORTIFY_SOURCE=2 but at least the following... Compile time: - static buffer length checks - missed return values - open() checks for missing mode when used with O_CREAT Run time: - dynamic buffer length checks - dynamic format string safety check - dynamic format position safety checks Functions with buffer length (or other) checks: asprintf confstr dprintf fgets fgetws fprintf fread fwprintf getcwd getdomainname getgroups gethostname getlogin_r gets getwd longjmp mbsnrtowcs mbsrtowcs mbstowcs memcpy memmove mempcpy memset pread64 pread printf ptsname_r read readlinkat readlink realpath recv recvfrom snprintf sprintf stpcpy stpncpy strcat strcpy strncat strncpy swprintf syslog ttyname_r vasprintf vdprintf vfprintf vfwprintf vprintf vsnprintf vsprintf vswprintf vsyslog vwprintf wcpcpy wcpncpy wcrtomb wcscat wcscpy wcsncat wcsncpy wcsnrtombs wcsrtombs wcstombs wctomb wmemcpy wmemmove wmempcpy wmemset wprintf -- Kees Cook Ubuntu Security Team
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 ?
