diff options
author | Wladimir J. van der Laan <laanwj@protonmail.com> | 2020-05-13 16:12:34 +0200 |
---|---|---|
committer | Wladimir J. van der Laan <laanwj@protonmail.com> | 2020-05-13 19:35:25 +0200 |
commit | 5d18c0ae18272f371fe44c62a43574288719ed7e (patch) | |
tree | d746ed57bf87cdbdacd6b406c24d2442351eba5a /src | |
parent | 99c03728c07ce8d51b971ae7a508d428b4c24ea4 (diff) | |
parent | df6bde031b24112abf3a94337a2c096698acde6e (diff) | |
download | bitcoin-5d18c0ae18272f371fe44c62a43574288719ed7e.tar.xz |
Merge #18862: Remove fdelt_chk back-compat code and sanity check
df6bde031b24112abf3a94337a2c096698acde6e test: remove glibc fdelt sanity check (fanquake)
8bf1540cc235fb8fb5330a7ae8ab638247ceb177 build: remove fdelt_chk backwards compatibility code (fanquake)
Pull request description:
https://github.com/bitcoin/bitcoin/commit/ae30d40e50e9d63d875d29d54d22147b09fc420c
The return type of [`fdelt_chk`](https://sourceware.org/git/?p=glibc.git;a=blob;f=debug/fdelt_chk.c;h=f62ce7349707cb68f55831c1c591fd7387a90258;hb=HEAD) changed from `unsigned long int` to `long int` in glibc 2.16. See [this commit](https://sourceware.org/git/?p=glibc.git;a=commit;h=ceb9e56b3d1f8c1922e0526c2e841373843460e2). Now that we require [glibc >=2.17](https://github.com/bitcoin/bitcoin/pull/17538) we can remove our back-compat code.
https://github.com/bitcoin/bitcoin/commit/ab7bce584ae02bf25e2e91aa54f9b0249427127d
While looking at the above changes, I noticed that our glibc fdelt sanity check doesn't seem to be checking anything. `fdelt_warn()` also isn't something we'd want to actually "trigger" at runtime, as doing so would cause `bitcoind` to abort.
The comments:
> // trigger: Call FD_SET to trigger __fdelt_chk. FORTIFY_SOURCE must be defined
> // as >0 and optimizations must be set to at least -O2.
suggest calling FD_SET to check the invocation of `fdelt_chk` (this is [aliased with fdelt_warn in glibc](https://sourceware.org/git/?p=glibc.git;a=blob;f=debug/fdelt_chk.c;h=f62ce7349707cb68f55831c1c591fd7387a90258;hb=HEAD)). However just calling `FD_SET()` will not necessarily cause the compiler to insert a call to `fd_warn()`.
Whether or not GCC (recent Clang should work, but may use different heuristics) inserts a call to `fdelt_warn()` depends on if the compiler can determine if the value passed in is a compile time constant (using [`__builtin_constant_p`](https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html)) and whether the value is < 0 or >= `FD_SETSIZE`. The glibc implementation is [here](https://sourceware.org/git/?p=glibc.git;a=blob;f=misc/bits/select2.h;h=7e17430ed94dd1679af10afa3d74795f9c97c0e8;hb=HEAD). This means our check should never cause a call to be inserted.
Compiling master without `--glibc-back-compat` (if you do pass `--glibc-back-compat` the outcome is still the same; however the abort will only happen with >=`FD_SETSIZE` as that is what our [fdelt_warn()](https://github.com/bitcoin/bitcoin/blob/master/src/compat/glibc_compat.cpp#L24) checks for), there are no calls to `fdelt_warn()` inserted by the compiler:
```bash
objdump -dC bitcoind | grep sanity_fdelt
...
0000000000399d20 <sanity_test_fdelt()>:
399d20: 48 81 ec 98 00 00 00 sub $0x98,%rsp
399d27: b9 10 00 00 00 mov $0x10,%ecx
399d2c: 64 48 8b 04 25 28 00 mov %fs:0x28,%rax
399d33: 00 00
399d35: 48 89 84 24 88 00 00 mov %rax,0x88(%rsp)
399d3c: 00
399d3d: 31 c0 xor %eax,%eax
399d3f: 48 89 e7 mov %rsp,%rdi
399d42: fc cld
399d43: f3 48 ab rep stos %rax,%es:(%rdi)
399d46: 48 8b 84 24 88 00 00 mov 0x88(%rsp),%rax
399d4d: 00
399d4e: 64 48 33 04 25 28 00 xor %fs:0x28,%rax
399d55: 00 00
399d57: 75 0d jne 399d66 <sanity_test_fdelt()+0x46>
399d59: b8 01 00 00 00 mov $0x1,%eax
399d5e: 48 81 c4 98 00 00 00 add $0x98,%rsp
399d65: c3 retq
399d66: e8 85 df c8 ff callq 27cf0 <__stack_chk_fail@plt>
399d6b: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
```
If you modify the sanity test to pass `-1` or `FD_SETSIZE` to `FD_SET`, you'll see calls to `fdelt_warn` inserted, and the runtime behaviour is an abort as expected.
```diff
diff --git a/src/compat/glibc_sanity_fdelt.cpp b/src/compat/glibc_sanity_fdelt.cpp
index 87140d0c7..16974bfa0 100644
--- a/src/compat/glibc_sanity_fdelt.cpp
+++ b/src/compat/glibc_sanity_fdelt.cpp
@@ -20,7 +20,7 @@ bool sanity_test_fdelt()
{
fd_set fds;
FD_ZERO(&fds);
- FD_SET(0, &fds);
+ FD_SET(FD_SETSIZE, &fds);
return FD_ISSET(0, &fds);
}
#endif
```
```bash
0000000000399d20 <sanity_test_fdelt()>:
399d20: 48 81 ec 98 00 00 00 sub $0x98,%rsp
399d27: b9 10 00 00 00 mov $0x10,%ecx
399d2c: 64 48 8b 04 25 28 00 mov %fs:0x28,%rax
399d33: 00 00
399d35: 48 89 84 24 88 00 00 mov %rax,0x88(%rsp)
399d3c: 00
399d3d: 31 c0 xor %eax,%eax
399d3f: 48 89 e7 mov %rsp,%rdi
399d42: fc cld
399d43: f3 48 ab rep stos %rax,%es:(%rdi)
399d46: 48 c7 c7 ff ff ff ff mov $0xffffffffffffffff,%rdi
399d4d: e8 3e ff ff ff callq 399c90 <__fdelt_warn>
399d52: 0f b6 04 24 movzbl (%rsp),%eax
399d56: 83 e0 01 and $0x1,%eax
399d59: 48 8b 94 24 88 00 00 mov 0x88(%rsp),%rdx
399d60: 00
399d61: 64 48 33 14 25 28 00 xor %fs:0x28,%rdx
399d68: 00 00
399d6a: 75 08 jne 399d74 <sanity_test_fdelt()+0x54>
399d6c: 48 81 c4 98 00 00 00 add $0x98,%rsp
399d73: c3 retq
399d74: e8 77 df c8 ff callq 27cf0 <__stack_chk_fail@plt>
399d79: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
```
```bash
src/bitcoind
*** buffer overflow detected ***: src/bitcoind terminated
Aborted
```
I think the test should should be removed and replaced (if possible) with additional checks in security-check.py. I was thinking about adding a version of [this script](https://github.com/fanquake/core-review/blob/master/fortify.py) as part of the output, but that needs more thought. I'll address this in a follow up.
ACKs for top commit:
laanwj:
ACK df6bde031b24112abf3a94337a2c096698acde6e
Tree-SHA512: d8b3af4f4eb2d6c767ca6e72ece51d0ab9042e1bbdfcbbdb7ad713414df21489ba3217662b531b8bfdac0265d2ce5431abfae6e861b6187d182ff26c6e59b32d
Diffstat (limited to 'src')
-rw-r--r-- | src/Makefile.am | 1 | ||||
-rw-r--r-- | src/compat/glibc_compat.cpp | 13 | ||||
-rw-r--r-- | src/compat/glibc_sanity.cpp | 8 | ||||
-rw-r--r-- | src/compat/glibc_sanity_fdelt.cpp | 26 |
4 files changed, 0 insertions, 48 deletions
diff --git a/src/Makefile.am b/src/Makefile.am index 627df97cad..13812de0e7 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -496,7 +496,6 @@ libbitcoin_util_a_SOURCES = \ support/lockedpool.cpp \ chainparamsbase.cpp \ clientversion.cpp \ - compat/glibc_sanity_fdelt.cpp \ compat/glibc_sanity.cpp \ compat/glibcxx_sanity.cpp \ compat/strnlen.cpp \ diff --git a/src/compat/glibc_compat.cpp b/src/compat/glibc_compat.cpp index 4de4fd7f45..d17de33e86 100644 --- a/src/compat/glibc_compat.cpp +++ b/src/compat/glibc_compat.cpp @@ -9,10 +9,6 @@ #include <cstddef> #include <cstdint> -#if defined(HAVE_SYS_SELECT_H) -#include <sys/select.h> -#endif - // Prior to GLIBC_2.14, memcpy was aliased to memmove. extern "C" void* memmove(void* a, const void* b, size_t c); extern "C" void* memcpy(void* a, const void* b, size_t c) @@ -20,15 +16,6 @@ extern "C" void* memcpy(void* a, const void* b, size_t c) return memmove(a, b, c); } -extern "C" void __chk_fail(void) __attribute__((__noreturn__)); -extern "C" FDELT_TYPE __fdelt_warn(FDELT_TYPE a) -{ - if (a >= FD_SETSIZE) - __chk_fail(); - return a / __NFDBITS; -} -extern "C" FDELT_TYPE __fdelt_chk(FDELT_TYPE) __attribute__((weak, alias("__fdelt_warn"))); - #if defined(__i386__) || defined(__arm__) extern "C" int64_t __udivmoddi4(uint64_t u, uint64_t v, uint64_t* rp); diff --git a/src/compat/glibc_sanity.cpp b/src/compat/glibc_sanity.cpp index cc74f28899..0367b9a53f 100644 --- a/src/compat/glibc_sanity.cpp +++ b/src/compat/glibc_sanity.cpp @@ -8,10 +8,6 @@ #include <cstddef> -#if defined(HAVE_SYS_SELECT_H) -bool sanity_test_fdelt(); -#endif - extern "C" void* memcpy(void* a, const void* b, size_t c); void* memcpy_int(void* a, const void* b, size_t c) { @@ -45,9 +41,5 @@ bool sanity_test_memcpy() bool glibc_sanity_test() { -#if defined(HAVE_SYS_SELECT_H) - if (!sanity_test_fdelt()) - return false; -#endif return sanity_test_memcpy<1025>(); } diff --git a/src/compat/glibc_sanity_fdelt.cpp b/src/compat/glibc_sanity_fdelt.cpp deleted file mode 100644 index 87140d0c71..0000000000 --- a/src/compat/glibc_sanity_fdelt.cpp +++ /dev/null @@ -1,26 +0,0 @@ -// Copyright (c) 2009-2019 The Bitcoin Core developers -// Distributed under the MIT software license, see the accompanying -// file COPYING or http://www.opensource.org/licenses/mit-license.php. - -#if defined(HAVE_CONFIG_H) -#include <config/bitcoin-config.h> -#endif - -#if defined(HAVE_SYS_SELECT_H) -#ifdef HAVE_CSTRING_DEPENDENT_FD_ZERO -#include <cstring> -#endif -#include <sys/select.h> - -// trigger: Call FD_SET to trigger __fdelt_chk. FORTIFY_SOURCE must be defined -// as >0 and optimizations must be set to at least -O2. -// test: Add a file descriptor to an empty fd_set. Verify that it has been -// correctly added. -bool sanity_test_fdelt() -{ - fd_set fds; - FD_ZERO(&fds); - FD_SET(0, &fds); - return FD_ISSET(0, &fds); -} -#endif |