scripts: check for control flow instrumentation in security-check.py

fanquake commented at 3:25 am on February 10, 2021: member

Note that the presence of instrumentation does not mean it will be used, as that is determined at runtime by the CPU. From the Intel control flow enforcement documentation:

The ENDBR32 and ENDBR64 instructions will have the same effect as the NOP instruction on Intel 64 processors that do not support CET. On processors supporting CET, these instructions do not change register or flag state. This allows CET instrumented programs to execute on processors that do not support CET. Even when CET is supported and enabled, these NOP–like instructions do not affect the execution state of the program, do not cause any additional register pressure, and are minimally intrusive from power and performance perspectives.

fanquake added the label Scripts and tools on Feb 10, 2021

fanquake added the label Needs gitian build on Feb 10, 2021

fanquake force-pushed on Feb 10, 2021

fanquake marked this as a draft on Feb 10, 2021

fanquake commented at 7:59 am on February 10, 2021: member

Aside from the missing test additions, this approach is currently too naive. There is likely to be the presence of endbr64 instructions in the .text section, which we do not control (i.e from libc). I’m also not sure that targeting a specific symbol via objdump will work, as that will still dump all output after the symbol (and may require a too newer version of objdump).

laanwj commented at 10:07 am on February 10, 2021: member

There is likely to be the presence of endbr64 instructions in the .text section, which we do not control (i.e from libc).

The check is naive, but maybe enough. Let’s update test-security-check for it and see if it distinguishes between the hardening flag being present or not?

Remember that we do not link libc statically at this point so there should be no instructions from libc in the binary.

in contrib/devtools/security-check.py:202 in 27e98161c6 outdated

197@@ -184,6 +198,14 @@ def check_PE_NX(executable) -> bool:
198     bits = get_PE_dll_characteristics(executable)
199     return (bits & IMAGE_DLL_CHARACTERISTICS_NX_COMPAT) == IMAGE_DLL_CHARACTERISTICS_NX_COMPAT
200 
201+def check_PE_control_flow_instrumentation(executable) -> bool:
202+    stdout = run_command([OBJDUMP_CMD, '-d', '--section=.text', executable])

laanwj commented at 10:08 am on February 10, 2021:

Does PE have a .text section?

fanquake commented at 6:16 am on February 11, 2021:

Yes, see https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#special-sections, and filltering by –section here prevents additional output from objdump.

in contrib/devtools/security-check.py:155 in 27e98161c6 outdated

142@@ -143,6 +143,20 @@ def check_ELF_separate_code(executable):
143                 return False
144     return True
145 
146+def check_ELF_control_flow_instrumentation(executable) -> bool:
147+
148+    elf = pixie.load(executable)
149+
150+    if elf.hdr.e_machine is not pixie.EM_X86_64:
151+        return True

laanwj commented at 10:21 am on February 10, 2021:

At some point we might want to add a ‘skipped’ return status for checks that could not be performed for a platform (they would count as true for the final return value, but might be displayed differently).

fanquake commented at 6:13 am on February 11, 2021:

Agree. I think printing some sort of informational output may be useful.

in contrib/devtools/security-check.py:205 in 27e98161c6 outdated

197@@ -184,6 +198,14 @@ def check_PE_NX(executable) -> bool:
198     bits = get_PE_dll_characteristics(executable)
199     return (bits & IMAGE_DLL_CHARACTERISTICS_NX_COMPAT) == IMAGE_DLL_CHARACTERISTICS_NX_COMPAT
200 
201+def check_PE_control_flow_instrumentation(executable) -> bool:
202+    stdout = run_command([OBJDUMP_CMD, '-d', '--section=.text', executable])
203+
204+    for line in stdout.splitlines():
205+        if 'endbr64' in line:

laanwj commented at 10:22 am on February 10, 2021:

It’s too bad that x86 has variable-length instructions (so it’s tricky to find instruction boundaries), or we could just look in the section contents for the raw instruction instead of parsing text.

fanquake commented at 6:12 am on February 11, 2021:

You’ve implemented this!

in contrib/devtools/security-check.py:282 in 27e98161c6 outdated

278@@ -257,12 +279,14 @@ def check_MACHO_Canary(executable) -> bool:
279     ('RELRO', check_ELF_RELRO),
280     ('Canary', check_ELF_Canary),
281     ('separate_code', check_ELF_separate_code),
282+    ('control_flow', check_ELF_control_flow_instrumentation)

prusnak commented at 10:39 am on February 10, 2021:

0    ('control_flow', check_ELF_control_flow_instrumentation),

fanquake commented at 6:12 am on February 11, 2021:

Thanks. Included these suggestions.

in contrib/devtools/security-check.py:289 in 27e98161c6 outdated

285     ('DYNAMIC_BASE', check_PE_DYNAMIC_BASE),
286     ('HIGH_ENTROPY_VA', check_PE_HIGH_ENTROPY_VA),
287     ('NX', check_PE_NX),
288-    ('RELOC_SECTION', check_PE_RELOC_SECTION)
289+    ('RELOC_SECTION', check_PE_RELOC_SECTION),
290+    ('control_flow', check_PE_control_flow_instrumentation)

prusnak commented at 10:39 am on February 10, 2021:

0    ('control_flow', check_PE_control_flow_instrumentation),

prusnak changes_requested

prusnak commented at 10:40 am on February 10, 2021: contributor

Python supports trailing commas in lists. Using them avoids future changes unrelated to the desired change.

practicalswift commented at 8:55 pm on February 10, 2021: contributor

Concept ACK: hardening good! :)

scripts: add separate_code failure to all calls in ELF test security check b83d5f2f0d

fanquake force-pushed on Feb 11, 2021

fanquake commented at 6:58 am on February 11, 2021: member

I’ve pulled in @laanwj’s changes, as well as added a basic test for the PE binaries. One thing to note is that our test_bitcoin.exe binary doesn’t currently have an endbr64 instruction in main, like all the other PE binaries.

The only endbr64 instructions that come after main are from some ZeroMQ functions:

 0objdump -C --disassemble=main --section=.text src/test/test_bitcoin.exe | rg endbr64 -C5
 1 13c670d:	90                   	nop
 2 13c670e:	90                   	nop
 3 13c670f:	90                   	nop
 4
 500000000013c6710 <_GLOBAL__sub_I__ZN25CZMQNotificationInterfaceC2Ev>:
 6 13c6710:	f3 0f 1e fa          	endbr64 
 7 13c6714:	53                   	push   %rbx
 8 13c6715:	48 83 ec 30          	sub    $0x30,%rsp
 9 13c6719:	48 8b 1d 10 fd 1f 00 	mov    0x1ffd10(%rip),%rbx        # 15c6430 <.refptr.__stack_chk_guard>
10 13c6720:	48 8d 15 c1 25 1f 00 	lea    0x1f25c1(%rip),%rdx        # 15b8ce8 <.rdata+0xb98>
11 13c6727:	48 8d 0d f2 aa 37 00 	lea    0x37aaf2(%rip),%rcx        # 1741220 <CURRENCY_UNIT>
12--
13 13c679d:	90                   	nop
14 13c679e:	90                   	nop
15 13c679f:	90                   	nop
16
1700000000013c67a0 <_GLOBAL__sub_I__ZN27CZMQAbstractPublishNotifier10InitializeEPv>:
18 13c67a0:	f3 0f 1e fa          	endbr64 
19 13c67a4:	53                   	push   %rbx
20 13c67a5:	48 83 ec 30          	sub    $0x30,%rsp
21 13c67a9:	48 8b 1d 80 fc 1f 00 	mov    0x1ffc80(%rip),%rbx        # 15c6430 <.refptr.__stack_chk_guard>
22 13c67b0:	48 8d 0d 09 ab 37 00 	lea    0x37ab09(%rip),%rcx        # 17412c0 <std::__ioinit>
23 13c67b7:	48 8b 03             	mov    (%rbx),%rax
24--
25 13c686d:	90                   	nop
26 13c686e:	90                   	nop
27 13c686f:	90                   	nop
28
2900000000013c6870 <_GLOBAL__sub_I__Z22RegisterZMQRPCCommandsR9CRPCTable>:
30 13c6870:	f3 0f 1e fa          	endbr64 
31 13c6874:	41 54                	push   %r12
32 13c6876:	53                   	push   %rbx
33 13c6877:	48 83 ec 58          	sub    $0x58,%rsp
34 13c687b:	48 8b 1d ae fb 1f 00 	mov    0x1ffbae(%rip),%rbx        # 15c6430 <.refptr.__stack_chk_guard>
35 13c6882:	48 8d 15 57 37 1f 00 	lea    0x1f3757(%rip),%rdx        # 15b9fe0 <.rdata+0x560>
36--
37 13c6936:	48 83 c4 58          	add    $0x58,%rsp
38 13c693a:	5b                   	pop    %rbx
39 13c693b:	41 5c                	pop    %r12
40 13c693d:	c3                   	retq   
41 13c693e:	e8 9d ed cc ff       	callq  10956e0 <__stack_chk_fail>
42 13c6943:	f3 0f 1e fa          	endbr64 
43 13c6947:	48 8b 4c 24 20       	mov    0x20(%rsp),%rcx
44 13c694c:	49 89 c4             	mov    %rax,%r12
45 13c694f:	48 8d 44 24 30       	lea    0x30(%rsp),%rax
46 13c6954:	48 39 c1             	cmp    %rax,%rcx
47 13c6957:	74 05                	je     13c695e <_GLOBAL__sub_I__Z22RegisterZMQRPCCommandsR9CRPCTable+0xee>
48--
49 13c6a2c:	5d                   	pop    %rbp
50 13c6a2d:	41 5c                	pop    %r12
51 13c6a2f:	c3                   	retq   
52
5300000000013c6a30 <_GLOBAL__sub_I__Z8zmqErrorPKc>:
54 13c6a30:	f3 0f 1e fa          	endbr64 
55 13c6a34:	53                   	push   %rbx
56 13c6a35:	48 83 ec 30          	sub    $0x30,%rsp
57 13c6a39:	48 8b 1d f0 f9 1f 00 	mov    0x1ff9f0(%rip),%rbx        # 15c6430 <.refptr.__stack_chk_guard>
58 13c6a40:	48 8d 0d 79 a9 37 00 	lea    0x37a979(%rip),%rcx        # 17413c0 <std::__ioinit>
59 13c6a47:	48 8b 03             	mov    (%rbx),%rax

There are more endbr64 instructions in the binary:

0objdump -d --section=.text src/test/test_bitcoin.exe | rg endbr64 | wc -l
159313

they are all just pre-main in the .text section, and main itself doesn’t have one.

Looks like we also need to fix the linter for the pixie additions:

0contrib/devtools/pixie.py:211: error: Item "None" of "Optional[Dict[int, bytes]]" has no attribute "get"
1contrib/devtools/pixie.py:213: error: Incompatible types in assignment (expression has type "Iterator[None]", variable has type "Generator[Union[bytes, None, Any], None, None]")
2Found 2 errors in 1 file (checked 192 source files)
3^---- failure generated from test/lint/lint-python.sh

fanquake commented at 7:03 am on February 11, 2021: member

I’ve currently moved the security check tests to the multiprocess CI job, which uses Focal.

Another issue is that the CI which currently runs the security check tests uses Bionic, which has a GCC (7.4.0) which does not have the -fcf-protection option. So we’ll need to move the running of the security check tests to one of the CIs using Focal.

 0gcc: error: unrecognized command line option ‘-fcf-protection=none’; did you mean ‘-flto-partition=none’?
 1E
 2======================================================================
 3ERROR: test_ELF (__main__.TestSecurityChecks)
 4----------------------------------------------------------------------
 5Traceback (most recent call last):
 6  File "./contrib/devtools/test-security-check.py", line 34, in test_ELF
 7    self.assertEqual(call_security_check(cc, source, executable, ['-Wl,-zexecstack','-fno-stack-protector','-Wl,-znorelro','-no-pie','-fno-PIE', '-Wl,-z,noseparate-code', '-fcf-protection=none']),
 8  File "./contrib/devtools/test-security-check.py", line 23, in call_security_check
 9    subprocess.run([cc,source,'-o',executable] + options, check=True)
10  File "/usr/lib/python3.6/subprocess.py", line 438, in run
11    output=stdout, stderr=stderr)
12subprocess.CalledProcessError: Command '['gcc', 'test1.c', '-o', 'test1', '-Wl,-zexecstack', '-fno-stack-protector', '-Wl,-znorelro', '-no-pie', '-fno-PIE', '-Wl,-z,noseparate-code', '-fcf-protection=none']' returned non-zero exit status 1.
13
14----------------------------------------------------------------------
15Ran 1 test in 0.046s
16
17FAILED (errors=1)

contrib: Implement ELF control_flow check with pixie, add test

Don't need to do shell out to `objdump` for ELF, we simply check if
`main` starts with the right instruction.

Add a test in `test-security-check.py`.

458e6e6ec0

contrib: add control flow check for PE binaries 2e83109c6c

fanquake force-pushed on Feb 11, 2021

DrahtBot removed the label Needs gitian build on Feb 12, 2021

DrahtBot commented at 9:41 am on March 3, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21664 (contrib: use LIEF for macOS and Windows symbol & security checks by fanquake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

laanwj commented at 9:51 am on March 3, 2021: member

Is there anything specific holding this up?

MarcoFalke deleted a comment on Mar 3, 2021

fanquake commented at 6:00 am on March 4, 2021: member

Is there anything specific holding this up?

Just my lack of getting things done. I was planning on modifying this so we didn’t have to special case test_bitcoin.exe

fanquake commented at 4:48 am on May 9, 2021: member

Going to close this PR, open an issue explaining why control flow instrumentation isn’t always present, and then open separate PRs per platform modifying dependencies and adding the security checks. The way we test for this will also be simpler now that we are using LIEF.

fanquake closed this on May 9, 2021

laanwj referenced this in commit ecf5f2c1a0 on May 14, 2021

sidhujag referenced this in commit d8f8fb752f on May 14, 2021

PastaPastaPasta referenced this in commit bab7b055f9 on Jun 19, 2022

PastaPastaPasta referenced this in commit 5fd6b64e72 on Jun 19, 2022

DrahtBot locked this on Aug 16, 2022

fanquake deleted the branch on Dec 7, 2022

scripts: check for control flow instrumentation in security-check.py #21135

Conflicts