[Buildroot] [PATCH 0/6] Hardening Flag Bugfix/Enhancement

[Buildroot] [PATCH 0/6] Hardening Flag Bugfix/Enhancement

[Matt Weber]

This series pulls together a few pending patches required for hardening
flag bug-fixes.  Additionally a tool is added with Buildroot test cases
to validate that the hardening options are working correctly.

Stefan S?ena
http://patchwork.ozlabs.org/patch/904057/  (Bugfix)
http://patchwork.ozlabs.org/patch/904034/  (Bugfix)


Matt Weber (Both have been marked as superseded)
http://patchwork.ozlabs.org/patch/907093/  (Bugfix)
http://patchwork.ozlabs.org/patch/932853/  (New checksec tool)

A unrelated patch was also included which adds proxy env support for the runtests script.

Matt Weber (2):
  support/testing: runtest proxy support
  support/testing/tests/core: SSP & hardening flags

Paresh Chaudhary (1):
  package/checksec: new package

Stefan S?rensen (3):
  package/Makefile.in: Do not use CPPFLAGS for hardening options
  package/Makefile.in: Add missing options to LDFLAGS for full RELRO
    build
  package/Makefile.in: Use gcc spec files for PIE build flags

 package/Config.in.host                        |   1 +
 package/Makefile.in                           |  18 +--
 ...cksec-Fixed-issue-with-relative-path.patch |  43 ++++++++
 package/checksec/Config.in.host               |  16 +++
 package/checksec/checksec.hash                |   3 +
 package/checksec/checksec.mk                  |  16 +++
 support/testing/infra/builder.py              |   6 +
 support/testing/tests/core/test_hardening.py  | 104 ++++++++++++++++++
 toolchain/gcc-specs-pie-cc1                   |   2 +
 toolchain/gcc-specs-pie-ld                    |   2 +
 10 files changed, 202 insertions(+), 9 deletions(-)
 create mode 100644 package/checksec/0001-checksec-Fixed-issue-with-relative-path.patch
 create mode 100644 package/checksec/Config.in.host
 create mode 100644 package/checksec/checksec.hash
 create mode 100644 package/checksec/checksec.mk
 create mode 100644 support/testing/tests/core/test_hardening.py
 create mode 100644 toolchain/gcc-specs-pie-cc1
 create mode 100644 toolchain/gcc-specs-pie-ld

-- 
2.17.0

[Buildroot] [PATCH 1/6] package/Makefile.in: Do not use CPPFLAGS for hardening options

[Matt Weber]

From: Stefan S?rensen <stefan.sorensen@spectralink.com>

The hardening options are compiler flags, not pure pre-processor flags, so
put them in CFLAGS, not CPPFLAGS.

This fixes build errors where -D_FORTIFY_SOURCE=2 whas put in CPPFLAGS and
then applied to configure tests which could fail since the required -O2 is
only in CFLAGS.

Originally submitted as
http://patchwork.ozlabs.org/patch/904057/

Signed-off-by: Stefan S?rensen <stefan.sorensen@spectralink.com>
Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
---
 package/Makefile.in | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/package/Makefile.in b/package/Makefile.in
index f2962767cc..5e0ff8c841 100644
--- a/package/Makefile.in
+++ b/package/Makefile.in
@@ -147,29 +147,29 @@ TARGET_CFLAGS_RELRO_FULL = -Wl,-z,now $(TARGET_CFLAGS_RELRO)
 TARGET_LDFLAGS = $(call qstrip,$(BR2_TARGET_LDFLAGS))
 
 ifeq ($(BR2_SSP_REGULAR),y)
-TARGET_CPPFLAGS += -fstack-protector
+TARGET_HARDENED += -fstack-protector
 else ifeq ($(BR2_SSP_STRONG),y)
-TARGET_CPPFLAGS += -fstack-protector-strong
+TARGET_HARDENED += -fstack-protector-strong
 else ifeq ($(BR2_SSP_ALL),y)
-TARGET_CPPFLAGS += -fstack-protector-all
+TARGET_HARDENED += -fstack-protector-all
 endif
 
 ifeq ($(BR2_RELRO_PARTIAL),y)
-TARGET_CPPFLAGS += $(TARGET_CFLAGS_RELRO)
+TARGET_HARDENED += $(TARGET_CFLAGS_RELRO)
 TARGET_LDFLAGS += $(TARGET_CFLAGS_RELRO)
 else ifeq ($(BR2_RELRO_FULL),y)
-TARGET_CPPFLAGS += -fPIE $(TARGET_CFLAGS_RELRO_FULL)
+TARGET_HARDENED += -fPIE $(TARGET_CFLAGS_RELRO_FULL)
 TARGET_LDFLAGS += -pie
 endif
 
 ifeq ($(BR2_FORTIFY_SOURCE_1),y)
-TARGET_CPPFLAGS += -D_FORTIFY_SOURCE=1
+TARGET_HARDENED += -D_FORTIFY_SOURCE=1
 else ifeq ($(BR2_FORTIFY_SOURCE_2),y)
-TARGET_CPPFLAGS += -D_FORTIFY_SOURCE=2
+TARGET_HARDENED += -D_FORTIFY_SOURCE=2
 endif
 
 TARGET_CPPFLAGS += -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
-TARGET_CFLAGS = $(TARGET_CPPFLAGS) $(TARGET_ABI) $(TARGET_OPTIMIZATION) $(TARGET_DEBUGGING)
+TARGET_CFLAGS = $(TARGET_CPPFLAGS) $(TARGET_ABI) $(TARGET_OPTIMIZATION) $(TARGET_DEBUGGING) $(TARGET_HARDENED)
 TARGET_CXXFLAGS = $(TARGET_CFLAGS)
 TARGET_FCFLAGS = $(TARGET_ABI) $(TARGET_OPTIMIZATION) $(TARGET_DEBUGGING)
 
-- 
2.17.0

[Buildroot] [PATCH 1/6] package/Makefile.in: Do not use CPPFLAGS for hardening options

[Arnout Vandecappelle]

On 11-07-18 16:31, Matt Weber wrote:
> From: Stefan S?rensen <stefan.sorensen@spectralink.com>
> 
> The hardening options are compiler flags, not pure pre-processor flags, so
> put them in CFLAGS, not CPPFLAGS.
> 
> This fixes build errors where -D_FORTIFY_SOURCE=2 whas put in CPPFLAGS and
> then applied to configure tests which could fail since the required -O2 is
> only in CFLAGS.
> 
> Originally submitted as
> http://patchwork.ozlabs.org/patch/904057/
> 
> Signed-off-by: Stefan S?rensen <stefan.sorensen@spectralink.com>
> Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>

 I was thinking: why introduce TARGET_HARDENED instead of just adding to
TARGET_CFLAGS directly. But it actually does look nicer this way. So

Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

> ---
>  package/Makefile.in | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/package/Makefile.in b/package/Makefile.in
> index f2962767cc..5e0ff8c841 100644
> --- a/package/Makefile.in
> +++ b/package/Makefile.in
> @@ -147,29 +147,29 @@ TARGET_CFLAGS_RELRO_FULL = -Wl,-z,now $(TARGET_CFLAGS_RELRO)
>  TARGET_LDFLAGS = $(call qstrip,$(BR2_TARGET_LDFLAGS))
>  
>  ifeq ($(BR2_SSP_REGULAR),y)
> -TARGET_CPPFLAGS += -fstack-protector
> +TARGET_HARDENED += -fstack-protector
>  else ifeq ($(BR2_SSP_STRONG),y)
> -TARGET_CPPFLAGS += -fstack-protector-strong
> +TARGET_HARDENED += -fstack-protector-strong
>  else ifeq ($(BR2_SSP_ALL),y)
> -TARGET_CPPFLAGS += -fstack-protector-all
> +TARGET_HARDENED += -fstack-protector-all
>  endif
>  
>  ifeq ($(BR2_RELRO_PARTIAL),y)
> -TARGET_CPPFLAGS += $(TARGET_CFLAGS_RELRO)
> +TARGET_HARDENED += $(TARGET_CFLAGS_RELRO)
>  TARGET_LDFLAGS += $(TARGET_CFLAGS_RELRO)
>  else ifeq ($(BR2_RELRO_FULL),y)
> -TARGET_CPPFLAGS += -fPIE $(TARGET_CFLAGS_RELRO_FULL)
> +TARGET_HARDENED += -fPIE $(TARGET_CFLAGS_RELRO_FULL)
>  TARGET_LDFLAGS += -pie
>  endif
>  
>  ifeq ($(BR2_FORTIFY_SOURCE_1),y)
> -TARGET_CPPFLAGS += -D_FORTIFY_SOURCE=1
> +TARGET_HARDENED += -D_FORTIFY_SOURCE=1
>  else ifeq ($(BR2_FORTIFY_SOURCE_2),y)
> -TARGET_CPPFLAGS += -D_FORTIFY_SOURCE=2
> +TARGET_HARDENED += -D_FORTIFY_SOURCE=2
>  endif
>  
>  TARGET_CPPFLAGS += -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
> -TARGET_CFLAGS = $(TARGET_CPPFLAGS) $(TARGET_ABI) $(TARGET_OPTIMIZATION) $(TARGET_DEBUGGING)
> +TARGET_CFLAGS = $(TARGET_CPPFLAGS) $(TARGET_ABI) $(TARGET_OPTIMIZATION) $(TARGET_DEBUGGING) $(TARGET_HARDENED)

 The line is getting a bit long, but TARGET_DEBUGGING was already too much so
not for this patch :-)


 Regards,
 Arnout

>  TARGET_CXXFLAGS = $(TARGET_CFLAGS)
>  TARGET_FCFLAGS = $(TARGET_ABI) $(TARGET_OPTIMIZATION) $(TARGET_DEBUGGING)
>  
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 1/6] package/Makefile.in: Do not use CPPFLAGS for hardening options

[Thomas Petazzoni]

Hello,

On Wed, 11 Jul 2018 09:31:08 -0500, Matt Weber wrote:
> From: Stefan S?rensen <stefan.sorensen@spectralink.com>
> 
> The hardening options are compiler flags, not pure pre-processor flags, so
> put them in CFLAGS, not CPPFLAGS.
> 
> This fixes build errors where -D_FORTIFY_SOURCE=2 whas put in CPPFLAGS and
> then applied to configure tests which could fail since the required -O2 is
> only in CFLAGS.
> 
> Originally submitted as
> http://patchwork.ozlabs.org/patch/904057/
> 
> Signed-off-by: Stefan S?rensen <stefan.sorensen@spectralink.com>
> Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
> ---
>  package/Makefile.in | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

Applied to next, thanks.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 2/6] package/Makefile.in: Add missing options to LDFLAGS for full RELRO build

[Matt Weber]

From: Stefan S?rensen <stefan.sorensen@spectralink.com>

The options for a full RELRO build should also be added to LDFLAGS.

Originally submitted as
http://patchwork.ozlabs.org/patch/904034/

Signed-off-by: Stefan S?rensen <stefan.sorensen@spectralink.com>
Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com>
---
 package/Makefile.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package/Makefile.in b/package/Makefile.in
index 5e0ff8c841..14b3bbd243 100644
--- a/package/Makefile.in
+++ b/package/Makefile.in
@@ -159,7 +159,7 @@ TARGET_HARDENED += $(TARGET_CFLAGS_RELRO)
 TARGET_LDFLAGS += $(TARGET_CFLAGS_RELRO)
 else ifeq ($(BR2_RELRO_FULL),y)
 TARGET_HARDENED += -fPIE $(TARGET_CFLAGS_RELRO_FULL)
-TARGET_LDFLAGS += -pie
+TARGET_LDFLAGS += -pie $(TARGET_CFLAGS_RELRO_FULL)
 endif
 
 ifeq ($(BR2_FORTIFY_SOURCE_1),y)
-- 
2.17.0

[Buildroot] [PATCH 2/6] package/Makefile.in: Add missing options to LDFLAGS for full RELRO build

[Arnout Vandecappelle]

On 11-07-18 16:31, Matt Weber wrote:
> From: Stefan S?rensen <stefan.sorensen@spectralink.com>
> 
> The options for a full RELRO build should also be added to LDFLAGS.
> 
> Originally submitted as
> http://patchwork.ozlabs.org/patch/904034/
> 
> Signed-off-by: Stefan S?rensen <stefan.sorensen@spectralink.com>
> Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com>
> ---
>  package/Makefile.in | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/package/Makefile.in b/package/Makefile.in
> index 5e0ff8c841..14b3bbd243 100644
> --- a/package/Makefile.in
> +++ b/package/Makefile.in
> @@ -159,7 +159,7 @@ TARGET_HARDENED += $(TARGET_CFLAGS_RELRO)
>  TARGET_LDFLAGS += $(TARGET_CFLAGS_RELRO)
>  else ifeq ($(BR2_RELRO_FULL),y)
>  TARGET_HARDENED += -fPIE $(TARGET_CFLAGS_RELRO_FULL)
> -TARGET_LDFLAGS += -pie
> +TARGET_LDFLAGS += -pie $(TARGET_CFLAGS_RELRO_FULL)

 Actually, those flags (-Wl,-z,now,-z,relo) really are link-time flags only. So
really there is no reason why we would have them in TARGET_CFLAGS (i.e. in
TARGET_HARDENED). However, it is likely that there are packages that only apply
TARGET_CFLAGS to linking, not TARGET_LDFLAGS (we currently have no way of
knowing, since all of the LDFLAGS are also in CFLAGS, except for the hardening
ones).

 I have also considered to add TARGET_HARDENING to TARGET_LDFLAGS. However, it
is really only this -Wl,-z,... option which is relevant in LDFLAGS; the
-fstack-protector and -D_FORTIFY options can never have any effect. So this
looks good.

Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

 I have one little gripe, not directly related to this patch: I think the
variables TARGET_CFLAGS_RELRO and TARGET_CFLAGS_RELRO_FULL have little value,
they make IMO the code harder to read.

 Regards,
 Arnout
>  endif
>  
>  ifeq ($(BR2_FORTIFY_SOURCE_1),y)
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 2/6] package/Makefile.in: Add missing options to LDFLAGS for full RELRO build

[Thomas Petazzoni]

Hello,

On Wed, 11 Jul 2018 09:31:09 -0500, Matt Weber wrote:
> From: Stefan S?rensen <stefan.sorensen@spectralink.com>
> 
> The options for a full RELRO build should also be added to LDFLAGS.
> 
> Originally submitted as
> http://patchwork.ozlabs.org/patch/904034/
> 
> Signed-off-by: Stefan S?rensen <stefan.sorensen@spectralink.com>
> Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com>
> ---
>  package/Makefile.in | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to next, thanks.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Matt Weber]

From: Stefan S?rensen <stefan.sorensen@spectralink.com>

The PIE build flags are only intended for building executables and can not be
used in relocateable links (-r), static builds and shared library build -
including the flags here causes build errors.

So instead of parsing the PIE flags directly on the command line to gcc,
include them in a gcc spec file where it is possible to only apply the flags
when other incompatible flags are not set.

This method and the spec files are from the Fedora build system.

Originally submitted as
http://patchwork.ozlabs.org/patch/907093/

Signed-off-by: Stefan S?rensen <stefan.sorensen@spectralink.com>
Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com>
---
 package/Makefile.in         | 4 ++--
 toolchain/gcc-specs-pie-cc1 | 2 ++
 toolchain/gcc-specs-pie-ld  | 2 ++
 3 files changed, 6 insertions(+), 2 deletions(-)
 create mode 100644 toolchain/gcc-specs-pie-cc1
 create mode 100644 toolchain/gcc-specs-pie-ld

diff --git a/package/Makefile.in b/package/Makefile.in
index 14b3bbd243..00ddf48c10 100644
--- a/package/Makefile.in
+++ b/package/Makefile.in
@@ -158,8 +158,8 @@ ifeq ($(BR2_RELRO_PARTIAL),y)
 TARGET_HARDENED += $(TARGET_CFLAGS_RELRO)
 TARGET_LDFLAGS += $(TARGET_CFLAGS_RELRO)
 else ifeq ($(BR2_RELRO_FULL),y)
-TARGET_HARDENED += -fPIE $(TARGET_CFLAGS_RELRO_FULL)
-TARGET_LDFLAGS += -pie $(TARGET_CFLAGS_RELRO_FULL)
+TARGET_HARDENED += $(TARGET_CFLAGS_RELRO_FULL) -specs=$(TOPDIR)/toolchain/gcc-specs-pie-cc1
+TARGET_LDFLAGS += $(TARGET_CFLAGS_RELRO_FULL) -specs=$(TOPDIR)/toolchain/gcc-specs-pie-ld
 endif
 
 ifeq ($(BR2_FORTIFY_SOURCE_1),y)
diff --git a/toolchain/gcc-specs-pie-cc1 b/toolchain/gcc-specs-pie-cc1
new file mode 100644
index 0000000000..fc54bcb510
--- /dev/null
+++ b/toolchain/gcc-specs-pie-cc1
@@ -0,0 +1,2 @@
+*cc1_options:
++ %{!r:%{!fpie:%{!fPIE:%{!fpic:%{!fPIC:%{!fno-pic:-fPIE}}}}}}
diff --git a/toolchain/gcc-specs-pie-ld b/toolchain/gcc-specs-pie-ld
new file mode 100644
index 0000000000..bd6b9071ff
--- /dev/null
+++ b/toolchain/gcc-specs-pie-ld
@@ -0,0 +1,2 @@
+*self_spec:
++ %{!static:%{!shared:%{!r:-pie}}}
-- 
2.17.0

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Arnout Vandecappelle]

On 11-07-18 16:31, Matt Weber wrote:
> From: Stefan S?rensen <stefan.sorensen@spectralink.com>
> 
> The PIE build flags are only intended for building executables and can not be
> used in relocateable links (-r), static builds and shared library build -
> including the flags here causes build errors.
> 
> So instead of parsing the PIE flags directly on the command line to gcc,
> include them in a gcc spec file where it is possible to only apply the flags
> when other incompatible flags are not set.

 The commit message should also have the reasoning why this is considered better
than doing the same in the wrapper.

> 
> This method and the spec files are from the Fedora build system.
> 
> Originally submitted as
> http://patchwork.ozlabs.org/patch/907093/
> 
> Signed-off-by: Stefan S?rensen <stefan.sorensen@spectralink.com>
> Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com>
> ---
>  package/Makefile.in         | 4 ++--
>  toolchain/gcc-specs-pie-cc1 | 2 ++
>  toolchain/gcc-specs-pie-ld  | 2 ++
>  3 files changed, 6 insertions(+), 2 deletions(-)
>  create mode 100644 toolchain/gcc-specs-pie-cc1
>  create mode 100644 toolchain/gcc-specs-pie-ld
> 
> diff --git a/package/Makefile.in b/package/Makefile.in
> index 14b3bbd243..00ddf48c10 100644
> --- a/package/Makefile.in
> +++ b/package/Makefile.in
> @@ -158,8 +158,8 @@ ifeq ($(BR2_RELRO_PARTIAL),y)
>  TARGET_HARDENED += $(TARGET_CFLAGS_RELRO)
>  TARGET_LDFLAGS += $(TARGET_CFLAGS_RELRO)
>  else ifeq ($(BR2_RELRO_FULL),y)
> -TARGET_HARDENED += -fPIE $(TARGET_CFLAGS_RELRO_FULL)
> -TARGET_LDFLAGS += -pie $(TARGET_CFLAGS_RELRO_FULL)
> +TARGET_HARDENED += $(TARGET_CFLAGS_RELRO_FULL) -specs=$(TOPDIR)/toolchain/gcc-specs-pie-cc1
> +TARGET_LDFLAGS += $(TARGET_CFLAGS_RELRO_FULL) -specs=$(TOPDIR)/toolchain/gcc-specs-pie-ld

 As mentioned in my other mail, it is very likely that there are packages that
link with TARGET_CFLAGS instead of TARGET_LDFLAGS. Would that be a problem?
Actually, can't we just have a single spec file that contains both?


 Regards,
 Arnout

>  endif
>  
>  ifeq ($(BR2_FORTIFY_SOURCE_1),y)
> diff --git a/toolchain/gcc-specs-pie-cc1 b/toolchain/gcc-specs-pie-cc1
> new file mode 100644
> index 0000000000..fc54bcb510
> --- /dev/null
> +++ b/toolchain/gcc-specs-pie-cc1
> @@ -0,0 +1,2 @@
> +*cc1_options:
> ++ %{!r:%{!fpie:%{!fPIE:%{!fpic:%{!fPIC:%{!fno-pic:-fPIE}}}}}}
> diff --git a/toolchain/gcc-specs-pie-ld b/toolchain/gcc-specs-pie-ld
> new file mode 100644
> index 0000000000..bd6b9071ff
> --- /dev/null
> +++ b/toolchain/gcc-specs-pie-ld
> @@ -0,0 +1,2 @@
> +*self_spec:
> ++ %{!static:%{!shared:%{!r:-pie}}}


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Matthew Weber]

Arnout,

On Wed, Jul 11, 2018 at 4:44 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
>
>
> On 11-07-18 16:31, Matt Weber wrote:
>> From: Stefan S?rensen <stefan.sorensen@spectralink.com>
>>
>> The PIE build flags are only intended for building executables and can not be
>> used in relocateable links (-r), static builds and shared library build -
>> including the flags here causes build errors.
>>
>> So instead of parsing the PIE flags directly on the command line to gcc,
>> include them in a gcc spec file where it is possible to only apply the flags
>> when other incompatible flags are not set.
>
>  The commit message should also have the reasoning why this is considered better
> than doing the same in the wrapper.
>

It probably isn't better then doing it in the wrapper.  At this point,
its just fixing something that's broken with the current approach.  I
was assuming a solid baseline option with this feature that's working
first and then start looking at replacing the approach with the
suggestion awhile back with doing it via the wrapper.


>> -TARGET_HARDENED += -fPIE $(TARGET_CFLAGS_RELRO_FULL)
>> -TARGET_LDFLAGS += -pie $(TARGET_CFLAGS_RELRO_FULL)
>> +TARGET_HARDENED += $(TARGET_CFLAGS_RELRO_FULL) -specs=$(TOPDIR)/toolchain/gcc-specs-pie-cc1
>> +TARGET_LDFLAGS += $(TARGET_CFLAGS_RELRO_FULL) -specs=$(TOPDIR)/toolchain/gcc-specs-pie-ld
>
>  As mentioned in my other mail, it is very likely that there are packages that
> link with TARGET_CFLAGS instead of TARGET_LDFLAGS. Would that be a problem?
> Actually, can't we just have a single spec file that contains both?

Like you mentioned previously, flag usage isn't consistent.  When we
started doing validation of the elf files we found that inconsistency
and we were individually trying to fix the packages.  This didn't get
to far as lots of the packages are in stable, no real maintaining
occuring.  I'm glad Stefan brought up the spec file idea as that
solved all the rework on the linker ordering and combination of
values.

Related to using a single spec file.  I'm not to familiar with using
them but since we have a mix of good/bad flag options already
occurring, guessing it wouldn't matter to consolidate to one file.  I
can test going to a single file. Stefan, do you know?

Matt

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Arnout Vandecappelle]

On 12-07-18 01:17, Matthew Weber wrote:
> Arnout,
> 
> On Wed, Jul 11, 2018 at 4:44 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
>>
>>
>> On 11-07-18 16:31, Matt Weber wrote:
>>> From: Stefan S?rensen <stefan.sorensen@spectralink.com>
>>>
>>> The PIE build flags are only intended for building executables and can not be
>>> used in relocateable links (-r), static builds and shared library build -
>>> including the flags here causes build errors.
>>>
>>> So instead of parsing the PIE flags directly on the command line to gcc,
>>> include them in a gcc spec file where it is possible to only apply the flags
>>> when other incompatible flags are not set.
>>
>>  The commit message should also have the reasoning why this is considered better
>> than doing the same in the wrapper.
>>
> 
> It probably isn't better then doing it in the wrapper.  At this point,
> its just fixing something that's broken with the current approach.  I
> was assuming a solid baseline option with this feature that's working
> first and then start looking at replacing the approach with the
> suggestion awhile back with doing it via the wrapper.

 I'm a bit confused about where you'd like to go in the long run: spec file or
wrapper? In your reply to the cover letter you mention "I do like the concept of
changing to the wrapper and evaluating if we can use GCC spec files"...

 I guess the spec files have the advantage that it is a lot easier to specify
mutually exclusive options. We have that now in the wrapper for arch/cpu/tune,
but it's a lot of code to specify something simple. That said, spec files are so
arcane that you can't expect many people contributing to it. The C code, though
more verbose, is a lot more understandable.

 Back to the patch: basically, the same could be done in the wrapper, but here
the spec files are just taken from Fedora so that's a lot simpler. Right?


>>> -TARGET_HARDENED += -fPIE $(TARGET_CFLAGS_RELRO_FULL)
>>> -TARGET_LDFLAGS += -pie $(TARGET_CFLAGS_RELRO_FULL)
>>> +TARGET_HARDENED += $(TARGET_CFLAGS_RELRO_FULL) -specs=$(TOPDIR)/toolchain/gcc-specs-pie-cc1
>>> +TARGET_LDFLAGS += $(TARGET_CFLAGS_RELRO_FULL) -specs=$(TOPDIR)/toolchain/gcc-specs-pie-ld
>>
>>  As mentioned in my other mail, it is very likely that there are packages that
>> link with TARGET_CFLAGS instead of TARGET_LDFLAGS. Would that be a problem?

 I don't think you answered this question?

>> Actually, can't we just have a single spec file that contains both?
> 
> Like you mentioned previously, flag usage isn't consistent.  When we
> started doing validation of the elf files we found that inconsistency
> and we were individually trying to fix the packages.  This didn't get
> to far as lots of the packages are in stable, no real maintaining
> occuring.  I'm glad Stefan brought up the spec file idea as that
> solved all the rework on the linker ordering and combination of
> values.

 Yes, spec file (or wrapper) is definitely better than patching individual packages.


 Regards,
 Arnout

> Related to using a single spec file.  I'm not to familiar with using
> them but since we have a mix of good/bad flag options already
> occurring, guessing it wouldn't matter to consolidate to one file.  I
> can test going to a single file. Stefan, do you know?
> 
> Matt
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Matthew Weber]

Arnout,

On Fri, Jul 13, 2018 at 4:39 AM, Arnout Vandecappelle <arnout@mind.be> wrote:
>
>
> On 12-07-18 01:17, Matthew Weber wrote:
>> Arnout,
>>
>> On Wed, Jul 11, 2018 at 4:44 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
>>>
>>>
>>> On 11-07-18 16:31, Matt Weber wrote:
>>>> From: Stefan S?rensen <stefan.sorensen@spectralink.com>
>>>>
>>>> The PIE build flags are only intended for building executables and can not be
>>>> used in relocateable links (-r), static builds and shared library build -
>>>> including the flags here causes build errors.
>>>>
>>>> So instead of parsing the PIE flags directly on the command line to gcc,
>>>> include them in a gcc spec file where it is possible to only apply the flags
>>>> when other incompatible flags are not set.
>>>
>>>  The commit message should also have the reasoning why this is considered better
>>> than doing the same in the wrapper.
>>>
>>
>> It probably isn't better then doing it in the wrapper.  At this point,
>> its just fixing something that's broken with the current approach.  I
>> was assuming a solid baseline option with this feature that's working
>> first and then start looking at replacing the approach with the
>> suggestion awhile back with doing it via the wrapper.
>
>  I'm a bit confused about where you'd like to go in the long run: spec file or
> wrapper? In your reply to the cover letter you mention "I do like the concept of
> changing to the wrapper and evaluating if we can use GCC spec files"...

Sorry, I was pulling a few thoughts together and didn't explain it
clearly.  I was thinking back to previous threads were it was
discussed to take all the hardening options and use the wrapper to
manage them vs directly setting the flag variables.  Ideally, I agree
that a wrapper approach for that is more ideal.  Since that thread,
there has been discussion on Stefan's patches about using spec files
as an option to simplify what the wrapper currently does.  However,
I'd agree that the spec syntax is complex.  For this patchset, the
spec file is accompishing a specific task of the mutually exclusive
options just related to hardening.

So long term, I could see the wrapper handing the hardening flag
management directly and the seperate possibility of spec files
possibly being used for other unrelated items that might simplify
wrapper logic.

>
>  I guess the spec files have the advantage that it is a lot easier to specify
> mutually exclusive options. We have that now in the wrapper for arch/cpu/tune,
> but it's a lot of code to specify something simple. That said, spec files are so
> arcane that you can't expect many people contributing to it. The C code, though
> more verbose, is a lot more understandable.

Agree.

>
>  Back to the patch: basically, the same could be done in the wrapper, but here
> the spec files are just taken from Fedora so that's a lot simpler. Right?

Correct.

>
>
>>>> -TARGET_HARDENED += -fPIE $(TARGET_CFLAGS_RELRO_FULL)
>>>> -TARGET_LDFLAGS += -pie $(TARGET_CFLAGS_RELRO_FULL)
>>>> +TARGET_HARDENED += $(TARGET_CFLAGS_RELRO_FULL) -specs=$(TOPDIR)/toolchain/gcc-specs-pie-cc1
>>>> +TARGET_LDFLAGS += $(TARGET_CFLAGS_RELRO_FULL) -specs=$(TOPDIR)/toolchain/gcc-specs-pie-ld
>>>
>>>  As mentioned in my other mail, it is very likely that there are packages that
>>> link with TARGET_CFLAGS instead of TARGET_LDFLAGS. Would that be a problem?
>
>  I don't think you answered this question?

Sorry lumped this in with the one below.  We found that flag usage was
inconsistent across packages and we were required to over-specify
flags.  So some packages that link correctly may get linker oriented
flags through both cflags and ldflags.  We verified that syntax wise
that this will work and that the post build elf file testing reflects
they take affect.

>
>>> Actually, can't we just have a single spec file that contains both?
>>
>> Like you mentioned previously, flag usage isn't consistent.  When we
>> started doing validation of the elf files we found that inconsistency
>> and we were individually trying to fix the packages.  This didn't get
>> to far as lots of the packages are in stable, no real maintaining
>> occuring.  I'm glad Stefan brought up the spec file idea as that
>> solved all the rework on the linker ordering and combination of
>> values.
>
>  Yes, spec file (or wrapper) is definitely better than patching individual packages.
>
>
>> Related to using a single spec file.  I'm not to familiar with using
>> them but since we have a mix of good/bad flag options already
>> occurring, guessing it wouldn't matter to consolidate to one file.  I
>> can test going to a single file. Stefan, do you know?

I've verified you have to have individual files for each.  Guessing
there isn't shared syntax between cc/ld spec files based on the errors
I got when the compiler was invoked with the additional self_spec
entry in it's spec file.  I couldn't fine enough documentation to
understand what that entry is doing.....

Matt

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Sørensen, Stefan]

On Wed, 2018-07-11 at 18:17 -0500, Matthew Weber wrote:
> Related to using a single spec file.  I'm not to familiar with using
> them but since we have a mix of good/bad flag options already
> occurring, guessing it wouldn't matter to consolidate to one file.  I
> can test going to a single file. Stefan, do you know?

It works fine with a single spec file - I used two different files
since I was keeping CFLAGS and LDFLAGS separate.

I will post an updated series with a single spec file and a few other
changes.

Stefan

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Matthew Weber]

Stefan,

On Thu, Jul 19, 2018 at 4:49 AM, S?rensen, Stefan
<Stefan.Sorensen@spectralink.com> wrote:
>
> On Wed, 2018-07-11 at 18:17 -0500, Matthew Weber wrote:
> > Related to using a single spec file.  I'm not to familiar with using
> > them but since we have a mix of good/bad flag options already
> > occurring, guessing it wouldn't matter to consolidate to one file.  I
> > can test going to a single file. Stefan, do you know?
>
> It works fine with a single spec file - I used two different files
> since I was keeping CFLAGS and LDFLAGS separate.
>

Something in the syntax then would have to change, right?  I combined
the files and was getting CC errors.

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Sørensen, Stefan]

On Thu, 2018-07-19 at 07:58 -0500, Matthew Weber wrote:
> > It works fine with a single spec file - I used two different files
> > since I was keeping CFLAGS and LDFLAGS separate.
> > 
> 
> Something in the syntax then would have to change, right?  I combined
> the files and was getting CC errors.

That was what I did - I have not run into any problems. Any particular
packages that fails?

Stefan

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Matthew Weber]

On Thu, Jul 19, 2018 at 8:10 AM S?rensen, Stefan
<Stefan.Sorensen@spectralink.com> wrote:
>
> On Thu, 2018-07-19 at 07:58 -0500, Matthew Weber wrote:
> > > It works fine with a single spec file - I used two different files
> > > since I was keeping CFLAGS and LDFLAGS separate.
> > >
> >
> > Something in the syntax then would have to change, right?  I combined
> > the files and was getting CC errors.
>
> That was what I did - I have not run into any problems. Any particular
> packages that fails?

attr pkg during the configuring stage
https://paste.ubuntu.com/p/wdn3Pjsd39/

I can work up a simple defconfig but I didn't get to far before I ran into this.

The combined specs file looked like.
*cc1_options:
+ %{!r:%{!fpie:%{!fPIE:%{!fpic:%{!fPIC:%{!fno-pic:-fPIE}}}}}}
*self_spec:
+ %{!static:%{!shared:%{!r:-pie}}}

Matt

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Matthew Weber]

Stefan,
On Tue, Aug 7, 2018 at 12:02 PM Matthew Weber
<matthew.weber@rockwellcollins.com> wrote:
>
> On Thu, Jul 19, 2018 at 8:10 AM S?rensen, Stefan
> <Stefan.Sorensen@spectralink.com> wrote:
> >
> > On Thu, 2018-07-19 at 07:58 -0500, Matthew Weber wrote:
> > > > It works fine with a single spec file - I used two different files
> > > > since I was keeping CFLAGS and LDFLAGS separate.
> > > >
> > >
> > > Something in the syntax then would have to change, right?  I combined
> > > the files and was getting CC errors.
> >
> > That was what I did - I have not run into any problems. Any particular
> > packages that fails?
>
> attr pkg during the configuring stage
> https://paste.ubuntu.com/p/wdn3Pjsd39/
>
> I can work up a simple defconfig but I didn't get to far before I ran into this.
>
> The combined specs file looked like.
> *cc1_options:
> + %{!r:%{!fpie:%{!fPIE:%{!fpic:%{!fPIC:%{!fno-pic:-fPIE}}}}}}
> *self_spec:
> + %{!static:%{!shared:%{!r:-pie}}}
>

Config to reproduce.
BR2_aarch64=y
BR2_SSP_STRONG=y
BR2_RELRO_FULL=y
BR2_FORTIFY_SOURCE_2=y
BR2_TOOLCHAIN_EXTERNAL=y
BR2_TARGET_GENERIC_GETTY_PORT="ttyAMA0"
BR2_SYSTEM_DHCP="eth0"
BR2_LINUX_KERNEL=y
BR2_LINUX_KERNEL_CUSTOM_VERSION=y
BR2_LINUX_KERNEL_CUSTOM_VERSION_VALUE="4.13.6"
BR2_LINUX_KERNEL_USE_CUSTOM_CONFIG=y
BR2_LINUX_KERNEL_CUSTOM_CONFIG_FILE="board/qemu/aarch64-virt/linux-4.13.config"
BR2_PACKAGE_ATTR=y
BR2_TARGET_ROOTFS_EXT2=y
BR2_TARGET_ROOTFS_EXT2_4=y
# BR2_TARGET_ROOTFS_TAR is not set




-- 
Matthew L Weber / Pr Software Engineer
Airborne Information Systems / RC Linux Secure Platforms
MS 131-100, C Ave NE, Cedar Rapids, IA, 52498, USA
www.rockwellcollins.com

Note: Any Export License Required Information and License Restricted
Third Party Intellectual Property (TPIP) content must be encrypted and
sent to matthew.weber at corp.rockwellcollins.com.

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Jan Kundrát]

Hi Matt, thanks for working on this.

When I apply this patch, I'm getting a build failure from an out-of-tree 
[1] package, see the attached log. The package lives at [1], the Buildroot 
recipe is pretty straightforward [2]. Here's the relevant snippet from the 
failure:

  /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc -Wall 
-Wl,-z -Wl,now -Wl,-z -Wl,relro 
-specs=/home/jkt/work/cesnet/gerrit/github/buildroot/buildroot/toolchain/gcc-specs-pie-ld 
-o .libs/example example.o  ./.libs/libredblack.so
  
/home/jkt/work/prog/_build/br-cfb/host/opt/ext-toolchain/bin/../lib/gcc/arm-linux-gnueabihf/7.3.1/../../../../arm-linux-gnueabihf/bin/ld: 
example.o: relocation R_ARM_MOVW_ABS_NC against `stderr' can not be used 
when making a shared object; recompile with -fPIC

Is that a failure of that particular package, or a problem in the compiler 
specs?

Also, Gentoo Linux has been doing compiler hardening for years and their 
specs appear to be more complex according to the docs [4]. I have no clue 
if that extra complexity is needed in Buildroot.

With kind regards,
Jan

[1] I'll submit them once my patches reach a release upstream.
[2] https://github.com/sysrepo/libredblack
[3] 
https://gerrit.cesnet.cz/plugins/gitiles/github/buildroot/buildroot/+/a0cff08df5918e1721723400da42c8c68b8c0928%5E%21/
[4] https://wiki.gentoo.org/wiki/Hardened/Toolchain
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: log-libredblack-spec-files
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20180808/4e152f50/attachment.ksh>

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Jan Kundrát]

I just got a similar failure from an in-tree package, the spi-tools:

>>> spi-tools 0.8.1 Building
PATH="/home/jkt/work/prog/_build/br-cfb/host/bin:/home/jkt/work/prog/_build/br-cfb/host/sbin:/home/jkt/.local/bin:/home/jkt/bin/:/opt/qtc/bin:/opt/darktable/bin:/home/jkt/.local/bin:/home/jkt/bin/:/opt/qtc/bin:/opt/darktable/bin:/usr/x86_64-pc-linux-gnu/gcc-bin/8.1.0:/usr/lib/llvm/6/bin:/usr/lib/llvm/4/bin:/usr/local/bin:/usr/bin:/bin:/opt/bin:/usr/games/bin" 
 /usr/bin/make   -C 
/home/jkt/work/prog/_build/br-cfb/build/spi-tools-0.8.1/
Making all in src
/usr/bin/make  all-am
/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc  -O0  
-Wl,-z,now -Wl,-z,relro 
-specs=/home/jkt/work/cesnet/gerrit/github/buildroot/buildroot/toolchain/gcc-specs-pie-ld 
-o spi-config spi-config.o  
/home/jkt/work/prog/_build/br-cfb/host/opt/ext-toolchain/bin/../lib/gcc/arm-linux-gnueabihf/7.3.1/../../../../arm-linux-gnueabihf/bin/ld: 
spi-config.o: relocation R_ARM_MOVW_ABS_NC against `stderr' can not be used 
when making a shared object; recompile with -fPIC
spi-config.o: error adding symbols: Bad value
collect2: error: ld returned 1 exit status

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Matthew Weber]

Jan,

On Wed, Aug 8, 2018 at 3:36 AM Jan Kundr?t <jan.kundrat@cesnet.cz> wrote:
>
> I just got a similar failure from an in-tree package, the spi-tools:

Cool, would you mind sharing your buildroot master hash + which of the
patchwork patches you've applied to it?  I'll recreate and take a
look.

Matt

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Matthew Weber]

Jan,
On Wed, Aug 8, 2018 at 3:36 AM Jan Kundr?t <jan.kundrat@cesnet.cz> wrote:
>
> I just got a similar failure from an in-tree package, the spi-tools:
>
> >>> spi-tools 0.8.1 Building
> PATH="/home/jkt/work/prog/_build/br-cfb/host/bin:/home/jkt/work/prog/_build/br-cfb/host/sbin:/home/jkt/.local/bin:/home/jkt/bin/:/opt/qtc/bin:/opt/darktable/bin:/home/jkt/.local/bin:/home/jkt/bin/:/opt/qtc/bin:/opt/darktable/bin:/usr/x86_64-pc-linux-gnu/gcc-bin/8.1.0:/usr/lib/llvm/6/bin:/usr/lib/llvm/4/bin:/usr/local/bin:/usr/bin:/bin:/opt/bin:/usr/games/bin"
>  /usr/bin/make   -C
> /home/jkt/work/prog/_build/br-cfb/build/spi-tools-0.8.1/
> Making all in src
> /usr/bin/make  all-am
> /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc  -O0
> -Wl,-z,now -Wl,-z,relro
> -specs=/home/jkt/work/cesnet/gerrit/github/buildroot/buildroot/toolchain/gcc-specs-pie-ld
> -o spi-config spi-config.o
> /home/jkt/work/prog/_build/br-cfb/host/opt/ext-toolchain/bin/../lib/gcc/arm-linux-gnueabihf/7.3.1/../../../../arm-linux-gnueabihf/bin/ld:
> spi-config.o: relocation R_ARM_MOVW_ABS_NC against `stderr' can not be used
> when making a shared object; recompile with -fPIC
> spi-config.o: error adding symbols: Bad value
> collect2: error: ld returned 1 exit status

This one has to do with the configure.ac of that package not observing
cflags/cxxflags being passed to configure.  So the *.o that are trying
to be linked weren't build as position independent.  Probably a
similar case for your libredblack package.

I've posted an upstream issue at this point and a basic fix.  As
buildroot could probably use a bump, rather then carrying the patch,
I'll ask upstream if they could do a tag after this fix is merged.
https://github.com/cpb-/spi-tools/issues/12

Matt

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Matthew Weber]

Jan,
On Wed, Aug 8, 2018 at 3:36 AM Jan Kundr?t <jan.kundrat@cesnet.cz> wrote:
>
> I just got a similar failure from an in-tree package, the spi-tools:
>
> >>> spi-tools 0.8.1 Building
> PATH="/home/jkt/work/prog/_build/br-cfb/host/bin:/home/jkt/work/prog/_build/br-cfb/host/sbin:/home/jkt/.local/bin:/home/jkt/bin/:/opt/qtc/bin:/opt/darktable/bin:/home/jkt/.local/bin:/home/jkt/bin/:/opt/qtc/bin:/opt/darktable/bin:/usr/x86_64-pc-linux-gnu/gcc-bin/8.1.0:/usr/lib/llvm/6/bin:/usr/lib/llvm/4/bin:/usr/local/bin:/usr/bin:/bin:/opt/bin:/usr/games/bin"
>  /usr/bin/make   -C
> /home/jkt/work/prog/_build/br-cfb/build/spi-tools-0.8.1/
> Making all in src
> /usr/bin/make  all-am
> /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc  -O0
> -Wl,-z,now -Wl,-z,relro
> -specs=/home/jkt/work/cesnet/gerrit/github/buildroot/buildroot/toolchain/gcc-specs-pie-ld
> -o spi-config spi-config.o
> /home/jkt/work/prog/_build/br-cfb/host/opt/ext-toolchain/bin/../lib/gcc/arm-linux-gnueabihf/7.3.1/../../../../arm-linux-gnueabihf/bin/ld:
> spi-config.o: relocation R_ARM_MOVW_ABS_NC against `stderr' can not be used
> when making a shared object; recompile with -fPIC
> spi-config.o: error adding symbols: Bad value
> collect2: error: ld returned 1 exit status

I was able to get the spi-tools cflags not passing through from
buildroot fix upstreamed and new release from spi-tools.  Just sent a
bump to buildroot.

http://patchwork.ozlabs.org/patch/963100/

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Thomas Petazzoni]

Matt, Stefan,

On Wed, 11 Jul 2018 09:31:10 -0500, Matt Weber wrote:
> From: Stefan S?rensen <stefan.sorensen@spectralink.com>
> 
> The PIE build flags are only intended for building executables and can not be
> used in relocateable links (-r), static builds and shared library build -
> including the flags here causes build errors.
> 
> So instead of parsing the PIE flags directly on the command line to gcc,
> include them in a gcc spec file where it is possible to only apply the flags
> when other incompatible flags are not set.
> 
> This method and the spec files are from the Fedora build system.
> 
> Originally submitted as
> http://patchwork.ozlabs.org/patch/907093/
> 
> Signed-off-by: Stefan S?rensen <stefan.sorensen@spectralink.com>
> Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com>

I've read the whole discussion on this patch, but still I don't
understand which direction we want to take moving forward: move away
from handling options in the wrapper, and use a spec file for
everything ? Or keep the entire logic in the wrapper ?

I'm not happy at all with the approach of having some flags handled in
the wrapper, some flags handled through spec files. I believe choosing
the spec file direction makes this patch series more difficult to
merge, because we have to go through this whole discussion of spec file
vs. wrapper.

I have nothing against using spec files, but right now, our logic is
based on a wrapper program. Therefore, I would be more comfortable with
an approach that relies on the wrapper program, so that it is in line
with what we are doing today. Then, separately, we can discuss how our
wrapper can be replaced, completely or partially, by a spec file.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Matthew Weber]

Thomas,
On Fri, Aug 10, 2018 at 3:50 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Matt, Stefan,
>
> On Wed, 11 Jul 2018 09:31:10 -0500, Matt Weber wrote:
> > From: Stefan S?rensen <stefan.sorensen@spectralink.com>
> >
> > The PIE build flags are only intended for building executables and can not be
> > used in relocateable links (-r), static builds and shared library build -
> > including the flags here causes build errors.
> >
> > So instead of parsing the PIE flags directly on the command line to gcc,
> > include them in a gcc spec file where it is possible to only apply the flags
> > when other incompatible flags are not set.
> >
> > This method and the spec files are from the Fedora build system.
> >
> > Originally submitted as
> > http://patchwork.ozlabs.org/patch/907093/
> >
> > Signed-off-by: Stefan S?rensen <stefan.sorensen@spectralink.com>
> > Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com>
>
> I've read the whole discussion on this patch, but still I don't
> understand which direction we want to take moving forward: move away
> from handling options in the wrapper, and use a spec file for
> everything ? Or keep the entire logic in the wrapper ?

So far, the spec files are just fixing a conditional case where a
option isn't compatible.  ie PIE and shared can't be used together.
This prevents a lot of patchwork on all the packages.  I could move
this string search and condition process to the wrapper but it seems
cleaner to do that in the spec file.  I do want to note that we did
not go to the extent that gentoo did where they use spec files to
enable and do the conditional compatibility fixups at link time(PIE vs
shared) for all the hardening options.

>
> I'm not happy at all with the approach of having some flags handled in
> the wrapper, some flags handled through spec files. I believe choosing
> the spec file direction makes this patch series more difficult to
> merge, because we have to go through this whole discussion of spec file
> vs. wrapper.

Agree, the specfile even just doing the conditional fixup is a 3rd leg
on this whole thing.  We can add it to the wrapper code but we will
loose visibility to what the wrapper is doing vs with the spec we can
dump and analyze the output.

>
> I have nothing against using spec files, but right now, our logic is
> based on a wrapper program. Therefore, I would be more comfortable with
> an approach that relies on the wrapper program, so that it is in line
> with what we are doing today. Then, separately, we can discuss how our
> wrapper can be replaced, completely or partially, by a spec file.

Ok. If the above feedback in this email doesn't change the view
related to spec files, I can look at switching to a pure wrapper for
fixups and static setting of variables with flags.


Matt

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Thomas Petazzoni]

Hello,

On Fri, 10 Aug 2018 19:42:32 -0500, Matthew Weber wrote:

> > I've read the whole discussion on this patch, but still I don't
> > understand which direction we want to take moving forward: move away
> > from handling options in the wrapper, and use a spec file for
> > everything ? Or keep the entire logic in the wrapper ?  
> 
> So far, the spec files are just fixing a conditional case where a
> option isn't compatible.  ie PIE and shared can't be used together.
> This prevents a lot of patchwork on all the packages.  I could move
> this string search and condition process to the wrapper but it seems
> cleaner to do that in the spec file.  I do want to note that we did
> not go to the extent that gentoo did where they use spec files to
> enable and do the conditional compatibility fixups at link time(PIE vs
> shared) for all the hardening options.

I understand the idea of using the spec file. However, I'm worried that
it further complicates the overall toolchain setup. In addition, the
-spec option passed in TARGET_CFLAGS/TARGET_LDFLAGS will only work with
packages that do obey to CFLAGS/LDFLAGS, and some broken packages
don't. Being in the wrapper allows to ensure the flags are used. And
additionally, what's done in the wrapper is also used when the
Buildroot toolchain is used "manually" (outside of the Buildroot
infrastructure) to build stuff.

> > I'm not happy at all with the approach of having some flags handled in
> > the wrapper, some flags handled through spec files. I believe choosing
> > the spec file direction makes this patch series more difficult to
> > merge, because we have to go through this whole discussion of spec file
> > vs. wrapper.  
> 
> Agree, the specfile even just doing the conditional fixup is a 3rd leg
> on this whole thing.  We can add it to the wrapper code but we will
> loose visibility to what the wrapper is doing vs with the spec we can
> dump and analyze the output.

That's not true: the wrapper looks at BR2_DEBUG_WRAPPER={1,2} to dump
what it is doing:

        /* Debug the wrapper to see actual arguments passed to
         * the compiler:
         * unset, empty, or 0: do not trace
         * set to 1          : trace all arguments on a single line
         * set to 2          : trace one argument per line
         */
        if ((env_debug = getenv("BR2_DEBUG_WRAPPER"))) {

> > I have nothing against using spec files, but right now, our logic is
> > based on a wrapper program. Therefore, I would be more comfortable with
> > an approach that relies on the wrapper program, so that it is in line
> > with what we are doing today. Then, separately, we can discuss how our
> > wrapper can be replaced, completely or partially, by a spec file.  
> 
> Ok. If the above feedback in this email doesn't change the view
> related to spec files, I can look at switching to a pure wrapper for
> fixups and static setting of variables with flags.

I think I'd be happier with a first step where the wrapper is being
used. Then in a second step, we can decide to rethink what the wrapper
is doing, and move more of its logic to a spec file. I think a wrapper
will still be needed anyway, at least to pass --sysroot and --spec.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Matthew Weber]

Thomas,
On Sat, Aug 11, 2018 at 5:30 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello,
>
> On Fri, 10 Aug 2018 19:42:32 -0500, Matthew Weber wrote:
>
> > > I've read the whole discussion on this patch, but still I don't
> > > understand which direction we want to take moving forward: move away
> > > from handling options in the wrapper, and use a spec file for
> > > everything ? Or keep the entire logic in the wrapper ?
> >
> > So far, the spec files are just fixing a conditional case where a
> > option isn't compatible.  ie PIE and shared can't be used together.
> > This prevents a lot of patchwork on all the packages.  I could move
> > this string search and condition process to the wrapper but it seems
> > cleaner to do that in the spec file.  I do want to note that we did
> > not go to the extent that gentoo did where they use spec files to
> > enable and do the conditional compatibility fixups at link time(PIE vs
> > shared) for all the hardening options.
>
> I understand the idea of using the spec file. However, I'm worried that
> it further complicates the overall toolchain setup. In addition, the
> -spec option passed in TARGET_CFLAGS/TARGET_LDFLAGS will only work with
> packages that do obey to CFLAGS/LDFLAGS, and some broken packages
> don't. Being in the wrapper allows to ensure the flags are used. And
> additionally, what's done in the wrapper is also used when the
> Buildroot toolchain is used "manually" (outside of the Buildroot
> infrastructure) to build stuff.
>
> > > I'm not happy at all with the approach of having some flags handled in
> > > the wrapper, some flags handled through spec files. I believe choosing
> > > the spec file direction makes this patch series more difficult to
> > > merge, because we have to go through this whole discussion of spec file
> > > vs. wrapper.
> >
> > Agree, the specfile even just doing the conditional fixup is a 3rd leg
> > on this whole thing.  We can add it to the wrapper code but we will
> > loose visibility to what the wrapper is doing vs with the spec we can
> > dump and analyze the output.
>
> That's not true: the wrapper looks at BR2_DEBUG_WRAPPER={1,2} to dump
> what it is doing:
>
>         /* Debug the wrapper to see actual arguments passed to
>          * the compiler:
>          * unset, empty, or 0: do not trace
>          * set to 1          : trace all arguments on a single line
>          * set to 2          : trace one argument per line
>          */
>         if ((env_debug = getenv("BR2_DEBUG_WRAPPER"))) {
>
> > > I have nothing against using spec files, but right now, our logic is
> > > based on a wrapper program. Therefore, I would be more comfortable with
> > > an approach that relies on the wrapper program, so that it is in line
> > > with what we are doing today. Then, separately, we can discuss how our
> > > wrapper can be replaced, completely or partially, by a spec file.
> >
> > Ok. If the above feedback in this email doesn't change the view
> > related to spec files, I can look at switching to a pure wrapper for
> > fixups and static setting of variables with flags.
>
> I think I'd be happier with a first step where the wrapper is being
> used. Then in a second step, we can decide to rethink what the wrapper
> is doing, and move more of its logic to a spec file. I think a wrapper
> will still be needed anyway, at least to pass --sysroot and --spec.
>

I started to look at this tonight and if I understand the current
wrapper, it's solely working with cc/gcc.  If someone explicity uses
ld, the wrapper doesn't take affect? (I'll do a build tomorrow and
look at behavior, didn't have a linux machine handy tonight to check.

If that's the case, any pointers to detecting the type of invocation
of the wrapper if I update the symlinking so ld also goes through?

Matt

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Thomas Petazzoni]

Hello,

On Sat, 11 Aug 2018 22:55:47 -0500, Matthew Weber wrote:

> I started to look at this tonight and if I understand the current
> wrapper, it's solely working with cc/gcc.  If someone explicity uses
> ld, the wrapper doesn't take affect? (I'll do a build tomorrow and
> look at behavior, didn't have a linux machine handy tonight to check.

We only wrap gcc/cc indeed. Packages should not call "ld" directly, as
it breaks a bunch of other situations. For example, on mips64, using
"ld" directly doesn't work as it needs to be passed the "machine
emulation", which gcc does.

So I don't think we need to wrap "ld", as ld shouldn't be used
directly. The only packages that should use "ld" directly are things
like the Linux kernel or bootloaders.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Matthew Weber]

Thomas,
On Sun, Aug 12, 2018 at 2:41 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello,
>
> On Sat, 11 Aug 2018 22:55:47 -0500, Matthew Weber wrote:
>
> > I started to look at this tonight and if I understand the current
> > wrapper, it's solely working with cc/gcc.  If someone explicity uses
> > ld, the wrapper doesn't take affect? (I'll do a build tomorrow and
> > look at behavior, didn't have a linux machine handy tonight to check.
>
> We only wrap gcc/cc indeed. Packages should not call "ld" directly, as
> it breaks a bunch of other situations. For example, on mips64, using
> "ld" directly doesn't work as it needs to be passed the "machine
> emulation", which gcc does.
>
> So I don't think we need to wrap "ld", as ld shouldn't be used
> directly. The only packages that should use "ld" directly are things
> like the Linux kernel or bootloaders.
>

The current hardening approach is trying to cover the cases where
packages are still using ld directly and have other incompatible flags
set (static/shared/r).  I don't have the exact list but I believe
busybox is even one of those and others like valgrind, boost, etc who
use the "shared" flag and adding "pie" causes a compile failure.  So
we do still need to cover the ld case or go patch packages.

What I could do is move the cc1 spec file conditional add of PIE into
the wrapper.  Then leave the LDFLAGS as we have them and the
associated spec file that does a conditional add of "pie".  This would
prevent us from wrapping the ld tool and keep the existing wrapper
approach consistent.

Matt

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Thomas Petazzoni]

Hello,

On Sun, 12 Aug 2018 07:49:19 -0500, Matthew Weber wrote:

> > So I don't think we need to wrap "ld", as ld shouldn't be used
> > directly. The only packages that should use "ld" directly are things
> > like the Linux kernel or bootloaders.
> 
> The current hardening approach is trying to cover the cases where
> packages are still using ld directly and have other incompatible flags
> set (static/shared/r).  I don't have the exact list but I believe
> busybox is even one of those and others like valgrind, boost, etc who
> use the "shared" flag and adding "pie" causes a compile failure.  So
> we do still need to cover the ld case or go patch packages.

I find it weird that those packages are using "ld" directly, because if
that's the case, we would have build failures on some mips64
configurations.

> What I could do is move the cc1 spec file conditional add of PIE into
> the wrapper.  Then leave the LDFLAGS as we have them and the
> associated spec file that does a conditional add of "pie".  This would
> prevent us from wrapping the ld tool and keep the existing wrapper
> approach consistent.

If we really need to do some custom logic around ld, then I believe I'd
prefer to have a wrapper for it as well, to keep things consistent. But
of course, Arnout's opinion on the matter would be welcome.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 3/6] package/Makefile.in: Use gcc spec files for PIE build flags

[Arnout Vandecappelle]

On 12-08-18 17:07, Thomas Petazzoni wrote:
> Hello,
> 
> On Sun, 12 Aug 2018 07:49:19 -0500, Matthew Weber wrote:
> 
>>> So I don't think we need to wrap "ld", as ld shouldn't be used
>>> directly. The only packages that should use "ld" directly are things
>>> like the Linux kernel or bootloaders.
>>
>> The current hardening approach is trying to cover the cases where
>> packages are still using ld directly and have other incompatible flags
>> set (static/shared/r).  I don't have the exact list but I believe
>> busybox is even one of those and others like valgrind, boost, etc who
>> use the "shared" flag and adding "pie" causes a compile failure.  So
>> we do still need to cover the ld case or go patch packages.
> 
> I find it weird that those packages are using "ld" directly, because if
> that's the case, we would have build failures on some mips64
> configurations.

 In addition, if you call ld directly, the spec file is not used at all... The
spec file is only used by the gcc wrapper.


>> What I could do is move the cc1 spec file conditional add of PIE into
>> the wrapper.  Then leave the LDFLAGS as we have them and the
>> associated spec file that does a conditional add of "pie".  This would
>> prevent us from wrapping the ld tool and keep the existing wrapper
>> approach consistent.

 This indicates the crux of the problem with the wrapper: it is not easy to
detect in the wrapper that we're linking. This is "solved" by specifying the
linker spec file in LDFLAGS, but since many packages (probably) don't use
LDFLAGS, it is not really solved at all... Well, except in the sense that if a
package doesn't look at LDFLAGS, it's not going to get the -pie hardening flag
either so the problem that this patch is fixing will not occur.

 We *can* keep passing -pie in LDFLAGS, and then remove it again in the wrapper
when -shared or -r or -static is present, but it feels weird... Hm, but
apparently we can just always pass -pie in the wrapper, even when we're not
linking. That would solve the issue relatively elegantly within the wrapper.


> If we really need to do some custom logic around ld, then I believe I'd
> prefer to have a wrapper for it as well, to keep things consistent. But
> of course, Arnout's opinion on the matter would be welcome.

 I don't think an ld wrapper is needed. We've discussed this many times already,
and I see less and less reason for it.

 Regards,
 Arnout

> 
> Thomas
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 4/6] support/testing: runtest proxy support

[Matt Weber]

Allow builder.py to inherit the system proxy settings from
the env if they are present.

Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
---
 support/testing/infra/builder.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/support/testing/infra/builder.py b/support/testing/infra/builder.py
index faf1eb1494..30230fdb17 100644
--- a/support/testing/infra/builder.py
+++ b/support/testing/infra/builder.py
@@ -35,6 +35,12 @@ class Builder(object):
 
     def build(self):
         env = {"PATH": os.environ["PATH"]}
+        if "http_proxy" in os.environ:
+            self.logfile.write("Using system proxy: " +
+                               os.environ["http_proxy"] + "\n")
+            self.logfile.flush()
+            env['http_proxy'] = os.environ["http_proxy"]
+            env['https_proxy'] = os.environ["http_proxy"]
         cmd = ["make", "-C", self.builddir]
         ret = subprocess.call(cmd, stdout=self.logfile, stderr=self.logfile,
                               env=env)
-- 
2.17.0

[Buildroot] [PATCH 4/6] support/testing: runtest proxy support

[Arnout Vandecappelle]

On 11-07-18 16:31, Matt Weber wrote:
> Allow builder.py to inherit the system proxy settings from
> the env if they are present.
> 
> Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
> ---
>  support/testing/infra/builder.py | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/support/testing/infra/builder.py b/support/testing/infra/builder.py
> index faf1eb1494..30230fdb17 100644
> --- a/support/testing/infra/builder.py
> +++ b/support/testing/infra/builder.py
> @@ -35,6 +35,12 @@ class Builder(object):
>  
>      def build(self):
>          env = {"PATH": os.environ["PATH"]}
> +        if "http_proxy" in os.environ:
> +            self.logfile.write("Using system proxy: " +
> +                               os.environ["http_proxy"] + "\n")
> +            self.logfile.flush()
> +            env['http_proxy'] = os.environ["http_proxy"]
> +            env['https_proxy'] = os.environ["http_proxy"]

 I'm not sure if we wouldn't want to propagate the possibly different
https_proxy variable instead, but in practice I see little use of that, so

Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

 Regards,
 Arnout

>          cmd = ["make", "-C", self.builddir]
>          ret = subprocess.call(cmd, stdout=self.logfile, stderr=self.logfile,
>                                env=env)
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 4/6] support/testing: runtest proxy support

[Thomas Petazzoni]

Hello,

On Wed, 11 Jul 2018 09:31:11 -0500, Matt Weber wrote:
> Allow builder.py to inherit the system proxy settings from
> the env if they are present.
> 
> Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>

I have applied to next, but I have one question below.

> ---
>  support/testing/infra/builder.py | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/support/testing/infra/builder.py b/support/testing/infra/builder.py
> index faf1eb1494..30230fdb17 100644
> --- a/support/testing/infra/builder.py
> +++ b/support/testing/infra/builder.py
> @@ -35,6 +35,12 @@ class Builder(object):
>  
>      def build(self):
>          env = {"PATH": os.environ["PATH"]}
> +        if "http_proxy" in os.environ:
> +            self.logfile.write("Using system proxy: " +
> +                               os.environ["http_proxy"] + "\n")
> +            self.logfile.flush()

Is this flush() really needed ?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 4/6] support/testing: runtest proxy support

[Matthew Weber]

Thomas,

On Fri, Aug 10, 2018 at 3:51 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello,
>
> On Wed, 11 Jul 2018 09:31:11 -0500, Matt Weber wrote:
> > Allow builder.py to inherit the system proxy settings from
> > the env if they are present.
> >
> > Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
>
> I have applied to next, but I have one question below.
>
> > ---
> >  support/testing/infra/builder.py | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/support/testing/infra/builder.py b/support/testing/infra/builder.py
> > index faf1eb1494..30230fdb17 100644
> > --- a/support/testing/infra/builder.py
> > +++ b/support/testing/infra/builder.py
> > @@ -35,6 +35,12 @@ class Builder(object):
> >
> >      def build(self):
> >          env = {"PATH": os.environ["PATH"]}
> > +        if "http_proxy" in os.environ:
> > +            self.logfile.write("Using system proxy: " +
> > +                               os.environ["http_proxy"] + "\n")
> > +            self.logfile.flush()
>
> Is this flush() really needed ?

I had added it because my subprocess.call() in python was failing and
at that time I believed the proxy logfile write wasn't flushing.  It
could probably be removed now as it was an artifact of debugging.

Matt

[Buildroot] [PATCH 4/6] support/testing: runtest proxy support

[Matthew Weber]

Thomas,
On Fri, Aug 10, 2018 at 7:30 PM Matthew Weber
<matthew.weber@rockwellcollins.com> wrote:
>
> Thomas,
>
> On Fri, Aug 10, 2018 at 3:51 PM Thomas Petazzoni
> <thomas.petazzoni@bootlin.com> wrote:
> >
> > Hello,
> >
> > On Wed, 11 Jul 2018 09:31:11 -0500, Matt Weber wrote:
> > > Allow builder.py to inherit the system proxy settings from
> > > the env if they are present.
> > >
> > > Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
> >
> > I have applied to next, but I have one question below.
> >
> > > ---
> > >  support/testing/infra/builder.py | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/support/testing/infra/builder.py b/support/testing/infra/builder.py
> > > index faf1eb1494..30230fdb17 100644
> > > --- a/support/testing/infra/builder.py
> > > +++ b/support/testing/infra/builder.py
> > > @@ -35,6 +35,12 @@ class Builder(object):
> > >
> > >      def build(self):
> > >          env = {"PATH": os.environ["PATH"]}
> > > +        if "http_proxy" in os.environ:
> > > +            self.logfile.write("Using system proxy: " +
> > > +                               os.environ["http_proxy"] + "\n")
> > > +            self.logfile.flush()
> >
> > Is this flush() really needed ?
>
> I had added it because my subprocess.call() in python was failing and
> at that time I believed the proxy logfile write wasn't flushing.  It
> could probably be removed now as it was an artifact of debugging.
>
> Matt

https://patchwork.ozlabs.org/patch/956512/

[Buildroot] [PATCH 5/6] package/checksec: new package

[Matt Weber]

From: Paresh Chaudhary <paresh.chaudhary@rockwellcollins.com>

This patch added host-checksec package support. This tool
provides a script to offline check the properties of a
security hardened elf file.

REF: https://github.com/slimm609/checksec.sh

Signed-off-by: Paresh Chaudhary <paresh.chaudhary@rockwellcollins.com>
Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com>
---
 package/Config.in.host                        |  1 +
 ...cksec-Fixed-issue-with-relative-path.patch | 43 +++++++++++++++++++
 package/checksec/Config.in.host               | 16 +++++++
 package/checksec/checksec.hash                |  3 ++
 package/checksec/checksec.mk                  | 16 +++++++
 5 files changed, 79 insertions(+)
 create mode 100644 package/checksec/0001-checksec-Fixed-issue-with-relative-path.patch
 create mode 100644 package/checksec/Config.in.host
 create mode 100644 package/checksec/checksec.hash
 create mode 100644 package/checksec/checksec.mk

diff --git a/package/Config.in.host b/package/Config.in.host
index 7838ffc219..0c21b11bd0 100644
--- a/package/Config.in.host
+++ b/package/Config.in.host
@@ -5,6 +5,7 @@ menu "Host utilities"
 	source "package/cargo/Config.in.host"
 	source "package/cbootimage/Config.in.host"
 	source "package/checkpolicy/Config.in.host"
+	source "package/checksec/Config.in.host"
 	source "package/cmake/Config.in.host"
 	source "package/cramfs/Config.in.host"
 	source "package/cryptsetup/Config.in.host"
diff --git a/package/checksec/0001-checksec-Fixed-issue-with-relative-path.patch b/package/checksec/0001-checksec-Fixed-issue-with-relative-path.patch
new file mode 100644
index 0000000000..43a882d991
--- /dev/null
+++ b/package/checksec/0001-checksec-Fixed-issue-with-relative-path.patch
@@ -0,0 +1,43 @@
+From b48a2dfae26fa3b4af8e65fb5953b3caf62c137b Mon Sep 17 00:00:00 2001
+From: Paresh Chaudhary <paresh.chaudhary@rockwellcollins.com>
+Date: Mon, 21 May 2018 14:34:23 -0500
+Subject: [PATCH] checksec: Fixed issue with relative path
+
+Before this patch script was not able find exists directory when user pass
+relative directory path with '--dir' or '-d' option and also we faced this error
+when we execute script with relative path.
+
+This patch fixed  "No such file or directory" error in both cases.
+
+https://github.com/slimm609/checksec.sh/issues/54
+
+Signed-off-by: Paresh Chaudhary <paresh.chaudhary@rockwellcollins.com>
+---
+ checksec | 4 ++--
+ 1 file changed, 2 insertions(+), 2 deletions(-)
+
+diff --git a/checksec b/checksec
+index 24b521f..baf8d63 100755
+--- a/checksec
++++ b/checksec
+@@ -1193,7 +1193,7 @@ do
+     echo_message "RELRO           STACK CANARY      NX            PIE             RPATH      RUNPATH      FORTIFY Checked         Total   Filename\n" '' "<dir name='$tempdir'>\n" "{ \"dir\": { \"name\":\"$tempdir\" },"
+     fdircount=0
+     fdirtotal=0
+-    for N in $(find $tempdir -type f); do
++    for N in $(find . -type f); do
+       if [[ "$N" != "[A-Za-z1-0]*" ]]; then
+         out=$(file "$N")
+         if [[  $out =~ ELF ]] ; then
+@@ -1201,7 +1201,7 @@ do
+         fi
+       fi
+     done
+-    for N in $(find $tempdir -type f); do
++    for N in $(find . -type f); do
+       if [[ "$N" != "[A-Za-z1-0]*" ]]; then
+     # read permissions?
+     if [[ ! -r "$N" ]]; then
+-- 
+1.9.1
+
diff --git a/package/checksec/Config.in.host b/package/checksec/Config.in.host
new file mode 100644
index 0000000000..7f86f46b50
--- /dev/null
+++ b/package/checksec/Config.in.host
@@ -0,0 +1,16 @@
+config BR2_PACKAGE_HOST_CHECKSEC
+	bool "host checksec"
+	help
+	  This tool provides a shell script to check the
+	  properties of the executables
+	  (like PIE,RELRO,PaX,Canaries,ASLR,Fortify Source).
+
+	  https://github.com/slimm609/checksec.sh.git
+
+	  NOTE: This tool has a hard-coded path to the standard
+	  libraries for some of the fortify test cases and
+	  requires you to either test the local filesystem or be
+	  in a chroot'd environment.  The tool can still be used
+	  against a folder of files but requires discretion of
+	  which the tests may not report consistently vs
+	  chroot/on-target.
diff --git a/package/checksec/checksec.hash b/package/checksec/checksec.hash
new file mode 100644
index 0000000000..e3d1ffd5d1
--- /dev/null
+++ b/package/checksec/checksec.hash
@@ -0,0 +1,3 @@
+# Locally calculated
+sha256 510b0b0528f15d0bf13fa1ae7140d2b9fc9261323c98ff76c011bef475a69c14 checksec-cdefe53eb72e6e8f23308417d2fc6b68cba9dbac.tar.gz
+sha256 c5e2a8e188040fc34eb9362084778a2e25f8d1f888e47a2be09efa7cecd9c70d LICENSE.txt
diff --git a/package/checksec/checksec.mk b/package/checksec/checksec.mk
new file mode 100644
index 0000000000..31ceb43e21
--- /dev/null
+++ b/package/checksec/checksec.mk
@@ -0,0 +1,16 @@
+################################################################################
+#
+# checksec
+#
+################################################################################
+
+CHECKSEC_VERSION = cdefe53eb72e6e8f23308417d2fc6b68cba9dbac
+CHECKSEC_SITE = $(call github,slimm609,checksec.sh,$(CHECKSEC_VERSION))
+CHECKSEC_LICENSE = BSD-3-Clause
+CHECKSEC_LICENSE_FILES = LICENSE.txt
+
+define HOST_CHECKSEC_INSTALL_CMDS
+	$(INSTALL) -D -m 0755 $(@D)/checksec $(HOST_DIR)/bin/
+endef
+
+$(eval $(host-generic-package))
-- 
2.17.0

[Buildroot] [PATCH 5/6] package/checksec: new package

[Thomas Petazzoni]

Matt, Paresh,

On Wed, 11 Jul 2018 09:31:12 -0500, Matt Weber wrote:
> From: Paresh Chaudhary <paresh.chaudhary@rockwellcollins.com>
> 
> This patch added host-checksec package support. This tool

added -> adds

> diff --git a/package/checksec/0001-checksec-Fixed-issue-with-relative-path.patch b/package/checksec/0001-checksec-Fixed-issue-with-relative-path.patch
> new file mode 100644
> index 0000000000..43a882d991
> --- /dev/null
> +++ b/package/checksec/0001-checksec-Fixed-issue-with-relative-path.patch
> @@ -0,0 +1,43 @@
> +From b48a2dfae26fa3b4af8e65fb5953b3caf62c137b Mon Sep 17 00:00:00 2001
> +From: Paresh Chaudhary <paresh.chaudhary@rockwellcollins.com>
> +Date: Mon, 21 May 2018 14:34:23 -0500
> +Subject: [PATCH] checksec: Fixed issue with relative path
> +
> +Before this patch script was not able find exists directory when user pass
> +relative directory path with '--dir' or '-d' option and also we faced this error
> +when we execute script with relative path.

The english wording seems weird here, even though I'm not a native
speaker. Perhaps:

"""
Before this patch, the checksec script was not able to find existing
directories when the user passes a relative path with --dir/-d,
aborting with a "No such file or directory". The same error was
reported when the script is executed through a relative path.
"""

I'm sure Matt, as a native speaker, can come up with an even better
wording.

> diff --git a/package/checksec/Config.in.host b/package/checksec/Config.in.host
> new file mode 100644
> index 0000000000..7f86f46b50
> --- /dev/null
> +++ b/package/checksec/Config.in.host
> @@ -0,0 +1,16 @@
> +config BR2_PACKAGE_HOST_CHECKSEC
> +	bool "host checksec"
> +	help
> +	  This tool provides a shell script to check the
> +	  properties of the executables
> +	  (like PIE,RELRO,PaX,Canaries,ASLR,Fortify Source).
> +
> +	  https://github.com/slimm609/checksec.sh.git
> +
> +	  NOTE: This tool has a hard-coded path to the standard
> +	  libraries for some of the fortify test cases and
> +	  requires you to either test the local filesystem or be
> +	  in a chroot'd environment.  The tool can still be used
> +	  against a folder of files but requires discretion of
> +	  which the tests may not report consistently vs
> +	  chroot/on-target.

When I look at this and the comment from the maintainer at [0], I am
not sure about the usefulness of such a tool in the context of
Buildroot. Chrooting into the target filesystem is generally not
possible, because the target architecture is different than the build
system architecture. To me, this limitation makes the tool essentially
useless in the context of Buildroot. Could you comment on this a bit
more ?

Also, the formulation "requires discretion of which the test may not
report consistently vs chroot/on-target" doesn't make any sense to me.

[0] https://github.com/slimm609/checksec.sh/issues/62#issuecomment-389880584

> diff --git a/package/checksec/checksec.hash b/package/checksec/checksec.hash
> new file mode 100644
> index 0000000000..e3d1ffd5d1
> --- /dev/null
> +++ b/package/checksec/checksec.hash
> @@ -0,0 +1,3 @@
> +# Locally calculated
> +sha256 510b0b0528f15d0bf13fa1ae7140d2b9fc9261323c98ff76c011bef475a69c14 checksec-cdefe53eb72e6e8f23308417d2fc6b68cba9dbac.tar.gz
> +sha256 c5e2a8e188040fc34eb9362084778a2e25f8d1f888e47a2be09efa7cecd9c70d LICENSE.txt
> diff --git a/package/checksec/checksec.mk b/package/checksec/checksec.mk
> new file mode 100644
> index 0000000000..31ceb43e21
> --- /dev/null
> +++ b/package/checksec/checksec.mk
> @@ -0,0 +1,16 @@
> +################################################################################
> +#
> +# checksec
> +#
> +################################################################################
> +
> +CHECKSEC_VERSION = cdefe53eb72e6e8f23308417d2fc6b68cba9dbac
> +CHECKSEC_SITE = $(call github,slimm609,checksec.sh,$(CHECKSEC_VERSION))
> +CHECKSEC_LICENSE = BSD-3-Clause
> +CHECKSEC_LICENSE_FILES = LICENSE.txt
> +
> +define HOST_CHECKSEC_INSTALL_CMDS
> +	$(INSTALL) -D -m 0755 $(@D)/checksec $(HOST_DIR)/bin/

There should be a full destination path here.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 5/6] package/checksec: new package

[Matthew Weber]

Thomas,
On Fri, Aug 10, 2018 at 3:59 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Matt, Paresh,
>
> On Wed, 11 Jul 2018 09:31:12 -0500, Matt Weber wrote:
> > From: Paresh Chaudhary <paresh.chaudhary@rockwellcollins.com>
> >
> > This patch added host-checksec package support. This tool
>
> added -> adds

Noted.

>
> > diff --git a/package/checksec/0001-checksec-Fixed-issue-with-relative-path.patch b/package/checksec/0001-checksec-Fixed-issue-with-relative-path.patch
> > new file mode 100644
> > index 0000000000..43a882d991
> > --- /dev/null
> > +++ b/package/checksec/0001-checksec-Fixed-issue-with-relative-path.patch
> > @@ -0,0 +1,43 @@
> > +From b48a2dfae26fa3b4af8e65fb5953b3caf62c137b Mon Sep 17 00:00:00 2001
> > +From: Paresh Chaudhary <paresh.chaudhary@rockwellcollins.com>
> > +Date: Mon, 21 May 2018 14:34:23 -0500
> > +Subject: [PATCH] checksec: Fixed issue with relative path
> > +
> > +Before this patch script was not able find exists directory when user pass
> > +relative directory path with '--dir' or '-d' option and also we faced this error
> > +when we execute script with relative path.
>
> The english wording seems weird here, even though I'm not a native
> speaker. Perhaps:
>
> """
> Before this patch, the checksec script was not able to find existing
> directories when the user passes a relative path with --dir/-d,
> aborting with a "No such file or directory". The same error was
> reported when the script is executed through a relative path.
> """
>
> I'm sure Matt, as a native speaker, can come up with an even better
> wording.

Thanks, yes will rework.

>
> > diff --git a/package/checksec/Config.in.host b/package/checksec/Config.in.host
> > new file mode 100644
> > index 0000000000..7f86f46b50
> > --- /dev/null
> > +++ b/package/checksec/Config.in.host
> > @@ -0,0 +1,16 @@
> > +config BR2_PACKAGE_HOST_CHECKSEC
> > +     bool "host checksec"
> > +     help
> > +       This tool provides a shell script to check the
> > +       properties of the executables
> > +       (like PIE,RELRO,PaX,Canaries,ASLR,Fortify Source).
> > +
> > +       https://github.com/slimm609/checksec.sh.git
> > +
> > +       NOTE: This tool has a hard-coded path to the standard
> > +       libraries for some of the fortify test cases and
> > +       requires you to either test the local filesystem or be
> > +       in a chroot'd environment.  The tool can still be used
> > +       against a folder of files but requires discretion of
> > +       which the tests may not report consistently vs
> > +       chroot/on-target.
>
> When I look at this and the comment from the maintainer at [0], I am
> not sure about the usefulness of such a tool in the context of
> Buildroot. Chrooting into the target filesystem is generally not
> possible, because the target architecture is different than the build
> system architecture. To me, this limitation makes the tool essentially
> useless in the context of Buildroot. Could you comment on this a bit
> more ?

The tool tests a lot of items related to hardening and we were
originally trying to get the full set working.  In reality we only
needed the core items that show us ASLR related items.  The tool is
made up of scripts and uses readelf for the ASLR piece.  Thus it works
fine for a host (offline)target filesystem check of executable ALSR
requirements.  However, I can add a note stating what doesn't work
correctly.  There are test cases it has that use live proc information
and the system libraries, etc.

>
> Also, the formulation "requires discretion of which the test may not
> report consistently vs chroot/on-target" doesn't make any sense to me.

I can make a list do this is definitive.

>
> [0] https://github.com/slimm609/checksec.sh/issues/62#issuecomment-389880584
>
> > diff --git a/package/checksec/checksec.hash b/package/checksec/checksec.hash
> > new file mode 100644
> > index 0000000000..e3d1ffd5d1
> > --- /dev/null
> > +++ b/package/checksec/checksec.hash
> > @@ -0,0 +1,3 @@
> > +# Locally calculated
> > +sha256 510b0b0528f15d0bf13fa1ae7140d2b9fc9261323c98ff76c011bef475a69c14 checksec-cdefe53eb72e6e8f23308417d2fc6b68cba9dbac.tar.gz
> > +sha256 c5e2a8e188040fc34eb9362084778a2e25f8d1f888e47a2be09efa7cecd9c70d LICENSE.txt
> > diff --git a/package/checksec/checksec.mk b/package/checksec/checksec.mk
> > new file mode 100644
> > index 0000000000..31ceb43e21
> > --- /dev/null
> > +++ b/package/checksec/checksec.mk
> > @@ -0,0 +1,16 @@
> > +################################################################################
> > +#
> > +# checksec
> > +#
> > +################################################################################
> > +
> > +CHECKSEC_VERSION = cdefe53eb72e6e8f23308417d2fc6b68cba9dbac
> > +CHECKSEC_SITE = $(call github,slimm609,checksec.sh,$(CHECKSEC_VERSION))
> > +CHECKSEC_LICENSE = BSD-3-Clause
> > +CHECKSEC_LICENSE_FILES = LICENSE.txt
> > +
> > +define HOST_CHECKSEC_INSTALL_CMDS
> > +     $(INSTALL) -D -m 0755 $(@D)/checksec $(HOST_DIR)/bin/
>
> There should be a full destination path here.

Noted.

Thanks for the review.

Matt

[Buildroot] [PATCH 5/6] package/checksec: new package

[Thomas Petazzoni]

Hello Matt,

On Fri, 10 Aug 2018 19:57:06 -0500, Matthew Weber wrote:

> > When I look at this and the comment from the maintainer at [0], I am
> > not sure about the usefulness of such a tool in the context of
> > Buildroot. Chrooting into the target filesystem is generally not
> > possible, because the target architecture is different than the build
> > system architecture. To me, this limitation makes the tool essentially
> > useless in the context of Buildroot. Could you comment on this a bit
> > more ?  
> 
> The tool tests a lot of items related to hardening and we were
> originally trying to get the full set working.  In reality we only
> needed the core items that show us ASLR related items.  The tool is
> made up of scripts and uses readelf for the ASLR piece.  Thus it works
> fine for a host (offline)target filesystem check of executable ALSR
> requirements.  However, I can add a note stating what doesn't work
> correctly.  There are test cases it has that use live proc information
> and the system libraries, etc.

Yes, something more specific than the vague explanation in the proposed
Config.in help text would be good.

> > Also, the formulation "requires discretion of which the test may not
> > report consistently vs chroot/on-target" doesn't make any sense to me.  
> 
> I can make a list do this is definitive.

OK, good.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 6/6] support/testing/tests/core: SSP & hardening flags

[Matt Weber]

Catch the commonly used options of SSP, Relro, and fortify.

Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
---
 support/testing/tests/core/test_hardening.py | 104 +++++++++++++++++++
 1 file changed, 104 insertions(+)
 create mode 100644 support/testing/tests/core/test_hardening.py

diff --git a/support/testing/tests/core/test_hardening.py b/support/testing/tests/core/test_hardening.py
new file mode 100644
index 0000000000..2a479d89aa
--- /dev/null
+++ b/support/testing/tests/core/test_hardening.py
@@ -0,0 +1,104 @@
+import os
+import subprocess
+import json
+
+import infra.basetest
+
+HARD_DEFCONFIG = \
+    """
+    BR2_powerpc64=y
+    BR2_powerpc_e5500=y
+    BR2_TOOLCHAIN_EXTERNAL=y
+    BR2_TOOLCHAIN_EXTERNAL_DOWNLOAD=y
+    BR2_TOOLCHAIN_EXTERNAL_URL="https://toolchains.bootlin.com/downloads/releases/toolchains/powerpc64-e5500/tarballs/powerpc64-e5500--glibc--stable-2018.02-2.tar.bz2"
+    BR2_TOOLCHAIN_EXTERNAL_GCC_6=y
+    BR2_TOOLCHAIN_EXTERNAL_HEADERS_4_1=y
+    BR2_TOOLCHAIN_EXTERNAL_CUSTOM_GLIBC=y
+    BR2_TOOLCHAIN_EXTERNAL_CXX=y
+    BR2_PACKAGE_LIGHTTPD=y
+    BR2_PACKAGE_HOST_CHECKSEC=y
+    # BR2_TARGET_ROOTFS_TAR is not set
+    """
+
+def checksec_run(builddir, target_file):
+    cmd = ["host/bin/checksec", "--output", "json", "--file", target_file]
+    ret = subprocess.check_output(cmd,
+                                  stderr=open(os.devnull, "w"),
+                                  cwd=builddir,
+                                  env={"LANG": "C"})
+    return ret
+
+class TestRelro(infra.basetest.BRTest):
+    config = HARD_DEFCONFIG + \
+        """
+        BR2_RELRO_FULL=y
+        """
+
+    def test_run(self):
+        out = json.loads(checksec_run(self.builddir, "target/usr/sbin/lighttpd"))
+        self.assertEqual(out["file"]["relro"], "full")
+        self.assertEqual(out["file"]["pie"], "yes")
+        out = json.loads(checksec_run(self.builddir, "target/bin/busybox"))
+        self.assertEqual(out["file"]["relro"], "full")
+
+class TestRelroPartial(infra.basetest.BRTest):
+    config = HARD_DEFCONFIG + \
+        """
+        BR2_RELRO_PARTIAL=y
+        """
+
+    def test_run(self):
+        out = json.loads(checksec_run(self.builddir, "target/usr/sbin/lighttpd"))
+        self.assertEqual(out["file"]["relro"], "partial")
+        self.assertEqual(out["file"]["pie"], "no")
+        out = json.loads(checksec_run(self.builddir, "target/bin/busybox"))
+        self.assertEqual(out["file"]["relro"], "partial")
+
+class TestSspNone(infra.basetest.BRTest):
+    config = HARD_DEFCONFIG + \
+        """
+        BR2_SSP_NONE=y
+        """
+
+    def test_run(self):
+        out = json.loads(checksec_run(self.builddir, "target/usr/sbin/lighttpd"))
+        self.assertEqual(out["file"]["canary"], "no")
+        out = json.loads(checksec_run(self.builddir, "target/bin/busybox"))
+        self.assertEqual(out["file"]["canary"], "no")
+
+
+class TestSspStrong(infra.basetest.BRTest):
+    config = HARD_DEFCONFIG + \
+        """
+        BR2_SSP_STRONG=y
+        """
+
+    def test_run(self):
+        out = json.loads(checksec_run(self.builddir, "target/usr/sbin/lighttpd"))
+        self.assertEqual(out["file"]["canary"], "yes")
+        out = json.loads(checksec_run(self.builddir, "target/bin/busybox"))
+        self.assertEqual(out["file"]["canary"], "yes")
+
+class TestFortifyNone(infra.basetest.BRTest):
+    config = HARD_DEFCONFIG + \
+        """
+        BR2_FORTIFY_SOURCE_NONE=y
+        """
+
+    def test_run(self):
+        out = json.loads(checksec_run(self.builddir, "target/usr/sbin/lighttpd"))
+        self.assertEqual(out["file"]["fortified"], "0")
+        out = json.loads(checksec_run(self.builddir, "target/bin/busybox"))
+        self.assertEqual(out["file"]["fortified"], "0")
+
+class TestFortifyConserv(infra.basetest.BRTest):
+    config = HARD_DEFCONFIG + \
+        """
+        BR2_FORTIFY_SOURCE_1=y
+        """
+
+    def test_run(self):
+        out = json.loads(checksec_run(self.builddir, "target/usr/sbin/lighttpd"))
+        self.assertNotEqual(out["file"]["fortified"], "0")
+        out = json.loads(checksec_run(self.builddir, "target/bin/busybox"))
+        self.assertNotEqual(out["file"]["fortified"], "0")
-- 
2.17.0

[Buildroot] [PATCH 6/6] support/testing/tests/core: SSP & hardening flags

[Ricardo Martincoski]

Hello,

Looks good in general. A few nits below.

On Wed, Jul 11, 2018 at 11:31 AM, Matt Weber wrote:

> Catch the commonly used options of SSP, Relro, and fortify.
> 
> Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
> ---
>  support/testing/tests/core/test_hardening.py | 104 +++++++++++++++++++
>  1 file changed, 104 insertions(+)
>  create mode 100644 support/testing/tests/core/test_hardening.py

You forgot to run 'make .gitlab-ci.yml'. It could be done while applying.

> 
> diff --git a/support/testing/tests/core/test_hardening.py b/support/testing/tests/core/test_hardening.py
> new file mode 100644
> index 0000000000..2a479d89aa
> --- /dev/null
> +++ b/support/testing/tests/core/test_hardening.py

Could you fix the 6 warnings from flake8?
https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/81143173

[snip]
> +class TestRelro(infra.basetest.BRTest):
> +    config = HARD_DEFCONFIG + \
> +        """
> +        BR2_RELRO_FULL=y
> +        """
> +
> +    def test_run(self):
> +        out = json.loads(checksec_run(self.builddir, "target/usr/sbin/lighttpd"))
> +        self.assertEqual(out["file"]["relro"], "full")
> +        self.assertEqual(out["file"]["pie"], "yes")
> +        out = json.loads(checksec_run(self.builddir, "target/bin/busybox"))
> +        self.assertEqual(out["file"]["relro"], "full")

Any reason to not test 'pie' for busybox?
        self.assertEqual(out["file"]["pie"], "yes")

> +
> +class TestRelroPartial(infra.basetest.BRTest):
> +    config = HARD_DEFCONFIG + \
> +        """
> +        BR2_RELRO_PARTIAL=y
> +        """
> +
> +    def test_run(self):
> +        out = json.loads(checksec_run(self.builddir, "target/usr/sbin/lighttpd"))
> +        self.assertEqual(out["file"]["relro"], "partial")
> +        self.assertEqual(out["file"]["pie"], "no")
> +        out = json.loads(checksec_run(self.builddir, "target/bin/busybox"))
> +        self.assertEqual(out["file"]["relro"], "partial")

The same here:
        self.assertEqual(out["file"]["pie"], "no")


Regards,
Ricardo

[Buildroot] [PATCH 6/6] support/testing/tests/core: SSP & hardening flags

[Matthew Weber]

Ricardo,

On Sun, Jul 15, 2018 at 8:32 PM, Ricardo Martincoski
<ricardo.martincoski@gmail.com> wrote:
> Hello,
>
> Looks good in general. A few nits below.
>
> On Wed, Jul 11, 2018 at 11:31 AM, Matt Weber wrote:
>
>> Catch the commonly used options of SSP, Relro, and fortify.
>>
>> Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
>> ---
>>  support/testing/tests/core/test_hardening.py | 104 +++++++++++++++++++
>>  1 file changed, 104 insertions(+)
>>  create mode 100644 support/testing/tests/core/test_hardening.py
>
> You forgot to run 'make .gitlab-ci.yml'. It could be done while applying.

I didn't realize that existed.  Maybe I should add a new section 23
titled "Testing" and a subsection on contributing a test case?

>
>>
>> diff --git a/support/testing/tests/core/test_hardening.py b/support/testing/tests/core/test_hardening.py
>> new file mode 100644
>> index 0000000000..2a479d89aa
>> --- /dev/null
>> +++ b/support/testing/tests/core/test_hardening.py
>
> Could you fix the 6 warnings from flake8?
> https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/81143173

Can do.  I'll go ahead and respin a new rev with this

>
> [snip]
>> +class TestRelro(infra.basetest.BRTest):
>> +    config = HARD_DEFCONFIG + \
>> +        """
>> +        BR2_RELRO_FULL=y
>> +        """
>> +
>> +    def test_run(self):
>> +        out = json.loads(checksec_run(self.builddir, "target/usr/sbin/lighttpd"))
>> +        self.assertEqual(out["file"]["relro"], "full")
>> +        self.assertEqual(out["file"]["pie"], "yes")
>> +        out = json.loads(checksec_run(self.builddir, "target/bin/busybox"))
>> +        self.assertEqual(out["file"]["relro"], "full")
>
> Any reason to not test 'pie' for busybox?
>         self.assertEqual(out["file"]["pie"], "yes")
>

Oops, left that out.  I've updated both cases.

Thanks for the review
Matt

[Buildroot] [PATCH 6/6] support/testing/tests/core: SSP & hardening flags

[Matthew Weber]

Ricardo,

On Mon, Jul 16, 2018 at 9:53 PM, Matthew Weber
<matthew.weber@rockwellcollins.com> wrote:
> Ricardo,
>
> On Sun, Jul 15, 2018 at 8:32 PM, Ricardo Martincoski
> <ricardo.martincoski@gmail.com> wrote:
>> Hello,
>>
>> Looks good in general. A few nits below.
>>

Updated patch
http://patchwork.ozlabs.org/patch/944700/

[Buildroot] [PATCH 0/6] Hardening Flag Bugfix/Enhancement

[Matthew Weber]

All,

On Wed, Jul 11, 2018 at 9:31 AM, Matt Weber
<matthew.weber@rockwellcollins.com> wrote:
> This series pulls together a few pending patches required for hardening
> flag bug-fixes.  Additionally a tool is added with Buildroot test cases
> to validate that the hardening options are working correctly.

I forgot to mention the goal of this series is to fix and complete
testing of the existing approach.  I do like the concept of changing
to the wrapper and evaluating if we can use GCC spec files, however, I
was hoping we could establish a working baseline first.  Then propose
changes to possibly use a more elegant approach.

>
> Stefan S?ena
> http://patchwork.ozlabs.org/patch/904057/  (Bugfix)
> http://patchwork.ozlabs.org/patch/904034/  (Bugfix)
>
>
> Matt Weber (Both have been marked as superseded)
> http://patchwork.ozlabs.org/patch/907093/  (Bugfix)
> http://patchwork.ozlabs.org/patch/932853/  (New checksec tool)
>
> A unrelated patch was also included which adds proxy env support for the runtests script.
>
> Matt Weber (2):
>   support/testing: runtest proxy support
>   support/testing/tests/core: SSP & hardening flags
>
> Paresh Chaudhary (1):
>   package/checksec: new package
>
> Stefan S?rensen (3):
>   package/Makefile.in: Do not use CPPFLAGS for hardening options
>   package/Makefile.in: Add missing options to LDFLAGS for full RELRO
>     build
>   package/Makefile.in: Use gcc spec files for PIE build flags
>
>  package/Config.in.host                        |   1 +
>  package/Makefile.in                           |  18 +--
>  ...cksec-Fixed-issue-with-relative-path.patch |  43 ++++++++
>  package/checksec/Config.in.host               |  16 +++
>  package/checksec/checksec.hash                |   3 +
>  package/checksec/checksec.mk                  |  16 +++
>  support/testing/infra/builder.py              |   6 +
>  support/testing/tests/core/test_hardening.py  | 104 ++++++++++++++++++
>  toolchain/gcc-specs-pie-cc1                   |   2 +
>  toolchain/gcc-specs-pie-ld                    |   2 +
>  10 files changed, 202 insertions(+), 9 deletions(-)
>  create mode 100644 package/checksec/0001-checksec-Fixed-issue-with-relative-path.patch
>  create mode 100644 package/checksec/Config.in.host
>  create mode 100644 package/checksec/checksec.hash
>  create mode 100644 package/checksec/checksec.mk
>  create mode 100644 support/testing/tests/core/test_hardening.py
>  create mode 100644 toolchain/gcc-specs-pie-cc1
>  create mode 100644 toolchain/gcc-specs-pie-ld
>
> --
> 2.17.0
>



-- 
Matthew L Weber / Pr Software Engineer
Airborne Information Systems / RC Linux Secure Platforms
MS 131-100, C Ave NE, Cedar Rapids, IA, 52498, USA
www.rockwellcollins.com

Note: Any Export License Required Information and License Restricted
Third Party Intellectual Property (TPIP) content must be encrypted and
sent to matthew.weber at corp.rockwellcollins.com.