Bug 9488

Why these flags in particular, and not -Wall -Wextra, or a broader subset of them?

Also, these should be enabled only after agreeing who will fix the resulting errors and send the fixes upstream: it's not a small task. Enabling these warnings and then ignoring them is worse than not enabling them at all.

These flags do check for quite specific security issues which is why I think they're being suggested. I do share the concerns about how much work is potentially involved in this :/.

I am trying to enable them now in Ostro scope: https://github.com/ostroproject/meta-ostro/pull/116/commits/a2779f89fc7f92b674053237326479352fd79c4f

That touches also in my understanding oe core packages that are used in Ostro. So far I found only small amount of packages that need exceptions, so it might not be that bad as it seems.

So can you enable it for oe-core, issue 'bitbake world' and make a list of what starts to fail? It would be good to have some data, before making a decision of whether we proceed with this (against other things on our todo list).

(In reply to comment #4)
> So can you enable it for oe-core, issue 'bitbake world' and make a list of
> what starts to fail? It would be good to have some data, before making a
> decision of whether we proceed with this (against other things on our todo
> list).

I did a `bitbake world -k` in poky with DISTRO = "poky-lsb" (because that uses security_flags) and the following diff applied to my poky tree:

diff --git a/meta/conf/distro/include/security_flags.inc b/meta/conf/distro/include/security_flags.inc
index ff5f34e..1a50d6f 100644
--- a/meta/conf/distro/include/security_flags.inc
+++ b/meta/conf/distro/include/security_flags.inc
@@ -9,8 +9,8 @@
 # -O0 which then results in a compiler warning.
 lcl_maybe_fortify = "${@base_conditional('DEBUG_BUILD','1','','-D_FORTIFY_SOURCE=2',d)}"
 
-SECURITY_CFLAGS ?= "-fstack-protector-strong -pie -fpie ${lcl_maybe_fortify}"
-SECURITY_NO_PIE_CFLAGS ?= "-fstack-protector-strong ${lcl_maybe_fortify}"
+SECURITY_CFLAGS ?= "-fstack-protector-strong -pie -fpie ${lcl_maybe_fortify} -Wformat -Wformat-security"
+SECURITY_NO_PIE_CFLAGS ?= "-fstack-protector-strong ${lcl_maybe_fortify} -Wformat -Wformat-security"
 
 SECURITY_LDFLAGS ?= "-fstack-protector-strong -Wl,-z,relro,-z,now"
 SECURITY_X_LDFLAGS ?= "-fstack-protector-strong -Wl,-z,relro"

and nothing failed. I know, I too am a little shocked.

That's because it's only a warning, and will not make the build fail.
You need to use -Werror= to turn it into an error. 
https://gcc.gnu.org/onlinedocs/gcc-4.3.1/gcc/Warning-Options.html

Elena's Ostro patch has the same problem I believe, not sure how she found which packages need to be excepted from the check.

Of course, thanks Alexander.

I tested a world build with "-Wformat -Wformat-security -Werror=format-security" added to the SECURITY_CFLAGS.

13 recipes failed to build and I was able to complete a world build by removing the flags for the recipes in question, i.e.:

SECURITY_CFLAGS_remove_pn-libuser = "-Wformat -Wformat-security -Werror=format-security"

The failing recipes are:
- busybox
- console-tools
- cmake
- expect
- gcc
- gettext
- kexec-tools
- leafpad
- libuser
- ltp
- makedevs
- oh-puzzles
- stat

I haven't begun to look at how much work fixing those recipes is, or if we get that work for free in an upgrade. 
I did discover that these are amongst the default flags in Fedora[1][2] (since ) and Ubuntu[3] and part of the Debian hardening options[4], so there may be patches floating around.

1. https://fedoraproject.org/wiki/Format-Security-FAQ
2. https://fedorahosted.org/fesco/ticket/1185
3. https://wiki.ubuntu.com/ToolChain/CompilerFlags
4. https://wiki.debian.org/Hardening#DEB_BUILD_HARDENING_FORMAT_.28gcc.2Fg.2B-.2B-_-Wformat_-Wformat-security_-Werror.3Dformat-security.29

(In reply to comment #7)

Documenting what I mentioned in chat already:

> - leafpad

I'm currently in process of updating oe-core to GTK+3 (at least for things in core-image-sato). This almost certainly includes getting rid of leafpad.

A possible replacement is l3afpad which is a fork and might still have the same problems.

> - oh-puzzles

oh-puzzles will be removed (we already have "puzzles" in oe-core and according to your test that doesn't have this problem).

I've filed separate bugs (assigned to me) to resolve the string formatting issues in all of the identified recipes apart from Leafpad (where we should do the equivalent work on whatever replaces it, if required) and oh-puzzles (which will be removed).

Patch submitted to OE-Core which elevates string-formatting issues to an error for all but 15 recipes. In addition to the 13 recipes mentioned earlier I also found zip and unzip fail to compile with -Werror=format-security

http://lists.openembedded.org/pipermail/openembedded-core/2016-April/120753.html

(In reply to comment #6)
> That's because it's only a warning, and will not make the build fail.
> You need to use -Werror= to turn it into an error. 
> https://gcc.gnu.org/onlinedocs/gcc-4.3.1/gcc/Warning-Options.html
> 
> Elena's Ostro patch has the same problem I believe, not sure how she found
> which packages need to be excepted from the check.

Some packages turn Werror by default, this is how I found it. Having it as a warning it is a fist step.

(In reply to comment #9)
> I've filed separate bugs (assigned to me) to resolve the string formatting
> issues in all of the identified recipes apart from Leafpad (where we should
> do the equivalent work on whatever replaces it, if required) and oh-puzzles
> (which will be removed).

Thank you very much Joshua!

(In reply to comment #11)
> Some packages turn Werror by default, this is how I found it. Having it as a
> warning it is a fist step.

Adding more compilation warnings is not useful in the context of openembedded build system. There are already hundreds of such warnings written into compile logs when packages are built, and nobody is going look at those logs until something fails.

(In reply to comment #13)
> (In reply to comment #11)
> > Some packages turn Werror by default, this is how I found it. Having it as a
> > warning it is a fist step.
> 
> Adding more compilation warnings is not useful in the context of
> openembedded build system. There are already hundreds of such warnings
> written into compile logs when packages are built, and nobody is going look
> at those logs until something fails.

Sure, but now it is enabled with Werror, so I don't think we have an issue here. When the patch done by Joshua is merged, we can close this bug and track only separate components bugs. 

Joshua, could you please add me as a watcher to separate bugs?

(In reply to comment #14)
> Joshua, could you please add me as a watcher to separate bugs?

Done.

Patch to add format string vulnerability prevention flags to SECURITY_CFLAGS has been merged:

http://git.openembedded.org/openembedded-core/commit/?id=f335f8e744fb312b3eb599c331d08a9a6e5a8ff8

http://git.yoctoproject.org/cgit/cgit.cgi/poky/commit/?id=30f9a5072d94f6c383b1ef5d25448af39691dce0