contrib: Parse ELF directly for symbol and security checks #20434
Conversation
|
Concept ACK 🧚 |
18a1c5a to
8c3b297
Compare
|
Concept ACK |
|
I've ported the changes for PPC64 from #14066 in an extra commit. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
e6441c6 to
e656dbf
Compare
|
Added mypy annotations. This found a few (small) actual bugs, and might make review easier by clarifying expected types. |
Instead of the ever-messier text parsing of the output of the readelf tool (which is clearly meant for human consumption not to be machine parseable), parse the ELF binaries directly. Add a small dependency-less ELF parser specific to the checks. This is slightly more secure, too, because it removes potential ambiguity due to misparsing and changes in the output format of `elfread`. It also allows for stricter and more specific ELF format checks in the future. This removes the build-time dependency for `readelf`. It passes the test-security-check for me locally, though I haven't checked on all platforms.
Changes from bitcoin#14066.
3979251 to
a0a7718
Compare
|
Concept ACK |
|
Concept ACK. ELF looks remarkably simple! |
The secret is not needing to resolve relocations 🙂 That is where most of the verbose platform specific trickery comes in. Edit: not only don't we have any security checks that involve relocations and relocated code, so it's unnecessary to parse them, all the bitcoin binaries are compiled as position independent code so there shouldn't be that many at all. I still see some relocations for dynamic linking machinery like |
|
Concept ACK: removing dependencies improves robustness |
|
It's not really the dependency that is the problem here. Sure, needing to call out to an utility less is nice, but |
|
Concept ACK |
|
Would be nice to get #20476 in first, which adds a test that the symbol check can actually fail. |
Gitian builds
|
|
Good to see that both guix and gitian builds (well, the checks) passed for all platforms. |
|
Closing and reopening to re-trigger Travis with #20476 in. |
|
cirrus won't re-trigger on open-close, but on a push only (e.g. rebase or other any other git push) |
|
sigh okay, added an empty commit to re-trgger CI, please remind me to remove it before merge. |
|
Just wanted to note that this will fail the /usr/local/opt/binutils/bin/readelf -dw ~/Downloads/bitcoin-0.16.3/bin/bitcoind | rg -i bind
...
0x0000000000000009 (RELAENT) 24 (bytes)
0x0000000000000018 (BIND_NOW)
0x000000006ffffffb (FLAGS_1) Flags: NOWrather than the /usr/local/opt/binutils/bin/readelf -dw ~/Downloads/bitcoin-0.17.2/bin/bitcoind | rg -i bind
...
0x0000000000000009 (RELAENT) 24 (bytes)
0x000000000000001e (FLAGS) BIND_NOW
0x000000006ffffffb (FLAGS_1) Flags: NOW PIEand our current script checks for either. Not that it's a blocker, just noticed while testing. |
|
Thanks for testing! In contrast to passing, failing is safe in this case. I think it's fine to support the 'new way' only, as this is what we want our binaries to use.
|
|
Concept & approach ACK. I have tried the pixie module on a few binaries and seems to give reasonable results, but haven't reviewed the code in detail or verified against any ELF format reference. |
|
@sipa Thanks for testing and looking at it! FWIW, verifying it against |
|
I'm going to merge this so that #14066 can be rebased on top and we can introduce the POWER architecture binaries in the beginning of the 0.22 window instead of the last minute (or again miss it last minute). |
…ity checks a0a7718 contrib: Changes to checks for PowerPC64 (Luke Dashjr) 634f6ec contrib: Parse ELF directly for symbol and security checks (Wladimir J. van der Laan) Pull request description: Instead of the ever-messier text parsing of the output of the readelf tool (which is clearly meant for human consumption not to be machine parseable), parse the ELF binaries directly. Add a small dependency-less ELF parser specific to the checks. This is slightly more secure, too, because it removes potential ambiguity due to misparsing and changes in the output format of `elfread`. It also allows for stricter and more specific ELF format checks in the future. This removes the build-time dependency for `readelf`. It passes the test-security-check for me locally, ~~though I haven't checked on all platforms~~. I've checked that this works on the cross-compile output for all ELF platforms supported by Bitcoin Core at the moment, as well as PPC64 LE and BE. Top commit has no ACKs. Tree-SHA512: 7f9241fec83ee512642fecf5afd90546964561efd8c8c0f99826dcf6660604a4db2b7255e1afb1e9bb0211fd06f5dbad18a6175dfc03e39761a40025118e7bfc
1ef2138 lint: run mypy over contrib/devtools (fanquake) Pull request description: wumpus mentioned on IRC that we don't currently run `mypy` over the `contrib/devtools` directory, and that it would likely be worthwhile given #20434. This just adds that dir to the linter, as well as some missing annotations to fix existing errors. Note that now we require Python 3.6 we can make use of variable annotations. master (patched to check contrib devtools): ```bash test/lint/lint-python.sh contrib/devtools/symbol-check.py:154: error: Incompatible types in assignment (expression has type "List[str]", variable has type "str") contrib/devtools/circular-dependencies.py:35: error: Need type annotation for 'deps' (hint: "deps: Dict[<type>, <type>] = ...") contrib/devtools/circular-dependencies.py:67: error: Need type annotation for 'closure' (hint: "closure: Dict[<type>, <type>] = ...") Found 4 errors in 3 files (checked 187 source files) ``` I haven't quite gone as far as to add annotations like ```python CHECKS: Dict[str, List[Tuple[str, Callable[[Any], bool]]]] = {... ``` to `symbol-check.py`. ACKs for top commit: laanwj: ACK 1ef2138 Tree-SHA512: a58c2ece588c640289dc1d35dad5b1b8732788272daa0965d6bf44ee8a7f7c8e8585f94d233ac41c84b9ffcfc97841a00fe2c9acba41f58fd164f01de4b6512b
1ef2138 lint: run mypy over contrib/devtools (fanquake) Pull request description: wumpus mentioned on IRC that we don't currently run `mypy` over the `contrib/devtools` directory, and that it would likely be worthwhile given bitcoin#20434. This just adds that dir to the linter, as well as some missing annotations to fix existing errors. Note that now we require Python 3.6 we can make use of variable annotations. master (patched to check contrib devtools): ```bash test/lint/lint-python.sh contrib/devtools/symbol-check.py:154: error: Incompatible types in assignment (expression has type "List[str]", variable has type "str") contrib/devtools/circular-dependencies.py:35: error: Need type annotation for 'deps' (hint: "deps: Dict[<type>, <type>] = ...") contrib/devtools/circular-dependencies.py:67: error: Need type annotation for 'closure' (hint: "closure: Dict[<type>, <type>] = ...") Found 4 errors in 3 files (checked 187 source files) ``` I haven't quite gone as far as to add annotations like ```python CHECKS: Dict[str, List[Tuple[str, Callable[[Any], bool]]]] = {... ``` to `symbol-check.py`. ACKs for top commit: laanwj: ACK 1ef2138 Tree-SHA512: a58c2ece588c640289dc1d35dad5b1b8732788272daa0965d6bf44ee8a7f7c8e8585f94d233ac41c84b9ffcfc97841a00fe2c9acba41f58fd164f01de4b6512b
543bf74 gitian-linux: Extend noexec-stack workaround to powerpc (Wladimir J. van der Laan) 00f67c8 gitian-linux: Build binaries for 64-bit POWER (Luke Dashjr) 63fc2b1 gitian: Properly quote arguments in wrappers (Luke Dashjr) 798bc0b Support glibc-back-compat on 64-bit POWER (Luke Dashjr) Pull request description: Rebase of #14066 by luke-jr. Let's try to get PowerPC support in in the beginning of the 22.0 cycle so that it gets some testing, and is not a last-minute decision this time, like for last … 2 or 3 major versions. The symbol/security tooling-related changes have been dropped since they were part of #20434. Top commit has no ACKs. Tree-SHA512: df0f8cd320c90f359f8b512c5cb8b59bb277516b57a05482cc8923c656106513b7428e315aaa8ab53e0bd6f80556b07d3639c47f6d9913bcfbfe388b39ef47c4
Instead of the ever-messier text parsing of the output of the readelf tool (which is clearly meant for human consumption not to be machine parseable), parse the ELF binaries directly.
Add a small dependency-less ELF parser specific to the checks.
This is slightly more secure, too, because it removes potential ambiguity due to misparsing and changes in the output format of
elfread. It also allows for stricter and more specific ELF format checks in the future.This removes the build-time dependency for
readelf.It passes the test-security-check for me locally,
though I haven't checked on all platforms. I've checked that this works on the cross-compile output for all ELF platforms supported by Bitcoin Core at the moment, as well as PPC64 LE and BE.