build: add stack-clash and control-flow protection options to hardening flags #18921

pull fanquake wants to merge 2 commits into bitcoin:master from fanquake:add_cfi_to_hardening changing 1 files +8 −0
  1. fanquake commented at 11:28 am on May 9, 2020: member

    Beginning with Ubuntu 19.10, it’s packaged GCC now has some additional hardening options enabled by default (in addition to existing defaults like -fstack-protector-strong and reducing the minimum ssp buffer size). The new additions are-fcf-protection=full and -fstack-clash-protection.

    -fcf-protection=[full|branch|return|none] Enable code instrumentation of control-flow transfers to increase program security by checking that target addresses of control-flow transfer instructions (such as indirect function call, function return, indirect jump) are valid. This prevents diverting the flow of control to an unexpected target. This is intended to protect against such threats as Return-oriented Programming (ROP), and similarly call/jmp-oriented programming (COP/JOP).

    -fstack-clash-protection Generate code to prevent stack clash style attacks. When this option is enabled, the compiler will only allocate one page of stack space at a time and each page is accessed immediately after allocation. Thus, it prevents allocations from jumping over any stack guard page provided by the operating system.

    If your interested you can grab gcc-9_9.3.0-10ubuntu2.debian.tar.xz from https://packages.ubuntu.com/focal/g++-9. The relevant changes are part of the gcc-distro-specs patches, along with the relevant additions to the gcc manages:

    NOTE: In Ubuntu 19.10 and later versions, -fcf-protection is enabled by default for C, C++, ObjC, ObjC++, if none of -fno-cf-protection nor -fcf-protection=* are found.

    NOTE: In Ubuntu 19.10 and later versions, -fstack-clash-protection is enabled by default for C, C++, ObjC, ObjC++, unless -fno-stack-clash-protection is found.

    So, if you’re C++ using GCC on Ubuntu 19.10 or later, these options will be active unless you explicitly opt out. This can be observed with a small test:

    0int main() { return 0; }
    
     0g++ --version 
     1g++ (Ubuntu 9.3.0-10ubuntu2) 9.3.0
     2
     3g++ test.cpp
     4
     5objdump -dC a.out
     6..
     70000000000001129 <main>:
     8    1129:	f3 0f 1e fa          	endbr64 
     9    112d:	55                   	push   %rbp
    10    112e:	48 89 e5             	mov    %rsp,%rbp
    11    1131:	b8 00 00 00 00       	mov    $0x0,%eax
    12    1136:	5d                   	pop    %rbp
    13    1137:	c3                   	retq   
    14    1138:	0f 1f 84 00 00 00 00 	nopl   0x0(%rax,%rax,1)
    15    113f:	00
    16
    17
    18# recompile opting out of control flow protection
    19g++ test.cpp -fcf-protection=none
    20
    21objdump -dC a.out
    22...
    230000000000001129 <main>:
    24    1129:	55                   	push   %rbp
    25    112a:	48 89 e5             	mov    %rsp,%rbp
    26    112d:	b8 00 00 00 00       	mov    $0x0,%eax
    27    1132:	5d                   	pop    %rbp
    28    1133:	c3                   	retq   
    29    1134:	66 2e 0f 1f 84 00 00 	nopw   %cs:0x0(%rax,%rax,1)
    30    113b:	00 00 00 
    31    113e:	66 90                	xchg   %ax,%ax
    

    Note the insertion of an endbr64 instruction when compiling and not opting out. This instruction is part of the Intel Control-flow Enforcement Technology spec, which the GCC control flow implementation is based on.

    If we’re still doing gitian builds for the 0.21.0 and 0.22.0 releases, we’d likely update the gitian image to Ubuntu Focal, which would mean that the GCC used for gitian builds would also be using these options by default. So we should decide whether we want to explicitly turn these options on as part of our hardening options (although not just for this reason), or, we should be opting-out.

    GCC has supported both options since 8.0.0. Clang has supported -fcf-protection from 7.0.0 and will support -fstack-clash-protection in it’s upcoming 11.0.0 release.

  2. fanquake added the label Brainstorming on May 9, 2020
  3. fanquake added the label Build system on May 9, 2020
  4. fanquake added the label Needs gitian build on May 9, 2020
  5. fanquake added the label Needs Guix build on May 9, 2020
  6. in configure.ac:771 in 45a7f4888f outdated
    767@@ -768,6 +768,9 @@ if test x$use_hardening != xno; then
    768   AX_CHECK_COMPILE_FLAG([-Wstack-protector],[HARDENED_CXXFLAGS="$HARDENED_CXXFLAGS -Wstack-protector"])
    769   AX_CHECK_COMPILE_FLAG([-fstack-protector-all],[HARDENED_CXXFLAGS="$HARDENED_CXXFLAGS -fstack-protector-all"])
    770 
    771+  AX_CHECK_COMPILE_FLAG([-fcf-protection],[HARDENED_CXXFLAGS="$HARDENED_CXXFLAGS -fcf-protection=full"])
    


    practicalswift commented at 2:24 pm on May 9, 2020:
    Is the mismatch between -fcf-protection (left) and -fcf-protection=full (right) intentional? :)
  7. sipa commented at 6:02 pm on May 9, 2020: member
    The big question is what - if any - impact these options have on performance?
  8. practicalswift commented at 6:28 pm on May 9, 2020: contributor

    Strongest possible concept ACK for additional hardening: thanks a lot for taking lead on this important work!

    Performance impact would be nice to have quantified as @sipa requested to be able to reason about trade-offs :)

  9. fanquake commented at 11:43 am on May 10, 2020: member

    The big question is what - if any - impact these options have on performance?

    I was hoping I could tag in @jamesob for a couple benchmarks if possible?

  10. DrahtBot commented at 2:18 pm on May 10, 2020: member

    Gitian builds

    File commit 88d8b4e182bfc75e8496f7046af7aab93307b9d0(master) commit 161c933195699f283f6800b0dbefe09d16a155cd(master and this pull)
    *-aarch64-linux-gnu-debug.tar.gz 369f45f49680bb03... b4d2135fc9b073e8...
    *-aarch64-linux-gnu.tar.gz 8815fb42dbd6f035... 177fd9ced6bd602d...
    *-arm-linux-gnueabihf-debug.tar.gz c18e08956f03cbfb... f6ebc519988abffd...
    *-arm-linux-gnueabihf.tar.gz 45e33867d52277e1... a72ddd6e936d135b...
    *-osx-unsigned.dmg c9eeee10330ee2b9... f73bbb9a17b854fe...
    *-osx64.tar.gz 238b1b155bbde568... 88e464ddb830fbe8...
    *-riscv64-linux-gnu-debug.tar.gz 190c87703632edd7... a77a0fea842a79e9...
    *-riscv64-linux-gnu.tar.gz aac282c4af7d5944... 78d8a7dbc8f23f7b...
    *-win64-debug.zip 0a8ac7d8454af240... 25ef4d700bd26bdd...
    *-win64-setup-unsigned.exe 2500497846b0eebd... df5be8d409ce3282...
    *-win64.zip 78402ce9625df621... cdf020765a1177de...
    *-x86_64-linux-gnu-debug.tar.gz 5562d24f28e9ee15... ea88315bc0416c59...
    *-x86_64-linux-gnu.tar.gz cd1c4bb3f22de2c1... c4d0b54fde1b53d0...
    *.tar.gz c84b1941b1714f4f... 20b314b7da36b35b...
    bitcoin-core-linux-0.21-res.yml 9b5de9f3ddb6316d... b5bdc135df557ec5...
    bitcoin-core-osx-0.21-res.yml e431693014049dd8... b376622e0dd37546...
    bitcoin-core-win-0.21-res.yml 75df8c423af3ce9d... 6e142cf994767195...
    linux-build.log 0d674036963b8877... 5b103a54574bd5f2...
    osx-build.log de659e88e298c8ab... 24abd0649ea30b7c...
    win-build.log a01a9c1b8d076f53... f764bcfb00de2704...
    bitcoin-core-linux-0.21-res.yml.diff 15a360a0fa828e73...
    bitcoin-core-osx-0.21-res.yml.diff 693ba84c1a0c2f9f...
    bitcoin-core-win-0.21-res.yml.diff fe51427716a8e54c...
    linux-build.log.diff 9c63d277af01fdab...
    osx-build.log.diff 184b4fafee467a9b...
    win-build.log.diff 258002653b7bdbe6...
  11. DrahtBot removed the label Needs gitian build on May 10, 2020
  12. DrahtBot removed the label Needs Guix build on Jun 18, 2020
  13. fanquake deleted a comment on Jun 18, 2020
  14. MarcoFalke commented at 11:06 am on June 18, 2020: member
     0Making all in src
     1make[1]: Entering directory '/bitcoin/distsrc-x86_64-w64-mingw32/src'
     2make[2]: Entering directory '/bitcoin/distsrc-x86_64-w64-mingw32/src'
     3x86_64-w64-mingw32-g++ -std=c++11 -DHAVE_CONFIG_H -I. -I../src/config   -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -I.  -DBOOST_SP_USE_STD_ATOMIC -DBOOST_AC_USE_STD_ATOMIC -mthreads -I/bitcoin/depends/x86_64-w64-mingw32/share/../include -I./leveldb/include -I./leveldb/helpers/memenv -I./secp256k1/include -I./univalue/include -I/bitcoin/depends/x86_64-w64-mingw32/share/../include/  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -D_MT -DWIN32 -D_WINDOWS -DBOOST_THREAD_USE_LIB -D_WIN32_WINNT=0x0601 -D_FILE_OFFSET_BITS=64  -fstack-reuse=none -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection      -fPIE -pipe -O2 -O2 -g -fno-ident -fno-extended-identifiers -fvisibility=hidden -c -o bitcoind-bitcoind.o `test -f 'bitcoind.cpp' || echo './'`bitcoind.cpp
     4during RTL pass: final
     5In file included from ./logging.h:10,
     6                 from ./util/system.h:21,
     7                 from ./init.h:11,
     8                 from bitcoind.cpp:13:
     9./tinyformat.h: In static member function 'static int tinyformat::detail::FormatArg::toIntImpl(const void*) [with T = std::__cxx11::basic_string<char>]':
    10./tinyformat.h:550:9: internal compiler error: in seh_emit_stackalloc, at config/i386/winnt.c:1043
    11  550 |         }
    12      |         ^
    13Please submit a full bug report,
    14with preprocessed source if appropriate.
    15See <https://gcc.gnu.org/bugs/> for instructions.
    16{standard input}: Assembler messages:
    17{standard input}: Error: open CFI at the end of file; missing .cfi_endproc directive
    18make[2]: *** [Makefile:14690: bitcoind-bitcoind.o] Error 1
    19make[2]: Leaving directory '/bitcoin/distsrc-x86_64-w64-mingw32/src'
    20make[1]: *** [Makefile:18181: all-recursive] Error 1
    21make[1]: Leaving directory '/bitcoin/distsrc-x86_64-w64-mingw32/src'
    22make: *** [Makefile:787: all-recursive] Error 1
    
  15. build: add -fcf-protection=full to hardening options
    Enables code instrumentation of control-flow transfers. Available in
    GCC 8 and Clang 7.
    
    This option is now on by default in Ubuntu GCC as of 19.10.
    076183b36b
  16. build: add -fstack-clash-protection to hardening flags
    This option causes the compiler to insert probes whenever stack space
    is allocated statically or dynamically to reliably detect stack overflows
    and thus mitigate the attack vector that relies on jumping over a stack
    guard page as provided by the operating system.
    
    This option is now enabled by default in Ubuntu GCC as of 19.10.
    
    Available in GCC 8 and Clang 11.
    b536813cef
  17. fanquake force-pushed on Jun 19, 2020
  18. fanquake marked this as ready for review on Jun 19, 2020
  19. fanquake added this to the "Blockers" column in a project

  20. fanquake commented at 10:14 am on June 19, 2020: member
    I’ve modified this to include the test case from https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90458, for -stack-clash-protection, rather than my test case from #19318.
  21. theuni commented at 5:28 pm on June 19, 2020: member

    Setting aside for a moment whether we should enable these, the code changes look good to me.

    I agree with @fanquake’s point here:

    So we should decide whether we want to explicitly turn these options on as part of our hardening options (although not just for this reason), or, we should be opting-out.

    Rather than letting distro’s make the call, we should make a decision and explicitly opt-in or opt-out ourselves.

    Agree with @sipa as well:

    The big question is what - if any - impact these options have on performance?

    With all of the stack safety options we’re tweaking lately, I think it would be helpful to make sure we’re not blowing up a bunch of our chained small+common hot-path functions with wasteful stack guarding operations. The most obvious that come to mind are the byteswapping and endian-changing functions in compat/. We generally assume (I always have, anyway) that those all end up inlined and thus having a negligible performance impact, but I think there’s a slight chance that these options may change that assumption. That’s not to say that we wouldn’t want the additional stack protection safety in that case, only that we’d want to be more mindful of inlining annotations.

    So in addition to running our existing benchmarks, how about adding one specifically to exercise a bunch of those small functions on either side of the fringe of inlining?

  22. fanquake removed this from the "Blockers" column in a project

  23. laanwj commented at 11:09 am on August 13, 2020: member
    I think that we should ideally opt in to these options. If they’re deemed suitable for general distribution use, I don’t think the performance impact can be that substantial. It’s still better to measure, of course. Would be nice to get this in 0.21.0.
  24. laanwj added this to the milestone 0.21.0 on Aug 13, 2020
  25. hebasto commented at 12:26 pm on August 27, 2020: member
    Concept ACK on more code hardening and explicit compiler flag settings.
  26. jamesob commented at 12:41 pm on August 27, 2020: member

    I was hoping I could tag in @jamesob for a couple benchmarks if possible?

    Sorry for the delay here, missed the notification. Will run a few benchmarks.

  27. jamesob commented at 3:31 pm on August 28, 2020: member

    Changes look good to me.

    ibd local range 500000 505000

     011:27:57 james@slug /home/james % g++-8 --version
     1g++-8 (Debian 8.3.0-6) 8.3.0
     2Copyright (C) 2018 Free Software Foundation, Inc.
     3This is free software; see the source for copying conditions.  There is NO
     4warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
     5
     611:28:03 james@slug /home/james % g++-9 --version
     7g++-9 (Debian 9.3.0-17) 9.3.0
     8Copyright (C) 2019 Free Software Foundation, Inc.
     9This is free software; see the source for copying conditions.  There is NO
    10warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
    11
    1211:28:10 james@slug /home/james % grep HARDEN ~/.bitcoinperf/runs/latest/artifacts-build.make.4.gcc-add_cfi_to_hardening.0/config.log
    13HARDENED_CPPFLAGS=' -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2'
    14HARDENED_CXXFLAGS=' -fstack-reuse=none -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection'
    15HARDENED_LDFLAGS=' -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -pie'
    16HARDEN_FALSE='#'
    17HARDEN_TRUE=''
    18
    1911:29:23 james@slug /home/james % grep HARDEN ~/.bitcoinperf/runs/latest/artifacts-build.make.4.gcc-master.0/config.log
    20HARDENED_CPPFLAGS=' -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2'
    21HARDENED_CXXFLAGS=' -fstack-reuse=none -Wstack-protector -fstack-protector-all'
    22HARDENED_LDFLAGS=' -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -pie'
    23HARDEN_FALSE='#'
    24HARDEN_TRUE=''
    

    Note that the peer servicing this IBD test was on my local network, not co-located on the same host (I don’t have enough disk space for that on a single host) but I think that should be fine given that it sort of resembles realistic use and we’re just looking for red flags here.

  28. jamesob commented at 3:54 pm on August 28, 2020: member

    ACK b536813cefc13f5c54a28a7c2fce8c69e89d6624 (jamesob/ackr/18921.1.fanquake.build_add_stack_clash_an)

    Tested locally via bench results above.

  29. laanwj commented at 11:39 am on August 29, 2020: member
    Thanks for doing benchmarks @jamesob ! Code review ACK b536813cefc13f5c54a28a7c2fce8c69e89d6624
  30. laanwj merged this on Aug 29, 2020
  31. laanwj closed this on Aug 29, 2020

  32. fanquake deleted the branch on Aug 30, 2020
  33. sidhujag referenced this in commit f8a3641ea6 on Aug 30, 2020
  34. vasild commented at 9:47 am on August 31, 2020: member

    This seems to be causing

    0clang++: warning: argument unused during compilation: '-fstack-clash-protection' [-Wunused-command-line-argument]
    
  35. fanquake commented at 10:08 am on August 31, 2020: member

    This seems to be causing

    You need to provide more information. Which version of Clang? What is the output of the test in config.log? If your Clang is claiming to support an argument that it doesn’t actually understand, something is wrong. As mentioned in the OP:

    GCC has supported both options since 8.0.0. Clang has supported -fcf-protection from 7.0.0 and will support -fstack-clash-protection in it’s upcoming 11.0.0 release.

    I’ve just re-tested using Clang 10 and as expected, it does not recognize -fstack-clash-protection, and it does not get added to the hardening options:

     0configure:21072: checking whether C++ compiler accepts -fcf-protection=full
     1configure:21091: clang++-10 -std=c++11 -c -g -O2  -fcf-protection=full  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS conftest.cpp >&5
     2configure:21091: $? = 0
     3configure:21099: result: yes
     4configure:21109: checking whether C++ compiler accepts -fstack-clash-protection
     5configure:21121: clang++-10 -std=c++11 -c -g -O2 -O0 -fstack-clash-protection  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS conftest.cpp >&5
     6clang: error: unknown argument: '-fstack-clash-protection'
     7configure:21121: $? = 1
     8configure: failed program was:
     9...
    10HARDENED_CXXFLAGS=' -Wstack-protector -fstack-protector-all -fcf-protection=full'
    
  36. vasild commented at 11:55 am on August 31, 2020: member

    Clang 12 (or I guess would also happen with any version of clang which supports -fstack-clash-protection, ie clang >= 11).

    Relevant snippet from config.log:

    0configure:25508: checking whether C++ compiler accepts -fstack-clash-protection
    1configure:25520: clang++-devel -std=c++11 -c  -O0 -fstack-clash-protection ... conftest.cpp >&5
    2clang++: warning: argument unused during compilation: '-fstack-clash-protection' [-Wunused-command-line-argument]
    3configure:25520: $? = 0
    4configure:25529: result: yes
    

    This should explain:

    0$ clang++-devel -c -Wall -fstack-clash-protection t.cc -o t.o # warning
    1clang++: warning: argument unused during compilation: '-fstack-clash-protection' [-Wunused-command-line-argument]
    2$ clang++-devel -Wall -fstack-clash-protection t.o -o t # no warning
    3$ clang++-devel --version
    4clang version 12.0.0 (git@github.com:freebsd/freebsd-ports.git 708bdc5c29e539ae9797f1c67aaef9aad7b68200)
    
  37. PastaPastaPasta referenced this in commit 44ea680359 on Jun 19, 2022
  38. PastaPastaPasta referenced this in commit e1b091be3b on Jun 19, 2022
  39. DrahtBot locked this on Aug 16, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-05 22:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me