https://github.com/bitcoin/bitcoin/commit/ae30d40e50e9d63d875d29d54d22147b09fc420c
The return type of fdelt_chk changed from unsigned long int to long int in glibc 2.16. See this commit. Now that we require glibc >=2.17 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). 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) and whether the value is < 0 or >= FD_SETSIZE. The glibc implementation is here. 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() checks for), there are no calls to fdelt_warn() inserted by the compiler:
0objdump -dC bitcoind | grep sanity_fdelt
1...
20000000000399d20 <sanity_test_fdelt()>:
3 399d20: 48 81 ec 98 00 00 00 sub $0x98,%rsp
4 399d27: b9 10 00 00 00 mov $0x10,%ecx
5 399d2c: 64 48 8b 04 25 28 00 mov %fs:0x28,%rax
6 399d33: 00 00
7 399d35: 48 89 84 24 88 00 00 mov %rax,0x88(%rsp)
8 399d3c: 00
9 399d3d: 31 c0 xor %eax,%eax
10 399d3f: 48 89 e7 mov %rsp,%rdi
11 399d42: fc cld
12 399d43: f3 48 ab rep stos %rax,%es:(%rdi)
13 399d46: 48 8b 84 24 88 00 00 mov 0x88(%rsp),%rax
14 399d4d: 00
15 399d4e: 64 48 33 04 25 28 00 xor %fs:0x28,%rax
16 399d55: 00 00
17 399d57: 75 0d jne 399d66 <sanity_test_fdelt()+0x46>
18 399d59: b8 01 00 00 00 mov $0x1,%eax
19 399d5e: 48 81 c4 98 00 00 00 add $0x98,%rsp
20 399d65: c3 retq
21 399d66: e8 85 df c8 ff callq 27cf0 <__stack_chk_fail@plt>
22 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.
0diff --git a/src/compat/glibc_sanity_fdelt.cpp b/src/compat/glibc_sanity_fdelt.cpp
1index 87140d0c7..16974bfa0 100644
2--- a/src/compat/glibc_sanity_fdelt.cpp
3+++ b/src/compat/glibc_sanity_fdelt.cpp
4@@ -20,7 +20,7 @@ bool sanity_test_fdelt()
5 {
6 fd_set fds;
7 FD_ZERO(&fds);
8- FD_SET(0, &fds);
9+ FD_SET(FD_SETSIZE, &fds);
10 return FD_ISSET(0, &fds);
11 }
12 #endif
00000000000399d20 <sanity_test_fdelt()>:
1 399d20: 48 81 ec 98 00 00 00 sub $0x98,%rsp
2 399d27: b9 10 00 00 00 mov $0x10,%ecx
3 399d2c: 64 48 8b 04 25 28 00 mov %fs:0x28,%rax
4 399d33: 00 00
5 399d35: 48 89 84 24 88 00 00 mov %rax,0x88(%rsp)
6 399d3c: 00
7 399d3d: 31 c0 xor %eax,%eax
8 399d3f: 48 89 e7 mov %rsp,%rdi
9 399d42: fc cld
10 399d43: f3 48 ab rep stos %rax,%es:(%rdi)
11 399d46: 48 c7 c7 ff ff ff ff mov $0xffffffffffffffff,%rdi
12 399d4d: e8 3e ff ff ff callq 399c90 <__fdelt_warn>
13 399d52: 0f b6 04 24 movzbl (%rsp),%eax
14 399d56: 83 e0 01 and $0x1,%eax
15 399d59: 48 8b 94 24 88 00 00 mov 0x88(%rsp),%rdx
16 399d60: 00
17 399d61: 64 48 33 14 25 28 00 xor %fs:0x28,%rdx
18 399d68: 00 00
19 399d6a: 75 08 jne 399d74 <sanity_test_fdelt()+0x54>
20 399d6c: 48 81 c4 98 00 00 00 add $0x98,%rsp
21 399d73: c3 retq
22 399d74: e8 77 df c8 ff callq 27cf0 <__stack_chk_fail@plt>
23 399d79: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
0src/bitcoind
1*** buffer overflow detected ***: src/bitcoind terminated
2Aborted
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 as part of the output, but that needs more thought. I’ll address this in a follow up.
