[PATCH 1/2] security_flags: turn potential string format security issues into an error

[PATCH 1/2] security_flags: turn potential string format security issues into an error

[Joshua Lock]

Add "-Wformat -Wformat-security -Werror=format-security" to the default
SECURITY_CFLAGS to catch potential security vulnerabilities due to the
misuse of various string formatting functions.

These flags are widely used in distributions such as Fedora and Ubuntu,
however we have 15 recipes in OE-Core which fail to build with these
flags included and thus the flags are removed for:
- busybox
- console-tools
- cmake
- expect
- gcc
- gettext
- kexec-tools
- leafpad
- libuser
- ltp
- makedevs
- oh-puzzles
- stat
- unzip
- zip

[YOCTO #9488]

Signed-off-by: Joshua Lock <joshua.g.lock@intel.com>
---
 meta/conf/distro/include/security_flags.inc | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/meta/conf/distro/include/security_flags.inc b/meta/conf/distro/include/security_flags.inc
index ff5f34e..5755d28 100644
--- a/meta/conf/distro/include/security_flags.inc
+++ b/meta/conf/distro/include/security_flags.inc
@@ -9,8 +9,11 @@
 # -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}"
+# Error on use of format strings that represent possible security problems
+SECURITY_STRINGFORMAT ?= "-Wformat -Wformat-security -Werror=format-security"
+
+SECURITY_CFLAGS ?= "-fstack-protector-strong -pie -fpie ${lcl_maybe_fortify} ${SECURITY_STRINGFORMAT}"
+SECURITY_NO_PIE_CFLAGS ?= "-fstack-protector-strong ${lcl_maybe_fortify} ${SECURITY_STRINGFORMAT}"
 
 SECURITY_LDFLAGS ?= "-fstack-protector-strong -Wl,-z,relro,-z,now"
 SECURITY_X_LDFLAGS ?= "-fstack-protector-strong -Wl,-z,relro"
@@ -92,6 +95,23 @@ SECURITY_CFLAGS_pn-zlib = "${SECURITY_NO_PIE_CFLAGS}"
 SECURITY_CFLAGS_pn-ltp = "${SECURITY_NO_PIE_CFLAGS}"
 SECURITY_CFLAGS_pn-pulseaudio = "${SECURITY_NO_PIE_CFLAGS}"
 
+# Recipes which fail to compile when elevating -Wformat-security to an error
+SECURITY_STRINGFORMAT_pn-busybox = ""
+SECURITY_STRINGFORMAT_pn-console-tools = ""
+SECURITY_STRINGFORMAT_pn-cmake = ""
+SECURITY_STRINGFORMAT_pn-expect = ""
+SECURITY_STRINGFORMAT_pn-gcc = ""
+SECURITY_STRINGFORMAT_pn-gettext = ""
+SECURITY_STRINGFORMAT_pn-kexec-tools = ""
+SECURITY_STRINGFORMAT_pn-leafpad = ""
+SECURITY_STRINGFORMAT_pn-libuser = ""
+SECURITY_STRINGFORMAT_pn-ltp = ""
+SECURITY_STRINGFORMAT_pn-makedevs = ""
+SECURITY_STRINGFORMAT_pn-oh-puzzles = ""
+SECURITY_STRINGFORMAT_pn-stat = ""
+SECURITY_STRINGFORMAT_pn-unzip = ""
+SECURITY_STRINGFORMAT_pn-zip = ""
+
 TARGET_CFLAGS_append_class-target = " ${SECURITY_CFLAGS}"
 TARGET_LDFLAGS_append_class-target = " ${SECURITY_LDFLAGS}"
 
-- 
2.5.5

[PATCH 2/2] packagegroup-core-lsb: fix whitespace in meta-qt* warnings

[Joshua Lock]

Without these extra space characters the messages are ill-formatted, i.e:
'The meta-qt3 layer should be added, this layer provides Qt 3.xlibraries.
Its intended use is for passing LSB tests as Qt3 isa requirement for LSB.'

Changes to:
'The meta-qt3 layer should be added, this layer provides Qt 3.x libraries.
Its intended use is for passing LSB tests as Qt3 is a requirement for LSB.'

Signed-off-by: Joshua Lock <joshua.g.lock@intel.com>
---
 meta/recipes-extended/packagegroups/packagegroup-core-lsb.bb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/meta/recipes-extended/packagegroups/packagegroup-core-lsb.bb b/meta/recipes-extended/packagegroups/packagegroup-core-lsb.bb
index a29c3d7..fb533ce 100644
--- a/meta/recipes-extended/packagegroups/packagegroup-core-lsb.bb
+++ b/meta/recipes-extended/packagegroups/packagegroup-core-lsb.bb
@@ -206,8 +206,8 @@ def get_libqt3(d):
         if 'qt3' in d.getVar('BBFILE_COLLECTIONS', False) or "":
             return 'libqt-mt3'
 
-        bb.warn('The meta-qt3 layer should be added, this layer provides Qt 3.x' \
-                'libraries. Its intended use is for passing LSB tests as Qt3 is' \
+        bb.warn('The meta-qt3 layer should be added, this layer provides Qt 3.x ' \
+                'libraries. Its intended use is for passing LSB tests as Qt3 is ' \
                 'a requirement for LSB.')
     return ''
 
@@ -229,8 +229,8 @@ def get_libqt4(d):
         if 'qt4' in d.getVar('BBFILE_COLLECTIONS', False) or "":
             return d.getVar('QT4PKGS', False)
 
-        bb.warn('The meta-qt4 layer should be added, this layer provides Qt 4.x' \
-                'libraries. Its intended use is for passing LSB tests as Qt4 is' \
+        bb.warn('The meta-qt4 layer should be added, this layer provides Qt 4.x ' \
+                'libraries. Its intended use is for passing LSB tests as Qt4 is ' \
                 'a requirement for LSB.')
     return ''
 
-- 
2.5.5

Re: [PATCH 1/2] security_flags: turn potential string format security issues into an error

[Khem Raj]

[-- Attachment #1: Type: text/plain, Size: 1758 bytes --]


> On Apr 28, 2016, at 6:27 AM, Joshua Lock <joshua.g.lock@intel.com> wrote:
> 
> -SECURITY_CFLAGS ?= "-fstack-protector-strong -pie -fpie ${lcl_maybe_fortify}"
> -SECURITY_NO_PIE_CFLAGS ?= "-fstack-protector-strong ${lcl_maybe_fortify}"
> +# Error on use of format strings that represent possible security problems
> +SECURITY_STRINGFORMAT ?= "-Wformat -Wformat-security -Werror=format-security"
> +
> +SECURITY_CFLAGS ?= "-fstack-protector-strong -pie -fpie ${lcl_maybe_fortify} ${SECURITY_STRINGFORMAT}"
> +SECURITY_NO_PIE_CFLAGS ?= "-fstack-protector-strong ${lcl_maybe_fortify} ${SECURITY_STRINGFORMAT}"
> 
> SECURITY_LDFLAGS ?= "-fstack-protector-strong -Wl,-z,relro,-z,now"
> SECURITY_X_LDFLAGS ?= "-fstack-protector-strong -Wl,-z,relro"
> @@ -92,6 +95,23 @@ SECURITY_CFLAGS_pn-zlib = "${SECURITY_NO_PIE_CFLAGS}"
> SECURITY_CFLAGS_pn-ltp = "${SECURITY_NO_PIE_CFLAGS}"
> SECURITY_CFLAGS_pn-pulseaudio = "${SECURITY_NO_PIE_CFLAGS}"
> 
> +# Recipes which fail to compile when elevating -Wformat-security to an error
> +SECURITY_STRINGFORMAT_pn-busybox = ""
> +SECURITY_STRINGFORMAT_pn-console-tools = ""
> +SECURITY_STRINGFORMAT_pn-cmake = ""
> +SECURITY_STRINGFORMAT_pn-expect = ""
> +SECURITY_STRINGFORMAT_pn-gcc = ""
> +SECURITY_STRINGFORMAT_pn-gettext = ""
> +SECURITY_STRINGFORMAT_pn-kexec-tools = ""
> +SECURITY_STRINGFORMAT_pn-leafpad = ""
> +SECURITY_STRINGFORMAT_pn-libuser = ""
> +SECURITY_STRINGFORMAT_pn-ltp = ""
> +SECURITY_STRINGFORMAT_pn-makedevs = ""
> +SECURITY_STRINGFORMAT_pn-oh-puzzles = ""
> +SECURITY_STRINGFORMAT_pn-stat = ""
> +SECURITY_STRINGFORMAT_pn-unzip = ""
> +SECURITY_STRINGFORMAT_pn-zip = ""

Can we use _remove operation instead of introducing a new variable and emptying it out here.

[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 211 bytes --]

Re: [PATCH 1/2] security_flags: turn potential string format security issues into an error

[Richard Purdie]

On Thu, 2016-04-28 at 08:58 -0700, Khem Raj wrote:
> > On Apr 28, 2016, at 6:27 AM, Joshua Lock <joshua.g.lock@intel.com>
> > wrote:
> > 
> > -SECURITY_CFLAGS ?= "-fstack-protector-strong -pie -fpie
> > ${lcl_maybe_fortify}"
> > -SECURITY_NO_PIE_CFLAGS ?= "-fstack-protector-strong
> > ${lcl_maybe_fortify}"
> > +# Error on use of format strings that represent possible security
> > problems
> > +SECURITY_STRINGFORMAT ?= "-Wformat -Wformat-security 
> > -Werror=format-security"
> > +
> > +SECURITY_CFLAGS ?= "-fstack-protector-strong -pie -fpie
> > ${lcl_maybe_fortify} ${SECURITY_STRINGFORMAT}"
> > +SECURITY_NO_PIE_CFLAGS ?= "-fstack-protector-strong
> > ${lcl_maybe_fortify} ${SECURITY_STRINGFORMAT}"
> > 
> > SECURITY_LDFLAGS ?= "-fstack-protector-strong -Wl,-z,relro,-z,now"
> > SECURITY_X_LDFLAGS ?= "-fstack-protector-strong -Wl,-z,relro"
> > @@ -92,6 +95,23 @@ SECURITY_CFLAGS_pn-zlib =
> > "${SECURITY_NO_PIE_CFLAGS}"
> > SECURITY_CFLAGS_pn-ltp = "${SECURITY_NO_PIE_CFLAGS}"
> > SECURITY_CFLAGS_pn-pulseaudio = "${SECURITY_NO_PIE_CFLAGS}"
> > 
> > +# Recipes which fail to compile when elevating -Wformat-security
> > to an error
> > +SECURITY_STRINGFORMAT_pn-busybox = ""
> > +SECURITY_STRINGFORMAT_pn-console-tools = ""
> > +SECURITY_STRINGFORMAT_pn-cmake = ""
> > +SECURITY_STRINGFORMAT_pn-expect = ""
> > +SECURITY_STRINGFORMAT_pn-gcc = ""
> > +SECURITY_STRINGFORMAT_pn-gettext = ""
> > +SECURITY_STRINGFORMAT_pn-kexec-tools = ""
> > +SECURITY_STRINGFORMAT_pn-leafpad = ""
> > +SECURITY_STRINGFORMAT_pn-libuser = ""
> > +SECURITY_STRINGFORMAT_pn-ltp = ""
> > +SECURITY_STRINGFORMAT_pn-makedevs = ""
> > +SECURITY_STRINGFORMAT_pn-oh-puzzles = ""
> > +SECURITY_STRINGFORMAT_pn-stat = ""
> > +SECURITY_STRINGFORMAT_pn-unzip = ""
> > +SECURITY_STRINGFORMAT_pn-zip = ""
> 
> Can we use _remove operation instead of introducing a new variable
> and emptying it out here.

I actually suggested we do the above. The reason is that this way, the
user can configure which flags they actually want to use. "remove" also
has the problem that its near impossible for the user to override
further.

I'm starting to believe that remove usage in OE-Core itself is actually
symptomatic of a problem and that if we end up using it, it probably
should be done differently.

Cheers,

Richard

Re: [PATCH 1/2] security_flags: turn potential string format security issues into an error

[Khem Raj]

[-- Attachment #1: Type: text/plain, Size: 2677 bytes --]


> On Apr 28, 2016, at 9:22 AM, Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
> 
> On Thu, 2016-04-28 at 08:58 -0700, Khem Raj wrote:
>>> On Apr 28, 2016, at 6:27 AM, Joshua Lock <joshua.g.lock@intel.com>
>>> wrote:
>>> 
>>> -SECURITY_CFLAGS ?= "-fstack-protector-strong -pie -fpie
>>> ${lcl_maybe_fortify}"
>>> -SECURITY_NO_PIE_CFLAGS ?= "-fstack-protector-strong
>>> ${lcl_maybe_fortify}"
>>> +# Error on use of format strings that represent possible security
>>> problems
>>> +SECURITY_STRINGFORMAT ?= "-Wformat -Wformat-security
>>> -Werror=format-security"
>>> +
>>> +SECURITY_CFLAGS ?= "-fstack-protector-strong -pie -fpie
>>> ${lcl_maybe_fortify} ${SECURITY_STRINGFORMAT}"
>>> +SECURITY_NO_PIE_CFLAGS ?= "-fstack-protector-strong
>>> ${lcl_maybe_fortify} ${SECURITY_STRINGFORMAT}"
>>> 
>>> SECURITY_LDFLAGS ?= "-fstack-protector-strong -Wl,-z,relro,-z,now"
>>> SECURITY_X_LDFLAGS ?= "-fstack-protector-strong -Wl,-z,relro"
>>> @@ -92,6 +95,23 @@ SECURITY_CFLAGS_pn-zlib =
>>> "${SECURITY_NO_PIE_CFLAGS}"
>>> SECURITY_CFLAGS_pn-ltp = "${SECURITY_NO_PIE_CFLAGS}"
>>> SECURITY_CFLAGS_pn-pulseaudio = "${SECURITY_NO_PIE_CFLAGS}"
>>> 
>>> +# Recipes which fail to compile when elevating -Wformat-security
>>> to an error
>>> +SECURITY_STRINGFORMAT_pn-busybox = ""
>>> +SECURITY_STRINGFORMAT_pn-console-tools = ""
>>> +SECURITY_STRINGFORMAT_pn-cmake = ""
>>> +SECURITY_STRINGFORMAT_pn-expect = ""
>>> +SECURITY_STRINGFORMAT_pn-gcc = ""
>>> +SECURITY_STRINGFORMAT_pn-gettext = ""
>>> +SECURITY_STRINGFORMAT_pn-kexec-tools = ""
>>> +SECURITY_STRINGFORMAT_pn-leafpad = ""
>>> +SECURITY_STRINGFORMAT_pn-libuser = ""
>>> +SECURITY_STRINGFORMAT_pn-ltp = ""
>>> +SECURITY_STRINGFORMAT_pn-makedevs = ""
>>> +SECURITY_STRINGFORMAT_pn-oh-puzzles = ""
>>> +SECURITY_STRINGFORMAT_pn-stat = ""
>>> +SECURITY_STRINGFORMAT_pn-unzip = ""
>>> +SECURITY_STRINGFORMAT_pn-zip = ""
>> 
>> Can we use _remove operation instead of introducing a new variable
>> and emptying it out here.
> 
> I actually suggested we do the above. The reason is that this way, the
> user can configure which flags they actually want to use. "remove" also
> has the problem that its near impossible for the user to override
> further.
> 

Thats right, and I was of the view that security flags should be one set
and not offered at combination of multiple options, we just end up increasing
the test matrix.

> I'm starting to believe that remove usage in OE-Core itself is actually
> symptomatic of a problem and that if we end up using it, it probably
> should be done differently.
> 

I believe _remove has the potential to be abused yes

[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 211 bytes --]

Re: [PATCH 1/2] security_flags: turn potential string format security issues into an error

[Richard Purdie]

On Thu, 2016-04-28 at 09:35 -0700, Khem Raj wrote:
> > On Apr 28, 2016, at 9:22 AM, Richard Purdie <
> > richard.purdie@linuxfoundation.org> wrote:
> > 
> > On Thu, 2016-04-28 at 08:58 -0700, Khem Raj wrote:

> > > Can we use _remove operation instead of introducing a new
> > > variable
> > > and emptying it out here.
> > 
> > I actually suggested we do the above. The reason is that this way,
> > the
> > user can configure which flags they actually want to use. "remove"
> > also
> > has the problem that its near impossible for the user to override
> > further.
> > 
> 
> Thats right, and I was of the view that security flags should be one
> set
> and not offered at combination of multiple options, we just end up
> increasing
> the test matrix.

OE-Core will continue to test with all of them, I think its better
thantpeople can disable part of this, than have to disable everything
for their layer though?

Cheers,

Richard

Re: [PATCH 1/2] security_flags: turn potential string format security issues into an error

[Khem Raj]

[-- Attachment #1: Type: text/plain, Size: 1127 bytes --]


> On Apr 28, 2016, at 9:39 AM, Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
> 
> On Thu, 2016-04-28 at 09:35 -0700, Khem Raj wrote:
>>> On Apr 28, 2016, at 9:22 AM, Richard Purdie <
>>> richard.purdie@linuxfoundation.org> wrote:
>>> 
>>> On Thu, 2016-04-28 at 08:58 -0700, Khem Raj wrote:
> 
>>>> Can we use _remove operation instead of introducing a new
>>>> variable
>>>> and emptying it out here.
>>> 
>>> I actually suggested we do the above. The reason is that this way,
>>> the
>>> user can configure which flags they actually want to use. "remove"
>>> also
>>> has the problem that its near impossible for the user to override
>>> further.
>>> 
>> 
>> Thats right, and I was of the view that security flags should be one
>> set
>> and not offered at combination of multiple options, we just end up
>> increasing
>> the test matrix.
> 
> OE-Core will continue to test with all of them, I think its better
> thantpeople can disable part of this, than have to disable everything
> for their layer though?
> 

I see your viewpoint. Mine was to not offer that option at all.


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 211 bytes --]