Add Security Warning Flags: -Wformat -Wformat-security -Werror=format-security -Wstack-protector -Wpointer-sign

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tjr, Assigned: Nomis101)

Tracking

(Blocks 1 bug, {sec-want})

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [sg:want][adv-main55-])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
This bug was created as a clone of Bug 620058 which contains more context.
(Reporter)

Updated

2 years ago
Depends on: 1359918
(Reporter)

Updated

2 years ago
Depends on: 1359920
(Reporter)

Updated

2 years ago
Depends on: 1359926
(Reporter)

Updated

2 years ago
Summary: Add Security Warning Flags: -Wformat -Wformat-security -Werror=format-security → Add Security Warning Flags: -Wformat -Wformat-security -Werror=format-security -Wstack-protector -Wpointer-sign
(Reporter)

Updated

2 years ago
Depends on: 1359928
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.
(Reporter)

Updated

2 years ago
Depends on: 1360299
(Reporter)

Updated

2 years ago
Depends on: 1360300
(Reporter)

Updated

2 years ago
Depends on: 1360301
(Reporter)

Updated

2 years ago
No longer depends on: 1359918
(Reporter)

Updated

2 years ago
No longer depends on: 1360301
(Reporter)

Updated

2 years ago
No longer depends on: 1360300
(Reporter)

Updated

2 years ago
No longer depends on: 1360299
(Reporter)

Updated

2 years ago
No longer blocks: 1359905
(Reporter)

Updated

2 years ago
No longer depends on: 1359928
(Reporter)

Updated

2 years ago
No longer depends on: 1359920
(Reporter)

Updated

2 years ago
No longer depends on: 1359926
(Reporter)

Updated

2 years ago
No longer depends on: 725941, 620058, 671426
(Assignee)

Comment 2

2 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
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
Assignee: nobody → Nomis101
Attachment #8869205 - Flags: feedback?(nfroyd)
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.
Attachment #8869205 - Flags: feedback?(nfroyd) → feedback+
(Assignee)

Comment 4

2 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
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
Attachment #8869205 - Attachment is obsolete: true
Attachment #8872764 - Flags: review?(nfroyd)
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?
Attachment #8872764 - Flags: review?(nfroyd) → review+
(Reporter)

Comment 6

2 years ago
(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.
(Assignee)

Comment 9

2 years ago
Patch with review comments addressed.
Attachment #8872764 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 10

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

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/aea06724da78
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Whiteboard: [sg:want] → [sg:want][adv-main55-]
You need to log in before you can comment on or make changes to this bug.