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

fanquake · 2021-02-10T03:25:21Z

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 · 2021-02-10T07:59:21Z

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 · 2021-02-10T10:07:42Z

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.

laanwj · 2021-02-10T10:08:20Z

contrib/devtools/security-check.py

@@ -184,6 +198,14 @@ def check_PE_NX(executable) -> bool:
    bits = get_PE_dll_characteristics(executable)
    return (bits & IMAGE_DLL_CHARACTERISTICS_NX_COMPAT) == IMAGE_DLL_CHARACTERISTICS_NX_COMPAT

+def check_PE_control_flow_instrumentation(executable) -> bool:
+    stdout = run_command([OBJDUMP_CMD, '-d', '--section=.text', executable])


Does PE have a .text section?

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.

contrib/devtools/security-check.py

prusnak

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

contrib/devtools/security-check.py

practicalswift · 2021-02-10T20:55:15Z

Concept ACK: hardening good! :)

…check

fanquake · 2021-02-11T06:58:02Z

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:

objdump -C --disassemble=main --section=.text src/test/test_bitcoin.exe | rg endbr64 -C5
 13c670d:	90                   	nop
 13c670e:	90                   	nop
 13c670f:	90                   	nop

00000000013c6710 <_GLOBAL__sub_I__ZN25CZMQNotificationInterfaceC2Ev>:
 13c6710:	f3 0f 1e fa          	endbr64 
 13c6714:	53                   	push   %rbx
 13c6715:	48 83 ec 30          	sub    $0x30,%rsp
 13c6719:	48 8b 1d 10 fd 1f 00 	mov    0x1ffd10(%rip),%rbx        # 15c6430 <.refptr.__stack_chk_guard>
 13c6720:	48 8d 15 c1 25 1f 00 	lea    0x1f25c1(%rip),%rdx        # 15b8ce8 <.rdata+0xb98>
 13c6727:	48 8d 0d f2 aa 37 00 	lea    0x37aaf2(%rip),%rcx        # 1741220 <CURRENCY_UNIT>
--
 13c679d:	90                   	nop
 13c679e:	90                   	nop
 13c679f:	90                   	nop

00000000013c67a0 <_GLOBAL__sub_I__ZN27CZMQAbstractPublishNotifier10InitializeEPv>:
 13c67a0:	f3 0f 1e fa          	endbr64 
 13c67a4:	53                   	push   %rbx
 13c67a5:	48 83 ec 30          	sub    $0x30,%rsp
 13c67a9:	48 8b 1d 80 fc 1f 00 	mov    0x1ffc80(%rip),%rbx        # 15c6430 <.refptr.__stack_chk_guard>
 13c67b0:	48 8d 0d 09 ab 37 00 	lea    0x37ab09(%rip),%rcx        # 17412c0 <std::__ioinit>
 13c67b7:	48 8b 03             	mov    (%rbx),%rax
--
 13c686d:	90                   	nop
 13c686e:	90                   	nop
 13c686f:	90                   	nop

00000000013c6870 <_GLOBAL__sub_I__Z22RegisterZMQRPCCommandsR9CRPCTable>:
 13c6870:	f3 0f 1e fa          	endbr64 
 13c6874:	41 54                	push   %r12
 13c6876:	53                   	push   %rbx
 13c6877:	48 83 ec 58          	sub    $0x58,%rsp
 13c687b:	48 8b 1d ae fb 1f 00 	mov    0x1ffbae(%rip),%rbx        # 15c6430 <.refptr.__stack_chk_guard>
 13c6882:	48 8d 15 57 37 1f 00 	lea    0x1f3757(%rip),%rdx        # 15b9fe0 <.rdata+0x560>
--
 13c6936:	48 83 c4 58          	add    $0x58,%rsp
 13c693a:	5b                   	pop    %rbx
 13c693b:	41 5c                	pop    %r12
 13c693d:	c3                   	retq   
 13c693e:	e8 9d ed cc ff       	callq  10956e0 <__stack_chk_fail>
 13c6943:	f3 0f 1e fa          	endbr64 
 13c6947:	48 8b 4c 24 20       	mov    0x20(%rsp),%rcx
 13c694c:	49 89 c4             	mov    %rax,%r12
 13c694f:	48 8d 44 24 30       	lea    0x30(%rsp),%rax
 13c6954:	48 39 c1             	cmp    %rax,%rcx
 13c6957:	74 05                	je     13c695e <_GLOBAL__sub_I__Z22RegisterZMQRPCCommandsR9CRPCTable+0xee>
--
 13c6a2c:	5d                   	pop    %rbp
 13c6a2d:	41 5c                	pop    %r12
 13c6a2f:	c3                   	retq   

00000000013c6a30 <_GLOBAL__sub_I__Z8zmqErrorPKc>:
 13c6a30:	f3 0f 1e fa          	endbr64 
 13c6a34:	53                   	push   %rbx
 13c6a35:	48 83 ec 30          	sub    $0x30,%rsp
 13c6a39:	48 8b 1d f0 f9 1f 00 	mov    0x1ff9f0(%rip),%rbx        # 15c6430 <.refptr.__stack_chk_guard>
 13c6a40:	48 8d 0d 79 a9 37 00 	lea    0x37a979(%rip),%rcx        # 17413c0 <std::__ioinit>
 13c6a47:	48 8b 03             	mov    (%rbx),%rax

There are more endbr64 instructions in the binary:

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

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:

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

fanquake · 2021-02-11T07:03:34Z

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.

gcc: error: unrecognized command line option ‘-fcf-protection=none’; did you mean ‘-flto-partition=none’?
E
======================================================================
ERROR: test_ELF (__main__.TestSecurityChecks)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./contrib/devtools/test-security-check.py", line 34, in test_ELF
    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']),
  File "./contrib/devtools/test-security-check.py", line 23, in call_security_check
    subprocess.run([cc,source,'-o',executable] + options, check=True)
  File "/usr/lib/python3.6/subprocess.py", line 438, in run
    output=stdout, stderr=stderr)
subprocess.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.

----------------------------------------------------------------------
Ran 1 test in 0.046s

FAILED (errors=1)

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

DrahtBot · 2021-03-03T09:41:18Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

contrib: use LIEF for macOS and Windows symbol & security checks #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 · 2021-03-03T09:51:46Z

Is there anything specific holding this up?

fanquake · 2021-03-04T06:00:31Z

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 · 2021-05-09T04:48:43Z

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.

42b589d scripts: test for MACHO control flow instrumentation (fanquake) 469a5bc build: build Boost with -fcf-protection when targeting Darwin (fanquake) Pull request description: Addresses the macOS portion of #21888. Build Boost with `-fcf-protection` when targeting Darwin. This should be ok, because our cross-compiler (Clang 10) supports the option, and I'd expect all versions of Apple Clang being used to compile Core would also support it. Building Boost with this option is required so that the `main` provided to `test_bitcoin` has instrumentation. 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. Follow up from #21135. Guix builds: ```bash 663df8471400f06d4da739e39a886aa17f56a36d66e0ff7cc290686294ef39c9 guix-build-42b589d18fed/output/dist-archive/bitcoin-42b589d18fed.tar.gz 45e841661e1659a634468b6f8c9fb0a7956c31ba296f1fd0c02cd880736d6127 guix-build-42b589d18fed/output/x86_64-apple-darwin18/bitcoin-42b589d18fed-osx-unsigned.dmg 0ea85c99fef35429a5048fa14850bce6b900eaa887aeea419b019852f8d2be78 guix-build-42b589d18fed/output/x86_64-apple-darwin18/bitcoin-42b589d18fed-osx-unsigned.tar.gz 85857a5a4a5d4d3a172d6c361c12c4a94f6505fc12b527ea63b75bfe54ee1001 guix-build-42b589d18fed/output/x86_64-apple-darwin18/bitcoin-42b589d18fed-osx64.tar.gz ``` Gitian builds: ```bash # macOS: bdfd677a6b88273a741b433e1e7f554af50cc76b3342d44ab0c441e2b40efc96 bitcoin-42b589d18fed-osx-unsigned.dmg f3b2d09f3bea7a5cc489b02e8e53dd76a9922338500fae79cad0506655af56f9 bitcoin-42b589d18fed-osx-unsigned.tar.gz 29d5ad5e46bc9fb0056922a8b47c026e5e9f71e6cf447203b74644587d6fb6f7 bitcoin-42b589d18fed-osx64.tar.gz 663df8471400f06d4da739e39a886aa17f56a36d66e0ff7cc290686294ef39c9 src/bitcoin-42b589d18fed.tar.gz 366f8d7a2fc1f3e22cb1018043099126a71ce65380cc27b1c3280cce42d06c98 bitcoin-core-osx-22-res.yml ``` ACKs for top commit: laanwj: Code review ACK 42b589d Tree-SHA512: 12cb8d462d64d845b9fe48c5c6978892adff8bf5b5572bb29f35df1f6176e47b32a68bcb6e4883c7d9454e76e8868851005a7325916852a2d0d32659ac7dae3f

42b589d scripts: test for MACHO control flow instrumentation (fanquake) 469a5bc build: build Boost with -fcf-protection when targeting Darwin (fanquake) Pull request description: Addresses the macOS portion of bitcoin#21888. Build Boost with `-fcf-protection` when targeting Darwin. This should be ok, because our cross-compiler (Clang 10) supports the option, and I'd expect all versions of Apple Clang being used to compile Core would also support it. Building Boost with this option is required so that the `main` provided to `test_bitcoin` has instrumentation. 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. Follow up from bitcoin#21135. Guix builds: ```bash 663df8471400f06d4da739e39a886aa17f56a36d66e0ff7cc290686294ef39c9 guix-build-42b589d18fed/output/dist-archive/bitcoin-42b589d18fed.tar.gz 45e841661e1659a634468b6f8c9fb0a7956c31ba296f1fd0c02cd880736d6127 guix-build-42b589d18fed/output/x86_64-apple-darwin18/bitcoin-42b589d18fed-osx-unsigned.dmg 0ea85c99fef35429a5048fa14850bce6b900eaa887aeea419b019852f8d2be78 guix-build-42b589d18fed/output/x86_64-apple-darwin18/bitcoin-42b589d18fed-osx-unsigned.tar.gz 85857a5a4a5d4d3a172d6c361c12c4a94f6505fc12b527ea63b75bfe54ee1001 guix-build-42b589d18fed/output/x86_64-apple-darwin18/bitcoin-42b589d18fed-osx64.tar.gz ``` Gitian builds: ```bash # macOS: bdfd677a6b88273a741b433e1e7f554af50cc76b3342d44ab0c441e2b40efc96 bitcoin-42b589d18fed-osx-unsigned.dmg f3b2d09f3bea7a5cc489b02e8e53dd76a9922338500fae79cad0506655af56f9 bitcoin-42b589d18fed-osx-unsigned.tar.gz 29d5ad5e46bc9fb0056922a8b47c026e5e9f71e6cf447203b74644587d6fb6f7 bitcoin-42b589d18fed-osx64.tar.gz 663df8471400f06d4da739e39a886aa17f56a36d66e0ff7cc290686294ef39c9 src/bitcoin-42b589d18fed.tar.gz 366f8d7a2fc1f3e22cb1018043099126a71ce65380cc27b1c3280cce42d06c98 bitcoin-core-osx-22-res.yml ``` ACKs for top commit: laanwj: Code review ACK 42b589d Tree-SHA512: 12cb8d462d64d845b9fe48c5c6978892adff8bf5b5572bb29f35df1f6176e47b32a68bcb6e4883c7d9454e76e8868851005a7325916852a2d0d32659ac7dae3f

fanquake added Scripts and tools Needs gitian build labels Feb 10, 2021

fanquake force-pushed the security_check_cf_protection branch from 6248569 to 27e9816 Compare February 10, 2021 03:25

fanquake marked this pull request as draft February 10, 2021 07:52

laanwj reviewed Feb 10, 2021

View reviewed changes

contrib/devtools/security-check.py Show resolved Hide resolved

laanwj reviewed Feb 10, 2021

View reviewed changes

contrib/devtools/security-check.py Outdated Show resolved Hide resolved

prusnak suggested changes Feb 10, 2021

View reviewed changes

contrib/devtools/security-check.py Outdated Show resolved Hide resolved

contrib/devtools/security-check.py Outdated Show resolved Hide resolved

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

b83d5f2

…check

fanquake force-pushed the security_check_cf_protection branch from 27e9816 to 2d38140 Compare February 11, 2021 06:12

laanwj and others added 2 commits February 11, 2021 15:08

contrib: Implement ELF control_flow check with pixie, add test

458e6e6

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

contrib: add control flow check for PE binaries

2e83109

fanquake force-pushed the security_check_cf_protection branch from 2d38140 to 2e83109 Compare February 11, 2021 07:08

DrahtBot removed the Needs gitian build label Feb 12, 2021

fanquake mentioned this pull request Feb 22, 2021

contrib: run test-symbol-check for RISC-V #21255

Merged

DrahtBot mentioned this pull request Mar 3, 2021

guix: Test security-check sanity before performing them #20980

Closed

bitcoin deleted a comment from DrahtBot Mar 3, 2021

DrahtBot mentioned this pull request Apr 13, 2021

contrib: use LIEF for macOS and Windows symbol & security checks #21664

Merged

fanquake closed this May 9, 2021

This was referenced May 9, 2021

build: control flow instrumentation not always present in test_bitcoin main #21888

Closed

macho: check for control flow instrumentation #21889

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

fanquake deleted the security_check_cf_protection branch December 7, 2022 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

fanquake commented Feb 10, 2021

Uh oh!

fanquake commented Feb 10, 2021

Uh oh!

laanwj commented Feb 10, 2021

Uh oh!

laanwj Feb 10, 2021

Uh oh!

fanquake Feb 11, 2021

Uh oh!

Uh oh!

Uh oh!

prusnak left a comment

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Feb 10, 2021 •

edited

Loading

Uh oh!

fanquake commented Feb 11, 2021

Uh oh!

fanquake commented Feb 11, 2021 •

edited

Loading

Uh oh!

DrahtBot commented Mar 3, 2021 •

edited

Loading

Uh oh!

laanwj commented Mar 3, 2021

Uh oh!

fanquake commented Mar 4, 2021

Uh oh!

fanquake commented May 9, 2021

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

fanquake commented Feb 10, 2021

Uh oh!

fanquake commented Feb 10, 2021

Uh oh!

laanwj commented Feb 10, 2021

Uh oh!

laanwj Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

fanquake Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

prusnak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Feb 11, 2021

Uh oh!

fanquake commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Mar 3, 2021

Uh oh!

fanquake commented Mar 4, 2021

Uh oh!

fanquake commented May 9, 2021

Uh oh!

Uh oh!

practicalswift commented Feb 10, 2021 •

edited

Loading

fanquake commented Feb 11, 2021 •

edited

Loading

DrahtBot commented Mar 3, 2021 •

edited

Loading