Hardening Flag Bugfix/Enhancement

From: Stefan S?rensen <[email protected]>

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 <[email protected]>

Signed-off-by Matthew Weber <[email protected]>

---
 package/Makefile.in | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

package/Makefile.in +5 -5

index f2962767cc..5e0ff8c841 100644
@@ -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)

package/Makefile.in +0 -0

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

From: Stefan S?rensen <[email protected]>

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 <[email protected]>

Signed-off-by Matthew Weber <[email protected]>

---
 package/Makefile.in | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

From: Stefan S?rensen <[email protected]>

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 <[email protected]>

Signed-off-by Matt Weber <[email protected]>

---
 package/Makefile.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

package/Makefile.in +0 -0

index 5e0ff8c841..14b3bbd243 100644

package/Makefile.in +0 -0

 endif
 
 ifeq ($(BR2_FORTIFY_SOURCE_1),y)

From: Stefan S?rensen <[email protected]>

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 <[email protected]>

Signed-off-by Matt Weber <[email protected]>

---
 package/Makefile.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

From: Stefan S?rensen <[email protected]>

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 <[email protected]>

Signed-off-by Matt Weber <[email protected]>

---
 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

package/Makefile.in +0 -0

index 14b3bbd243..00ddf48c10 100644

package/Makefile.in +0 -0

 endif
 
 ifeq ($(BR2_FORTIFY_SOURCE_1),y)

toolchain/gcc-specs-pie-cc1 new file +2 -0

new file mode 100644
index 0000000000..fc54bcb510
@@ -0,0 +1,2 @@
+*cc1_options:
++ %{!r:%{!fpie:%{!fPIE:%{!fpic:%{!fPIC:%{!fno-pic:-fPIE}}}}}}

toolchain/gcc-specs-pie-ld new file +2 -0

new file mode 100644
index 0000000000..bd6b9071ff
@@ -0,0 +1,2 @@
+*self_spec:
++ %{!static:%{!shared:%{!r:-pie}}}

Quoted 6 lines · Matt Weber

From: Stefan S?rensen <[email protected]>

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.

Quoted 3 lines

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.

package/Makefile.in ifeq ($(BR2_RELRO_PARTIAL),y) · line 162

Quoted 4 lines

package/Makefile.in +2 -2

-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?

Arnout,

Quoted 9 lines · Arnout Vandecappelle

Quoted 6 lines · Matt Weber

From: Stefan S?rensen <[email protected]>

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 7 lines

package/Makefile.in ifeq ($(BR2_RELRO_PARTIAL),y) · line 162

Quoted 4 lines

package/Makefile.in +2 -2

-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?

Quoted 1 line

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

Quoted 20 lines · Matthew Weber

Arnout,

Quoted 9 lines · Arnout Vandecappelle

Quoted 6 lines · Matt Weber

From: Stefan S?rensen <[email protected]>

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 5 lines

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?

Quoted 9 lines

Quoted 9 lines

package/Makefile.in ifeq ($(BR2_RELRO_PARTIAL),y) · line 162

Quoted 6 lines

package/Makefile.in +2 -2

-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?

Quoted 10 lines

Quoted 2 lines

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.

Quoted 4 lines

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?

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?

Quoted 4 lines · Matthew Weber

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.

Quoted 3 lines

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.

Quoted 6 lines · Matthew Weber

Quoted 3 lines

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?

Quoted 11 lines · Stefan

Quoted 6 lines · Matthew Weber

Quoted 3 lines

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}}}

Quoted 1 line

Quoted 1 line

spi-tools 0.8.1 Building

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

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

Quoted 1 line

Quoted 1 line

Quoted 1 line

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

(quoted diff) +0 -3

-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 just got a similar failure from an in-tree package, the spi-tools:

Quoted 1 line

Quoted 1 line

Quoted 1 line

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

(quoted diff) +0 -3

-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

From: Stefan S?rensen <[email protected]>

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 <[email protected]>

Signed-off-by Matt Weber <[email protected]>

Matt, Stefan,

Quoted 15 lines · Matt Weber

From: Stefan S?rensen <[email protected]>

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/

Quoted 2 lines

Signed-off-by Stefan S?rensen <[email protected]>

Signed-off-by Matt Weber <[email protected]>

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.

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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.

Hello,

Quoted 5 lines · Matthew Weber

Quoted 4 lines

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 ?

Quoted 8 lines

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.

Quoted 10 lines

Quoted 5 lines

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"))) {

Quoted 6 lines

Quoted 5 lines

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.

Quoted 3 lines

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.

Hello,

Quoted 4 lines · Matthew Weber

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.

Quoted 3 lines

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.

Hello,

Quoted 10 lines · Matthew Weber

Quoted 3 lines

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.

Quoted 5 lines

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

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

Signed-off-by Matthew Weber <[email protected]>

---
 support/testing/infra/builder.py | 6 ++++++
 1 file changed, 6 insertions(+)

support/testing/infra/builder.py +0 -0

index faf1eb1494..30230fdb17 100644

support/testing/infra/builder.py +0 -0

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

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

Signed-off-by Matthew Weber <[email protected]>

---
 support/testing/infra/builder.py | 6 ++++++
 1 file changed, 6 insertions(+)

support/testing/infra/builder.py +0 -0

index faf1eb1494..30230fdb17 100644

Hello,

Quoted 4 lines · Matt Weber

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

Signed-off-by Matthew Weber <[email protected]>

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

Quoted 10 lines

---
 support/testing/infra/builder.py | 6 ++++++
 1 file changed, 6 insertions(+)

support/testing/infra/builder.py +0 -0

index faf1eb1494..30230fdb17 100644
@@ -35,6 +35,12 @@ class Builder(object):
 

support/testing/infra/builder.py class Builder(object): · line 41

Quoted 6 lines

support/testing/infra/builder.py +4 -0

     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 ?

Thomas,

Quoted 21 lines · Thomas Petazzoni

Hello,

Quoted 4 lines · Matt Weber

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

Signed-off-by Matthew Weber <[email protected]>

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

Quoted 10 lines

---
 support/testing/infra/builder.py | 6 ++++++
 1 file changed, 6 insertions(+)

support/testing/infra/builder.py +0 -0

index faf1eb1494..30230fdb17 100644
@@ -35,6 +35,12 @@ class Builder(object):
 

Quoted 8 lines

support/testing/infra/builder.py class Builder(object): · line 41

Quoted 6 lines

support/testing/infra/builder.py +4 -0

     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

From: Paresh Chaudhary <[email protected]>

This patch added host-checksec package support. This tool

package/checksec/0001-checksec-Fixed-issue-with-relative-path.patch new file +0 -0

new file mode 100644
index 0000000000..43a882d991

package/checksec/Config.in.host new file +6 -0

new file mode 100644
index 0000000000..7f86f46b50
@@ -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).

package/checksec/checksec.hash new file +3 -0

new file mode 100644
index 0000000000..e3d1ffd5d1
@@ -0,0 +1,3 @@
+# Locally calculated
+sha256 510b0b0528f15d0bf13fa1ae7140d2b9fc9261323c98ff76c011bef475a69c14 checksec-cdefe53eb72e6e8f23308417d2fc6b68cba9dbac.tar.gz
+sha256 c5e2a8e188040fc34eb9362084778a2e25f8d1f888e47a2be09efa7cecd9c70d LICENSE.txt

package/checksec/checksec.mk new file +3 -0

new file mode 100644
index 0000000000..31ceb43e21
@@ -0,0 +1,16 @@
+################################################################################
+#
+# checksec

Matt, Paresh,

Quoted 3 lines · Matt Weber

From: Paresh Chaudhary <[email protected]>

This patch added host-checksec package support. This tool

added -> adds

Quoted 7 lines

package/checksec/0001-checksec-Fixed-issue-with-relative-path.patch new file +0 -0

new file mode 100644
index 0000000000..43a882d991

package/checksec/0001-checksec-Fixed-issue-with-relative-path.patch line 8

Quoted 8 lines

package/checksec/0001-checksec-Fixed-issue-with-relative-path.patch new file +8 -0

@@ -0,0 +1,43 @@
+From b48a2dfae26fa3b4af8e65fb5953b3caf62c137b Mon Sep 17 00:00:00 2001
+From: Paresh Chaudhary <[email protected]>
+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.

Quoted 13 lines

package/checksec/Config.in.host new file +6 -0

new file mode 100644
index 0000000000..7f86f46b50
@@ -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).

package/checksec/Config.in.host line 16

Quoted 10 lines

package/checksec/Config.in.host new file +10 -0

+
+       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

Quoted 22 lines

package/checksec/checksec.hash new file +3 -0

new file mode 100644
index 0000000000..e3d1ffd5d1
@@ -0,0 +1,3 @@
+# Locally calculated
+sha256 510b0b0528f15d0bf13fa1ae7140d2b9fc9261323c98ff76c011bef475a69c14 checksec-cdefe53eb72e6e8f23308417d2fc6b68cba9dbac.tar.gz
+sha256 c5e2a8e188040fc34eb9362084778a2e25f8d1f888e47a2be09efa7cecd9c70d LICENSE.txt

package/checksec/checksec.mk new file +7 -0

new file mode 100644
index 0000000000..31ceb43e21
@@ -0,0 +1,16 @@
+################################################################################
+#
+# checksec
+#
+################################################################################
+
+CHECKSEC_VERSION = cdefe53eb72e6e8f23308417d2fc6b68cba9dbac

package/checksec/checksec.mk line 13

Quoted 6 lines

package/checksec/checksec.mk new file +6 -0

+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.

Quoted 7 lines

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.

Quoted 2 lines

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.

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

Signed-off-by Matthew Weber <[email protected]>

---
 support/testing/tests/core/test_hardening.py | 104 +++++++++++++++++++
 1 file changed, 104 insertions(+)
 create mode 100644 support/testing/tests/core/test_hardening.py

Hello,

Looks good in general. A few nits below.

Quoted 2 lines · Matt Weber

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

Quoted 5 lines

Signed-off-by Matthew Weber <[email protected]>

---
 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.

support/testing/tests/core/test_hardening.py

Quoted 7 lines

support/testing/tests/core/test_hardening.py new file +0 -0

new file mode 100644
index 0000000000..2a479d89aa

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

[snip]

support/testing/tests/core/test_hardening.py line 42

Quoted 12 lines

support/testing/tests/core/test_hardening.py new file +12 -0

+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")

Ricardo,

Quoted 4 lines · Ricardo Martincoski

Hello,

Looks good in general. A few nits below.

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