From 061acf62a15ad3dbb9f055b7c2569b9832ed623a Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 24 Mar 2020 10:24:58 +0800 Subject: [PATCH 1/4] scripts: no-longer check for 32 bit windows in security-check.py --- contrib/devtools/security-check.py | 33 +++++++++--------------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/contrib/devtools/security-check.py b/contrib/devtools/security-check.py index 9444271bd..2171ee2e0 100755 --- a/contrib/devtools/security-check.py +++ b/contrib/devtools/security-check.py @@ -116,25 +116,18 @@ def check_ELF_Canary(executable): ok = True return ok -def get_PE_dll_characteristics(executable): - ''' - Get PE DllCharacteristics bits. - Returns a tuple (arch,bits) where arch is 'i386:x86-64' or 'i386' - and bits is the DllCharacteristics value. - ''' +def get_PE_dll_characteristics(executable) -> int: + '''Get PE DllCharacteristics bits''' p = subprocess.Popen([OBJDUMP_CMD, '-x', executable], stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE, universal_newlines=True) (stdout, stderr) = p.communicate() if p.returncode: raise IOError('Error opening file') - arch = '' bits = 0 for line in stdout.splitlines(): tokens = line.split() - if len(tokens)>=2 and tokens[0] == 'architecture:': - arch = tokens[1].rstrip(',') if len(tokens)>=2 and tokens[0] == 'DllCharacteristics': bits = int(tokens[1],16) - return (arch,bits) + return bits IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA = 0x0020 IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE = 0x0040 @@ -142,21 +135,15 @@ IMAGE_DLL_CHARACTERISTICS_NX_COMPAT = 0x0100 def check_PE_DYNAMIC_BASE(executable): '''PIE: DllCharacteristics bit 0x40 signifies dynamicbase (ASLR)''' - (arch,bits) = get_PE_dll_characteristics(executable) - reqbits = IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE - return (bits & reqbits) == reqbits + bits = get_PE_dll_characteristics(executable) + return (bits & IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE) == IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE -# On 64 bit, must support high-entropy 64-bit address space layout randomization in addition to DYNAMIC_BASE -# to have secure ASLR. +# Must support high-entropy 64-bit address space layout randomization +# in addition to DYNAMIC_BASE to have secure ASLR. def check_PE_HIGH_ENTROPY_VA(executable): '''PIE: DllCharacteristics bit 0x20 signifies high-entropy ASLR''' - (arch,bits) = get_PE_dll_characteristics(executable) - if arch == 'i386:x86-64': - reqbits = IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA - else: # Unnecessary on 32-bit - assert(arch == 'i386') - reqbits = 0 - return (bits & reqbits) == reqbits + bits = get_PE_dll_characteristics(executable) + return (bits & IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA) == IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA def check_PE_RELOC_SECTION(executable) -> bool: '''Check for a reloc section. This is required for functional ASLR.''' @@ -171,7 +158,7 @@ def check_PE_RELOC_SECTION(executable) -> bool: def check_PE_NX(executable): '''NX: DllCharacteristics bit 0x100 signifies nxcompat (DEP)''' - (arch,bits) = get_PE_dll_characteristics(executable) + bits = get_PE_dll_characteristics(executable) return (bits & IMAGE_DLL_CHARACTERISTICS_NX_COMPAT) == IMAGE_DLL_CHARACTERISTICS_NX_COMPAT def get_MACHO_executable_flags(executable): From 13f606b4f940e5820ff21ea62fc27a5a91774b05 Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 28 Apr 2020 09:30:35 +0800 Subject: [PATCH 2/4] scripts: remove NONFATAL from security-check.py --- contrib/devtools/security-check.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/contrib/devtools/security-check.py b/contrib/devtools/security-check.py index 2171ee2e0..518023802 100755 --- a/contrib/devtools/security-check.py +++ b/contrib/devtools/security-check.py @@ -15,7 +15,6 @@ import os READELF_CMD = os.getenv('READELF', '/usr/bin/readelf') OBJDUMP_CMD = os.getenv('OBJDUMP', '/usr/bin/objdump') OTOOL_CMD = os.getenv('OTOOL', '/usr/bin/otool') -NONFATAL = {} # checks which are non-fatal for now but only generate a warning def check_ELF_PIE(executable): ''' @@ -279,18 +278,12 @@ if __name__ == '__main__': continue failed = [] - warning = [] for (name, func) in CHECKS[etype]: if not func(filename): - if name in NONFATAL: - warning.append(name) - else: - failed.append(name) + failed.append(name) if failed: print('%s: failed %s' % (filename, ' '.join(failed))) retval = 1 - if warning: - print('%s: warning %s' % (filename, ' '.join(warning))) except IOError: print('%s: cannot open' % filename) retval = 1 From 83d063e9541cc9ea41ea86919eb9435c73efb14e Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 28 Apr 2020 09:48:05 +0800 Subject: [PATCH 3/4] scripts: add run_command to security-check.py Deduplicate all the subprocess code as mentioned in 18713. --- contrib/devtools/security-check.py | 58 +++++++++++------------------- 1 file changed, 21 insertions(+), 37 deletions(-) diff --git a/contrib/devtools/security-check.py b/contrib/devtools/security-check.py index 518023802..c03e5d019 100755 --- a/contrib/devtools/security-check.py +++ b/contrib/devtools/security-check.py @@ -16,14 +16,15 @@ READELF_CMD = os.getenv('READELF', '/usr/bin/readelf') OBJDUMP_CMD = os.getenv('OBJDUMP', '/usr/bin/objdump') OTOOL_CMD = os.getenv('OTOOL', '/usr/bin/otool') +def run_command(command): + p = subprocess.run(command, stdout=subprocess.PIPE, check=True, universal_newlines=True) + return p.stdout + def check_ELF_PIE(executable): ''' Check for position independent executable (PIE), allowing for address space randomization. ''' - p = subprocess.Popen([READELF_CMD, '-h', '-W', executable], stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE, universal_newlines=True) - (stdout, stderr) = p.communicate() - if p.returncode: - raise IOError('Error opening file') + stdout = run_command([READELF_CMD, '-h', '-W', executable]) ok = False for line in stdout.splitlines(): @@ -34,10 +35,8 @@ def check_ELF_PIE(executable): def get_ELF_program_headers(executable): '''Return type and flags for ELF program headers''' - p = subprocess.Popen([READELF_CMD, '-l', '-W', executable], stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE, universal_newlines=True) - (stdout, stderr) = p.communicate() - if p.returncode: - raise IOError('Error opening file') + stdout = run_command([READELF_CMD, '-l', '-W', executable]) + in_headers = False count = 0 headers = [] @@ -83,7 +82,8 @@ def check_ELF_RELRO(executable): have_gnu_relro = False 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. + # This does not affect security: the permission flags of the GNU_RELRO program + # header are ignored, the PT_LOAD header determines the effective permissions. # However, the dynamic linker need to write to this area so these are RW. # Glibc itself takes care of mprotecting this area R after relocations are finished. # See also https://marc.info/?l=binutils&m=1498883354122353 @@ -91,10 +91,8 @@ def check_ELF_RELRO(executable): have_gnu_relro = True have_bindnow = False - p = subprocess.Popen([READELF_CMD, '-d', '-W', executable], stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE, universal_newlines=True) - (stdout, stderr) = p.communicate() - if p.returncode: - raise IOError('Error opening file') + stdout = run_command([READELF_CMD, '-d', '-W', executable]) + for line in stdout.splitlines(): tokens = line.split() if len(tokens)>1 and tokens[1] == '(BIND_NOW)' or (len(tokens)>2 and tokens[1] == '(FLAGS)' and 'BIND_NOW' in tokens[2:]): @@ -105,10 +103,8 @@ def check_ELF_Canary(executable): ''' Check for use of stack canary ''' - p = subprocess.Popen([READELF_CMD, '--dyn-syms', '-W', executable], stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE, universal_newlines=True) - (stdout, stderr) = p.communicate() - if p.returncode: - raise IOError('Error opening file') + stdout = run_command([READELF_CMD, '--dyn-syms', '-W', executable]) + ok = False for line in stdout.splitlines(): if '__stack_chk_fail' in line: @@ -117,10 +113,8 @@ def check_ELF_Canary(executable): def get_PE_dll_characteristics(executable) -> int: '''Get PE DllCharacteristics bits''' - p = subprocess.Popen([OBJDUMP_CMD, '-x', executable], stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE, universal_newlines=True) - (stdout, stderr) = p.communicate() - if p.returncode: - raise IOError('Error opening file') + stdout = run_command([OBJDUMP_CMD, '-x', executable]) + bits = 0 for line in stdout.splitlines(): tokens = line.split() @@ -146,10 +140,8 @@ def check_PE_HIGH_ENTROPY_VA(executable): def check_PE_RELOC_SECTION(executable) -> bool: '''Check for a reloc section. This is required for functional ASLR.''' - p = subprocess.Popen([OBJDUMP_CMD, '-h', executable], stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE, universal_newlines=True) - (stdout, stderr) = p.communicate() - if p.returncode: - raise IOError('Error opening file') + stdout = run_command([OBJDUMP_CMD, '-h', executable]) + for line in stdout.splitlines(): if '.reloc' in line: return True @@ -161,10 +153,7 @@ def check_PE_NX(executable): return (bits & IMAGE_DLL_CHARACTERISTICS_NX_COMPAT) == IMAGE_DLL_CHARACTERISTICS_NX_COMPAT def get_MACHO_executable_flags(executable): - p = subprocess.Popen([OTOOL_CMD, '-vh', executable], stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE, universal_newlines=True) - (stdout, stderr) = p.communicate() - if p.returncode: - raise IOError('Error opening file') + stdout = run_command([OTOOL_CMD, '-vh', executable]) flags = [] for line in stdout.splitlines(): @@ -208,10 +197,7 @@ def check_MACHO_LAZY_BINDINGS(executable) -> bool: Check for no lazy bindings. We don't use or check for MH_BINDATLOAD. See #18295. ''' - p = subprocess.Popen([OTOOL_CMD, '-l', executable], stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE, universal_newlines=True) - (stdout, stderr) = p.communicate() - if p.returncode: - raise IOError('Error opening file') + stdout = run_command([OTOOL_CMD, '-l', executable]) for line in stdout.splitlines(): tokens = line.split() @@ -224,10 +210,8 @@ def check_MACHO_Canary(executable) -> bool: ''' Check for use of stack canary ''' - p = subprocess.Popen([OTOOL_CMD, '-Iv', executable], stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE, universal_newlines=True) - (stdout, stderr) = p.communicate() - if p.returncode: - raise IOError('Error opening file') + stdout = run_command([OTOOL_CMD, '-Iv', executable]) + ok = False for line in stdout.splitlines(): if '___stack_chk_fail' in line: From eacedfb0230978748cbcfb13817fed7e7c756ba7 Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 28 Apr 2020 10:05:45 +0800 Subject: [PATCH 4/4] scripts: add additional type annotations to security-check.py --- contrib/devtools/security-check.py | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/contrib/devtools/security-check.py b/contrib/devtools/security-check.py index c03e5d019..ca587ca9e 100755 --- a/contrib/devtools/security-check.py +++ b/contrib/devtools/security-check.py @@ -12,15 +12,17 @@ import subprocess import sys import os +from typing import List, Optional + READELF_CMD = os.getenv('READELF', '/usr/bin/readelf') OBJDUMP_CMD = os.getenv('OBJDUMP', '/usr/bin/objdump') OTOOL_CMD = os.getenv('OTOOL', '/usr/bin/otool') -def run_command(command): +def run_command(command) -> str: p = subprocess.run(command, stdout=subprocess.PIPE, check=True, universal_newlines=True) return p.stdout -def check_ELF_PIE(executable): +def check_ELF_PIE(executable) -> bool: ''' Check for position independent executable (PIE), allowing for address space randomization. ''' @@ -28,8 +30,8 @@ def check_ELF_PIE(executable): ok = False for line in stdout.splitlines(): - line = line.split() - if len(line)>=2 and line[0] == 'Type:' and line[1] == 'DYN': + tokens = line.split() + if len(line)>=2 and tokens[0] == 'Type:' and tokens[1] == 'DYN': ok = True return ok @@ -60,7 +62,7 @@ def get_ELF_program_headers(executable): count += 1 return headers -def check_ELF_NX(executable): +def check_ELF_NX(executable) -> bool: ''' Check that no sections are writable and executable (including the stack) ''' @@ -73,7 +75,7 @@ def check_ELF_NX(executable): have_wx = True return have_gnu_stack and not have_wx -def check_ELF_RELRO(executable): +def check_ELF_RELRO(executable) -> bool: ''' Check for read-only relocations. GNU_RELRO program header must exist @@ -99,7 +101,7 @@ def check_ELF_RELRO(executable): have_bindnow = True return have_gnu_relro and have_bindnow -def check_ELF_Canary(executable): +def check_ELF_Canary(executable) -> bool: ''' Check for use of stack canary ''' @@ -126,14 +128,14 @@ IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA = 0x0020 IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE = 0x0040 IMAGE_DLL_CHARACTERISTICS_NX_COMPAT = 0x0100 -def check_PE_DYNAMIC_BASE(executable): +def check_PE_DYNAMIC_BASE(executable) -> bool: '''PIE: DllCharacteristics bit 0x40 signifies dynamicbase (ASLR)''' bits = get_PE_dll_characteristics(executable) return (bits & IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE) == IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE # Must support high-entropy 64-bit address space layout randomization # in addition to DYNAMIC_BASE to have secure ASLR. -def check_PE_HIGH_ENTROPY_VA(executable): +def check_PE_HIGH_ENTROPY_VA(executable) -> bool: '''PIE: DllCharacteristics bit 0x20 signifies high-entropy ASLR''' bits = get_PE_dll_characteristics(executable) return (bits & IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA) == IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA @@ -147,12 +149,12 @@ def check_PE_RELOC_SECTION(executable) -> bool: return True return False -def check_PE_NX(executable): +def check_PE_NX(executable) -> bool: '''NX: DllCharacteristics bit 0x100 signifies nxcompat (DEP)''' bits = get_PE_dll_characteristics(executable) return (bits & IMAGE_DLL_CHARACTERISTICS_NX_COMPAT) == IMAGE_DLL_CHARACTERISTICS_NX_COMPAT -def get_MACHO_executable_flags(executable): +def get_MACHO_executable_flags(executable) -> List[str]: stdout = run_command([OTOOL_CMD, '-vh', executable]) flags = [] @@ -240,7 +242,7 @@ CHECKS = { ] } -def identify_executable(executable): +def identify_executable(executable) -> Optional[str]: with open(filename, 'rb') as f: magic = f.read(4) if magic.startswith(b'MZ'):