Closed Bug 222471 Opened 21 years ago Closed 16 years ago

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

Categories

(Firefox Build System :: General, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: hjtoi-bugzilla, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [sg:want])

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.
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.
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.
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.
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.
Assignee: leaf → cmp
Product: Browser → Seamonkey
Mass reassign of open bugs for chase@mozilla.org to build@mozilla-org.bugs.
Assignee: chase → build
Assignee: build → nobody
Product: Mozilla Application Suite → Core
QA Contact: build-config
*** Bug 344632 has been marked as a duplicate of this bug. ***
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.
Whiteboard: [sg:want]
Flags: blocking1.9.1?
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.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → WORKSFORME
Flags: blocking1.9.1?
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.