This bug was created as a clone of Bug 620058 which contains more context.

Unless I've misunderstood, these don't need to be under |--enable-hardening| and can just be default flags, since they're compile-time warnings, no runtime impact.

This patch will add -Wformat-security -Wstack-protector -Werror=format-security. Note that -Wformat is already added. I did not add -Wpointer-sign because it seems to make useless warnings. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=23087 https://www.winehq.org/pipermail/wine-patches/2005-July/018747.html I first was unsure about -Werror=format-security, because it will turn the warning into an error, but it builds just fine for me. Also I was thinking if -Wformat-overflow(=2) would be helpful to add here. What do you think? https://gcc.gnu.org/onlinedocs/gcc-7.1.0/gcc/Warning-Options.html#Warning-Options

Comment on attachment 8869205 [details] [diff] [review] Patch v1 Review of attachment 8869205 [details] [diff] [review]: ----------------------------------------------------------------- The commit message should be updated, given that several of the flags mentioned aren't activated by the patch. Perhaps a "enable compile-time warnings for security-sensitive patterns" would be a more appropriate commit message? -Wformat-overflow=2 seems like it would be helpful. ::: build/moz.configure/warnings.configure @@ +113,5 @@ > check_and_add_gcc_warning('-Wno-gnu-zero-variadic-macro-arguments') > > +# Add compile-time warnings for unprotected functions and format functions that represent possible security problems > +check_and_add_gcc_warning('-Wformat-security') > +check_and_add_gcc_warning('-Wstack-protector') Reading the documentation doesn't provide any information on why this is helpful. @@ +114,5 @@ > > +# Add compile-time warnings for unprotected functions and format functions that represent possible security problems > +check_and_add_gcc_warning('-Wformat-security') > +check_and_add_gcc_warning('-Wstack-protector') > +check_and_add_gcc_warning('-Werror=format-security') I don't see why this needs an explicit -Werror=. If it really does, then turning format-security into a hard error should be conditional on --enable-warnings-as-errors, as is done elsewhere in this file.

Updated patch. -Wformat-security, this warning is meant to let a user know if they're using printf and scanf functions in an unsafe manner. Works together with -Wformat. https://fedoraproject.org/wiki/Format-Security-FAQ#What_is_-Wformat-security -Wstack-protector, if --enable-hardening is set, this gives warnings for any functions that aren't going to get protected. -Wformat-overflow=2, see https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

Comment on attachment 8872764 [details] [diff] [review] Patch v2 Review of attachment 8872764 [details] [diff] [review]: ----------------------------------------------------------------- r=me without -Wstack-protector until its benefit is better understood. We may just want to add it in only when --enable-hardening, since it's just useless clutter otherwise. ::: build/moz.configure/warnings.configure @@ +113,5 @@ > check_and_add_gcc_warning('-Wno-gnu-zero-variadic-macro-arguments') > > +# Add compile-time warnings for unprotected functions and format functions that represent possible security problems > +check_and_add_gcc_warning('-Wformat-security') > +check_and_add_gcc_warning('-Wstack-protector') I understand that -Wstack-protector provides warnings on what functions aren't going to get protected. My understanding is that the compiler is the one adding the stack protection to functions, so if it decides what functions can't be stack protected--a decision process that's not documented anywhere accessible--are we able to do anything about it?

(In reply to Nathan Froyd [:froydnj] from comment #5) > I understand that -Wstack-protector provides warnings on what functions > aren't going to get protected. My understanding is that the compiler is the > one adding the stack protection to functions, so if it decides what > functions can't be stack protected--a decision process that's not documented > anywhere accessible I believe it emits warnings for functions it does _not_ stack protect. And the functions it does not stack protect depends on the option chosen: -fstack-protector-all - Protects all functions. So we wouldn't see any warnings I think. -fstack-protector - Protects functions with a const size buffer 8 byts or larger or calls alloca. -fstack-protector-strong - Protects functions from -fstack-protector plus local array definitions or local frame addresses (see https://outflux.net/blog/archives/2014/01/27/fstack-protector-strong/ ) - ssp-buffer-size - a tuning parameter to change the above 8 bytes to whatever you want. Since in --enable-hardening we use '-fstack-protector-strong' (not counting /js/) we should still see warnings. > are we able to do anything about it? ¯\_(ツ)_/¯ In theory we could use the warnings to tune ssp-buffer-size - perhaps we want to try and eliminate more warnings from /dom/. But number of warnings isn't a great metric.

(In reply to Tom Ritter [:tjr] from comment #6) > > are we able to do anything about it? > > ¯\_(ツ)_/¯ > > In theory we could use the warnings to tune ssp-buffer-size - perhaps we > want to try and eliminate more warnings from /dom/. But number of warnings > isn't a great metric. OK. I think I'd prefer to leave the addition of the warning to people who feel like trying to tune the buffer size and leaving it off for the general population. Especially if we ever turn on --enable-hardening on infrastructure, having a warning that we can't do much about becoming a hard error isn't going to make anybody happy. (Yes, we could say -Wno-error, but then it's just noise...)

Agreed that it doesn't sound like a very useful warning; particularly given that --fstack-protector-strong is specifically designed to catch just the right functions, and _intentionally_ skip functions where it wouldn't add value.

Patch with review comments addressed.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/aea06724da78 Enable compile-time warnings for security-sensitive patterns. r=froydnj