From 2e9e6377f10ab1ca5021f6ec964d22161469ba60 Mon Sep 17 00:00:00 2001 From: fanquake Date: Fri, 10 Jul 2020 20:53:28 +0800 Subject: [PATCH 1/2] build: add -Wl,-z,separate-code to hardening flags This flag was added to binutils/ld in the 2.30 release, see commit c11c786f0b45617bb8807ab6a57220d5ff50e414: > The new "-z separate-code" option will generate separate code LOAD segment which must be in wholly disjoint pages from any other data. It was made the default for Linux/x86 targets in the 2.31 release, see commit f6aec96dce1ddbd8961a3aa8a2925db2021719bb: > This patch adds --enable-separate-code to ld configure to turn on -z separate-code by default and enables it by default for Linux/x86. This avoids mixing code pages with data to improve cache performance as well as security. > To reduce x86-64 executable and shared object sizes, the maximum page size is reduced from 2MB to 4KB when -z separate-code is turned on by default. Note: -z max-page-size= can be used to set the maximum page size. > We compared SPEC CPU 2017 performance before and after this change on Skylake server. There are no any significant performance changes. Everything is mostly below +/-1%. Support was also added to LLVMs lld: https://reviews.llvm.org/D64903, however there is remains off by default. There were concerns about an increase in binary size, however in our case, the increase (1 page worth of bytes) would seem negligible, given we are shipping a multi-megabyte binary, which then downloads 100's of GBs of data. Also note that most recent versions of distros are shipping a new enough version of binutils that this is available and/or on by default (assuming the distro has not turned it off, I haven't checked everywhere): CentOS 8: 2.30 Debian Buster 2.31.1 Fedora 29: 2.31.1 FreeBSD: 2.33 GNU Guix: 2.33 / 2.34 Ubuntu 18.04: 2.30 Related threads / discussion: https://bugzilla.redhat.com/show_bug.cgi?id=1623218 --- configure.ac | 1 + 1 file changed, 1 insertion(+) diff --git a/configure.ac b/configure.ac index c6b8aafef9..f11f2b2059 100644 --- a/configure.ac +++ b/configure.ac @@ -785,6 +785,7 @@ if test x$use_hardening != xno; then AX_CHECK_LINK_FLAG([[-Wl,--high-entropy-va]], [HARDENED_LDFLAGS="$HARDENED_LDFLAGS -Wl,--high-entropy-va"],, [[$LDFLAG_WERROR]]) AX_CHECK_LINK_FLAG([[-Wl,-z,relro]], [HARDENED_LDFLAGS="$HARDENED_LDFLAGS -Wl,-z,relro"],, [[$LDFLAG_WERROR]]) AX_CHECK_LINK_FLAG([[-Wl,-z,now]], [HARDENED_LDFLAGS="$HARDENED_LDFLAGS -Wl,-z,now"],, [[$LDFLAG_WERROR]]) + AX_CHECK_LINK_FLAG([[-Wl,-z,separate-code]], [HARDENED_LDFLAGS="$HARDENED_LDFLAGS -Wl,-z,separate-code"],, [[$LDFLAG_WERROR]]) AX_CHECK_LINK_FLAG([[-fPIE -pie]], [PIE_FLAGS="-fPIE"; HARDENED_LDFLAGS="$HARDENED_LDFLAGS -pie"],, [[$CXXFLAG_WERROR]]) case $host in From 65d0f1a53354fb25c8152ee5b430cf57e6508594 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 22 Jul 2020 16:00:07 +0200 Subject: [PATCH 2/2] devtools: Add security check for separate_code Check that sections are appropriately separated in virtual memory, based on their (expected) permissions. This checks for missing -Wl,-z,separate-code and potentially other problems. Co-authored-by: fanquake --- contrib/devtools/security-check.py | 104 +++++++++++++++++++++--- contrib/devtools/test-security-check.py | 12 +-- 2 files changed, 99 insertions(+), 17 deletions(-) diff --git a/contrib/devtools/security-check.py b/contrib/devtools/security-check.py index ca587ca9e5..dc74de9198 100755 --- a/contrib/devtools/security-check.py +++ b/contrib/devtools/security-check.py @@ -40,25 +40,48 @@ def get_ELF_program_headers(executable): stdout = run_command([READELF_CMD, '-l', '-W', executable]) in_headers = False - count = 0 headers = [] for line in stdout.splitlines(): if line.startswith('Program Headers:'): in_headers = True + count = 0 if line == '': in_headers = False if in_headers: if count == 1: # header line - ofs_typ = line.find('Type') - ofs_offset = line.find('Offset') - ofs_flags = line.find('Flg') - ofs_align = line.find('Align') - if ofs_typ == -1 or ofs_offset == -1 or ofs_flags == -1 or ofs_align == -1: + header = [x.strip() for x in line.split()] + ofs_typ = header.index('Type') + ofs_flags = header.index('Flg') + # assert readelf output is what we expect + if ofs_typ == -1 or ofs_flags == -1: raise ValueError('Cannot parse elfread -lW output') elif count > 1: - typ = line[ofs_typ:ofs_offset].rstrip() - flags = line[ofs_flags:ofs_align].rstrip() - headers.append((typ, flags)) + splitline = [x.strip() for x in line.split()] + typ = splitline[ofs_typ] + if not typ.startswith('[R'): # skip [Requesting ...] + splitline = [x.strip() for x in line.split()] + flags = splitline[ofs_flags] + # check for 'R', ' E' + if splitline[ofs_flags + 1] is 'E': + flags += ' E' + headers.append((typ, flags, [])) + count += 1 + + if line.startswith(' Section to Segment mapping:'): + in_mapping = True + count = 0 + if line == '': + in_mapping = False + if in_mapping: + if count == 1: # header line + ofs_segment = line.find('Segment') + ofs_sections = line.find('Sections...') + if ofs_segment == -1 or ofs_sections == -1: + raise ValueError('Cannot parse elfread -lW output') + elif count > 1: + segment = int(line[ofs_segment:ofs_sections].strip()) + sections = line[ofs_sections:].strip().split() + headers[segment][2].extend(sections) count += 1 return headers @@ -68,7 +91,7 @@ def check_ELF_NX(executable) -> bool: ''' have_wx = False have_gnu_stack = False - for (typ, flags) in get_ELF_program_headers(executable): + for (typ, flags, _) in get_ELF_program_headers(executable): if typ == 'GNU_STACK': have_gnu_stack = True if 'W' in flags and 'E' in flags: # section is both writable and executable @@ -82,7 +105,7 @@ def check_ELF_RELRO(executable) -> bool: Dynamic section must have BIND_NOW flag ''' have_gnu_relro = False - for (typ, flags) in get_ELF_program_headers(executable): + for (typ, flags, _) in get_ELF_program_headers(executable): # Note: not checking flags == 'R': here as linkers set the permission differently # This does not affect security: the permission flags of the GNU_RELRO program # header are ignored, the PT_LOAD header determines the effective permissions. @@ -113,6 +136,62 @@ def check_ELF_Canary(executable) -> bool: ok = True return ok +def check_ELF_separate_code(executable): + ''' + Check that sections are appropriately separated in virtual memory, + based on their permissions. This checks for missing -Wl,-z,separate-code + and potentially other problems. + ''' + EXPECTED_FLAGS = { + # Read + execute + '.init': 'R E', + '.plt': 'R E', + '.plt.got': 'R E', + '.plt.sec': 'R E', + '.text': 'R E', + '.fini': 'R E', + # Read-only data + '.interp': 'R', + '.note.gnu.property': 'R', + '.note.gnu.build-id': 'R', + '.note.ABI-tag': 'R', + '.gnu.hash': 'R', + '.dynsym': 'R', + '.dynstr': 'R', + '.gnu.version': 'R', + '.gnu.version_r': 'R', + '.rela.dyn': 'R', + '.rela.plt': 'R', + '.rodata': 'R', + '.eh_frame_hdr': 'R', + '.eh_frame': 'R', + '.qtmetadata': 'R', + '.gcc_except_table': 'R', + '.stapsdt.base': 'R', + # Writable data + '.init_array': 'RW', + '.fini_array': 'RW', + '.dynamic': 'RW', + '.got': 'RW', + '.data': 'RW', + '.bss': 'RW', + } + # For all LOAD program headers get mapping to the list of sections, + # and for each section, remember the flags of the associated program header. + flags_per_section = {} + for (typ, flags, sections) in get_ELF_program_headers(executable): + if typ == 'LOAD': + for section in sections: + assert(section not in flags_per_section) + flags_per_section[section] = flags + # Spot-check ELF LOAD program header flags per section + # If these sections exist, check them against the expected R/W/E flags + for (section, flags) in flags_per_section.items(): + if section in EXPECTED_FLAGS: + if EXPECTED_FLAGS[section] != flags: + return False + return True + def get_PE_dll_characteristics(executable) -> int: '''Get PE DllCharacteristics bits''' stdout = run_command([OBJDUMP_CMD, '-x', executable]) @@ -225,7 +304,8 @@ CHECKS = { ('PIE', check_ELF_PIE), ('NX', check_ELF_NX), ('RELRO', check_ELF_RELRO), - ('Canary', check_ELF_Canary) + ('Canary', check_ELF_Canary), + ('separate_code', check_ELF_separate_code), ], 'PE': [ ('DYNAMIC_BASE', check_PE_DYNAMIC_BASE), diff --git a/contrib/devtools/test-security-check.py b/contrib/devtools/test-security-check.py index 629eba4f28..ec2d886653 100755 --- a/contrib/devtools/test-security-check.py +++ b/contrib/devtools/test-security-check.py @@ -31,15 +31,17 @@ class TestSecurityChecks(unittest.TestCase): cc = 'gcc' write_testcode(source) - self.assertEqual(call_security_check(cc, source, executable, ['-Wl,-zexecstack','-fno-stack-protector','-Wl,-znorelro','-no-pie','-fno-PIE']), + self.assertEqual(call_security_check(cc, source, executable, ['-Wl,-zexecstack','-fno-stack-protector','-Wl,-znorelro','-no-pie','-fno-PIE', '-Wl,-z,separate-code']), (1, executable+': failed PIE NX RELRO Canary')) - self.assertEqual(call_security_check(cc, source, executable, ['-Wl,-znoexecstack','-fno-stack-protector','-Wl,-znorelro','-no-pie','-fno-PIE']), + self.assertEqual(call_security_check(cc, source, executable, ['-Wl,-znoexecstack','-fno-stack-protector','-Wl,-znorelro','-no-pie','-fno-PIE', '-Wl,-z,separate-code']), (1, executable+': failed PIE RELRO Canary')) - self.assertEqual(call_security_check(cc, source, executable, ['-Wl,-znoexecstack','-fstack-protector-all','-Wl,-znorelro','-no-pie','-fno-PIE']), + self.assertEqual(call_security_check(cc, source, executable, ['-Wl,-znoexecstack','-fstack-protector-all','-Wl,-znorelro','-no-pie','-fno-PIE', '-Wl,-z,separate-code']), (1, executable+': failed PIE RELRO')) - self.assertEqual(call_security_check(cc, source, executable, ['-Wl,-znoexecstack','-fstack-protector-all','-Wl,-znorelro','-pie','-fPIE']), + self.assertEqual(call_security_check(cc, source, executable, ['-Wl,-znoexecstack','-fstack-protector-all','-Wl,-znorelro','-pie','-fPIE', '-Wl,-z,separate-code']), (1, executable+': failed RELRO')) - self.assertEqual(call_security_check(cc, source, executable, ['-Wl,-znoexecstack','-fstack-protector-all','-Wl,-zrelro','-Wl,-z,now','-pie','-fPIE']), + self.assertEqual(call_security_check(cc, source, executable, ['-Wl,-znoexecstack','-fstack-protector-all','-Wl,-zrelro','-Wl,-z,now','-pie','-fPIE', '-Wl,-z,noseparate-code']), + (1, executable+': failed separate_code')) + self.assertEqual(call_security_check(cc, source, executable, ['-Wl,-znoexecstack','-fstack-protector-all','-Wl,-zrelro','-Wl,-z,now','-pie','-fPIE', '-Wl,-z,separate-code']), (0, '')) def test_PE(self):