| Message ID | 20180717030420.12009-1-matthew.weber@rockwellcollins.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
Hello, On Mon, 16 Jul 2018 22:04:20 -0500, Matt Weber wrote: > Catch the commonly used options of SSP, Relro, and fortify. > > Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com> I'm a bit confused by this patch coming alone, outside of any series. Is it just because you had only changes to this PATCH 6/6 that you didn't resent the full series ? In general, my preference is that a full series gets resent, because it is obvious what your intention is. By just resending one patch, I wonder if you really sent that one patch, or if all patches were sent, but the previous ones were lost/not received for some reason. Thanks! Thomas
Thomas, On Tue, Jul 17, 2018 at 3:06 AM, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > Hello, > > On Mon, 16 Jul 2018 22:04:20 -0500, Matt Weber wrote: >> Catch the commonly used options of SSP, Relro, and fortify. >> >> Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com> > > I'm a bit confused by this patch coming alone, outside of any series. > Is it just because you had only changes to this PATCH 6/6 that you > didn't resent the full series ? Understood, yeah I should have asked on IRC today before sending. I already had reviewed by on the other patches so I hesitated resending them as there wasn't change. Generally, can I add reviewed by's on patches that don't change on a sending of a new version as long as the patch didn't change? > > In general, my preference is that a full series gets resent, because it > is obvious what your intention is. By just resending one patch, I > wonder if you really sent that one patch, or if all patches were sent, > but the previous ones were lost/not received for some reason. > Agree, it does make it confusing. My intent was just to update this one as the rest were in a good state without resending and had review. Do you want me to resent the complete set? Matt
Hello, On Tue, 17 Jul 2018 06:25:28 -0500, Matthew Weber wrote: > > I'm a bit confused by this patch coming alone, outside of any series. > > Is it just because you had only changes to this PATCH 6/6 that you > > didn't resent the full series ? > > Understood, yeah I should have asked on IRC today before sending. I > already had reviewed by on the other patches so I hesitated resending > them as there wasn't change. Generally, can I add reviewed by's on > patches that don't change on a sending of a new version as long as the > patch didn't change? It's not only that you "can", it's that you "should" so that just Reviewed-by are not lost. Of course, if you make changes to the patches, you cannot keep the Reviewed-by because it may not hold anymore (except if the changes are really tiny/trivial). > > In general, my preference is that a full series gets resent, because it > > is obvious what your intention is. By just resending one patch, I > > wonder if you really sent that one patch, or if all patches were sent, > > but the previous ones were lost/not received for some reason. > > Agree, it does make it confusing. My intent was just to update this > one as the rest were in a good state without resending and had review. > Do you want me to resent the complete set? No need to do that for this one. But in general, my preference goes to receiving the full series for each iteration. Yes it creates more noise/traffic on the mailing list, but it makes perfectly clear what is intended, which to me is more important. Thanks a lot! Thomas
Hello, On Tue, Jul 17, 2018 at 12:04 AM, Matt Weber wrote: > Catch the commonly used options of SSP, Relro, and fortify. > > Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com> Reviewed-by: Ricardo Martincoski <ricardo.martincoski@gmail.com> [with patches 1 to 5 from v1 + this patch, tested on the gitlab CI: https://gitlab.com/RicardoMartincoski/buildroot/pipelines/25963338] Tested-by: Ricardo Martincoski <ricardo.martincoski@gmail.com> Regards, Ricardo
Hello, On Mon, 16 Jul 2018 22:04:20 -0500, Matt Weber wrote: > diff --git a/support/testing/tests/core/test_hardening.py b/support/testing/tests/core/test_hardening.py > new file mode 100644 > index 0000000000..d3eb0941d3 > --- /dev/null > +++ b/support/testing/tests/core/test_hardening.py > @@ -0,0 +1,112 @@ > +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] I'm even more confused now. I thought checksec didn't work in a cross-compiled situation. Could you clarify ? > + ret = subprocess.check_output(cmd, > + stderr=open(os.devnull, "w"), > + cwd=builddir, > + env={"LANG": "C"}) > + return ret Perhaps this function should also do the json.loads and return only out["file"] to avoid duplicating this everywhere in the below classes ? Also, using inheritance would be better here: class TestHardeningBase(infra.basetest.BRTest): config = """ ... the base defconfig ... """ checksec_files = ["usr/sbin/lighttpd", "bin/busybox"] def checksec_run(self, target_file): filepath = os.path.join(self.builddir, "target", target_file) ... class TestHardeningRelRo(TestHardeningBase): config = TestHardeningBase.config + """ ... """ def test_run(): for f in checksec_files: out = self.checksec_run(f) self.assertEqual(out["relro"], "full") self.assertEqual(out["pie"], "yes") and ditto for the other tests. Best regards, Thomas
Thomas, On Fri, Aug 10, 2018 at 4:18 PM Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > > Hello, > > On Mon, 16 Jul 2018 22:04:20 -0500, Matt Weber wrote: > > > diff --git a/support/testing/tests/core/test_hardening.py b/support/testing/tests/core/test_hardening.py > > new file mode 100644 > > index 0000000000..d3eb0941d3 > > --- /dev/null > > +++ b/support/testing/tests/core/test_hardening.py > > @@ -0,0 +1,112 @@ > > +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] > > I'm even more confused now. I thought checksec didn't work in a > cross-compiled situation. Could you clarify ? It has some limitations related to FORTIFY checking when being used in a cross compile configuration (needs to run in the actual filesystem of the executable to look at the standard libraries). I can add a note to state that limitation. Otherwise it is completely functional for use against any type of binary as it is using readelf and shell scripting for ASLR related items. I'll address more detail in the other patch adding the tool when we clean that up. > > > + ret = subprocess.check_output(cmd, > > + stderr=open(os.devnull, "w"), > > + cwd=builddir, > > + env={"LANG": "C"}) > > + return ret > > Perhaps this function should also do the json.loads and return only > out["file"] to avoid duplicating this everywhere in the below classes ? > > Also, using inheritance would be better here: > > class TestHardeningBase(infra.basetest.BRTest): > config = """ > ... the base defconfig ... > """ > > checksec_files = ["usr/sbin/lighttpd", "bin/busybox"] > > def checksec_run(self, target_file): > filepath = os.path.join(self.builddir, "target", target_file) > ... > > class TestHardeningRelRo(TestHardeningBase): > config = TestHardeningBase.config + > """ > ... > """ > > def test_run(): > for f in checksec_files: > out = self.checksec_run(f) > self.assertEqual(out["relro"], "full") > self.assertEqual(out["pie"], "yes") > > and ditto for the other tests. > Thanks for the suggestion, I'll take a look at reworking. My python intuition isn't the best :-) Matt
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index e80491cdde..49f83918d6 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -258,6 +258,12 @@ zynq_zybo_defconfig: *defconfig tests.boot.test_atf.TestATFAllwinner: *runtime_test tests.boot.test_atf.TestATFMarvell: *runtime_test tests.boot.test_atf.TestATFVexpress: *runtime_test +tests.core.test_hardening.TestFortifyConserv: *runtime_test +tests.core.test_hardening.TestFortifyNone: *runtime_test +tests.core.test_hardening.TestRelro: *runtime_test +tests.core.test_hardening.TestRelroPartial: *runtime_test +tests.core.test_hardening.TestSspNone: *runtime_test +tests.core.test_hardening.TestSspStrong: *runtime_test tests.core.test_post_scripts.TestPostScripts: *runtime_test tests.core.test_rootfs_overlay.TestRootfsOverlay: *runtime_test tests.core.test_timezone.TestGlibcAllTimezone: *runtime_test diff --git a/support/testing/tests/core/test_hardening.py b/support/testing/tests/core/test_hardening.py new file mode 100644 index 0000000000..d3eb0941d3 --- /dev/null +++ b/support/testing/tests/core/test_hardening.py @@ -0,0 +1,112 @@ +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") + 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") + self.assertEqual(out["file"]["pie"], "no") + + +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")
Catch the commonly used options of SSP, Relro, and fortify. Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com> --- Changes v1 -> v2 [Ricardo - Fix flake8 warnings - Added missing busyfox pie assertions - Updated the yml to include new test cases --- .gitlab-ci.yml | 6 + support/testing/tests/core/test_hardening.py | 112 +++++++++++++++++++ 2 files changed, 118 insertions(+) create mode 100644 support/testing/tests/core/test_hardening.py