222471 - Use /GS (Buffer Security Check) option when building with Visual Studio .NET

Status: RESOLVED WORKSFORME

(Firefox Build System :: General, defect)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

The /GS option is similar to StackGuard for GCC. When an application built with
/GS encounters a buffer problem, it will pop up a message box warning the user
of a potential security issue and call ExitProcess.

Comment 1

[timeless]

personally i don't mind bigger apps or slower apps, but i don't really like it
when an app kills itself. to be fair, i don't like it when someone else kills
my apps either, so my only reservation doesn't hold much value.

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

And my point is that I'd rather have the app kill itself than have the bad guy
format my HD after reading my personal information. 

What timeless means is that in most cases a detected buffer overflow will
probably not be a hacking attempt, and you might even be able to save before
exit if you are lucky (what he wants to try at least).

I'd rather exit, and fix the bug, or risk the hacking attempt. Also I wouldn't
put much faith in being able to save data after buffer overflow has happened,
and I certainly would not trust anything the app was doing after a buffer overflow. 

In any case, it seems like you could actually control what should happen:

"_set_security_error_handler lets you customize the response to a buffer
overrun. Reporting buffer overrun conditions is enabled with /GS.
_set_security_error_handler registers a security error handler, which should
report the failure. A program compiled with /GS that overruns a buffer and does
not define a custom handler failure handler will display a message box.

The parameter _secerr_handler_func is the typedef for security error handler
functions, and is defined thus:

typedef void (__cdecl * _secerr_handler_func)(int, void *);
The error handler takes two parameters: the first is a code for the kind of
failure; the second is a generic pointer to data, the meaning of which depends
on the failure code.

The only security failure code available is _SECERR_BUFFER_OVERRUN, and the
extra data is unused, and should always be NULL.

A user-written security handler should not try to throw or raise any sort of
exception. If the return address is corrupted, any exception handler pointer in
the same function is also probably corrupted, so trying to issue an exception
will open the application to a security violation similar to a buffer overrun.

After handling a buffer overrun, you should terminate the thread or exit the
process because the thread's stack is corrupted.

There is a single _set_security_error_handler handler for all dynamically linked
DLLs or EXEs; even if you call _set_security_error_handler your handler may be
replaced by another or that you are replacing a handler set by another DLL or EXE.
"


I configured with --enable-optimize-GS and did debug clobber build without a
problem. However, when I try to run the app I get an alert dialog just after
showing splash screen which says roughly this:

Debug Assertion Failed!

File: dbgheap.c
Line: 1132

Expression: _CrtIsValidHeapPointer(pUserData)

I'll try optimized build next.

Someone who knows our build system etc. better than me would probably need to
figure that out. I think the relevant part in the compiler documentation is here:

"/GS requires CRT startup code. This raises an issue with /GS when used to
compile a DLL. The security cookie's expected value is reset by the CRT in the
CRT_INIT function. If you have a function that is compiled with /GS (and thus
has the security cookie) that in turn calls CRT_INIT, the expected security
cookie value will change and the program will think that it has had a buffer
overrun. The solutions are to: 

  * Not use arrays in any functions that call (or end up calling) CRT_INIT, for
example, use _alloca instead. 
  * Let the CRT initialize normally. Don't specify your own entry point, use
DllMain instead (and don't call CRT_INIT). 

/GS is not supported with /clr."

Btw, a .NET article mentions that in most cases the perf impact is less than 2%,
and that most applications will in fact not notice any performance difference.
Also, in most cases executables will not be noticeably bigger.

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Hmm, what do you know. It seems like my optimized build worked. I used objdir
and this .mozconfig:

mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-@CONFIG_GUESS@
ac_add_options --enable-crypto
ac_add_options --enable-extensions=all
ac_add_options --disable-debug
ac_add_options --enable-optimize="-O -GS"

Error detection even seems to work. I put a little sample buffer overrun in
nsAppRunner.cpp in the beginning of main() and got the warning dialog and exit.

Comment 4

[timeless]

i was supposed to mention that debug builds have a tendency to result in various
heap asserts. iirc i tend to get one in dbm.

Comment 5

[Chase Phillips]

Mass reassign of open bugs for [e-mail] to [e-mail].

Comment 6

[Darin Fisher]

*** Bug 344632 has been marked as a duplicate of this bug. ***

Comment 7

[Daniel Veditz [:dveditz]]

I run this way in my own builds all the time, works great. Haven't run any perf tests, but I haven't noticed a slowdown. I'm not likely to notice 2% though.

Comment 8

[(not currently active) Ted Mielczarek]

Per http://msdn.microsoft.com/en-us/library/8dbf701c(VS.80).aspx

"/GS is on by default. Use /GS- if you expect your application to have no security exposure."

We should probably look into using the hook mentioned in comment 2 in breakpad so we can catch and report buffer overflows as crashes.