contrib: Parse ELF directly for symbol and security checks #20434

pull laanwj wants to merge 2 commits into bitcoin:master from laanwj:2020_11_pixie changing 6 files +419 −169
  1. laanwj commented at 12:03 pm on November 20, 2020: member

    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.

  2. laanwj added the label Build system on Nov 20, 2020
  3. laanwj added the label Utils/log/libs on Nov 20, 2020
  4. fanquake commented at 12:24 pm on November 20, 2020: member
    Concept ACK 🧚
  5. laanwj force-pushed on Nov 20, 2020
  6. laanwj force-pushed on Nov 20, 2020
  7. laanwj force-pushed on Nov 20, 2020
  8. jonatack commented at 1:56 pm on November 20, 2020: member
    Concept ACK
  9. MarcoFalke added the label Needs gitian build on Nov 20, 2020
  10. MarcoFalke added the label Needs Guix build on Nov 20, 2020
  11. laanwj commented at 3:11 pm on November 20, 2020: member
    I’ve ported the changes for PPC64 from #14066 in an extra commit.
  12. in contrib/devtools/symbol-check.py:146 in e6441c6dec outdated
    144@@ -142,26 +145,15 @@ def close(self):
    145 
    146 def read_symbols(executable, imports=True) -> List[Tuple[str, str, str]]:
    


    laanwj commented at 4:04 pm on November 20, 2020:

    I don’t know much about python type specifications but this is wrong now, I think it is Tuple[List[Tuple[str, str]], int].

    Edit: resolved now

  13. DrahtBot commented at 5:05 pm on November 20, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #20451 (lint: run mypy over contrib/devtools by fanquake)
    • #14066 (gitian-linux: Build binaries for 64-bit POWER by luke-jr)

    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.

  14. laanwj force-pushed on Nov 21, 2020
  15. laanwj commented at 12:52 pm on November 21, 2020: member
    Added mypy annotations. This found a few (small) actual bugs, and might make review easier by clarifying expected types.
  16. contrib: Parse ELF directly for symbol and security checks
    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.
    634f6ec4eb
  17. contrib: Changes to checks for PowerPC64
    Changes from #14066.
    a0a771843f
  18. laanwj force-pushed on Nov 22, 2020
  19. theStack commented at 10:29 am on November 22, 2020: member
    Concept ACK
  20. sipa commented at 4:06 am on November 23, 2020: member

    Concept ACK.

    ELF looks remarkably simple!

  21. in contrib/devtools/pixie.py:13 in a0a771843f
     8+import struct
     9+import types
    10+from typing import Dict, List, Optional, Union, Tuple
    11+
    12+# you can find all these values in elf.h
    13+EI_NIDENT = 16
    


    fanquake commented at 4:08 am on November 23, 2020:

    note: Now that we require Python 3.6, if you want you can also use variable annotations:

    0EI_NIDENT: int = 16
    

    laanwj commented at 8:12 am on November 23, 2020:
    Sure but, I added any annotations that mypy required of me even with various --disallow-* options. I’m not convinced that decorating unambiguous assignments of integers with : int adds anything but verbosity.
  22. laanwj commented at 8:13 am on November 23, 2020: member

    ELF looks remarkably simple!

    The secret is not needing to resolve relocations :slightly_smiling_face: 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 .plt and .got sections.

  23. practicalswift commented at 10:28 am on November 23, 2020: contributor
    Concept ACK: removing dependencies improves robustness
  24. laanwj commented at 11:22 am on November 23, 2020: member
    It’s not really the dependency that is the problem here. Sure, needing to call out to an utility less is nice, but readelf is part of binutils that we require for build. If it had a stable, predictable output format for machine parsing (like git’s plumbing/porcelain distinction), this wouldn’t be needed. The problem is basing security checks on an ambiguous human-friendly output format.
  25. luke-jr commented at 0:15 am on November 25, 2020: member
    Concept ACK
  26. laanwj commented at 5:33 pm on November 26, 2020: member
    Would be nice to get #20476 in first, which adds a test that the symbol check can actually fail.
  27. DrahtBot commented at 11:02 pm on November 27, 2020: member

    Guix builds

    File commit afdfd3c8c1ce96adae11809e3989de381137fee9(master) commit 2cc79521b7af3cce8118b2a04fbe257764c44708(master and this pull)
    *-aarch64-linux-gnu-debug.tar.gz 321e361023e8ea7b... 813144f84559e0dc...
    *-aarch64-linux-gnu.tar.gz b054d1129516836e... 184917064f174e78...
    *-arm-linux-gnueabihf-debug.tar.gz cdd6185a356e7a80... 883c572d0fe1a7db...
    *-arm-linux-gnueabihf.tar.gz 4db9b1126b6601b7... 8f212d72c0159656...
    *-riscv64-linux-gnu-debug.tar.gz df4a6eb0267f7140... 12e5d6bdec38909f...
    *-riscv64-linux-gnu.tar.gz 3ff6d83bd5983e2b... e252e5ded636a464...
    *-win-unsigned.tar.gz 0dbc83ed5f22a4db... 68b64bab11c74876...
    *-win64-debug.zip 539b210aa13b56bc... 9f1868590f5c2034...
    *-win64-setup-unsigned.exe 5ff779deff3db721... c3d72b942f3ed28a...
    *-win64.zip 4e50c84031e8b116... c78d5b2dce8fab09...
    *-x86_64-linux-gnu-debug.tar.gz 345daa571d3557b0... 32a8cd9a90d81191...
    *-x86_64-linux-gnu.tar.gz 60655c6fb26e8633... 8a90973bb90f451b...
    *.tar.gz 0ddcf17cb856ed92... d150e18f32cf35ab...
    guix_build.log 6670400b0a6511d0... 1b9c8c06b0d5ed02...
    guix_build.log.diff 70d1bc211652bbf4...
  28. DrahtBot removed the label Needs Guix build on Nov 27, 2020
  29. DrahtBot commented at 5:32 am on December 1, 2020: member

    Gitian builds

    File commit e2ff5e7b35d71195278d2a2ed9485f141de33d7a(master) commit 1c4188e0337fc89fa7a72d99d16c7508f97f5497(master and this pull)
    bitcoin-core-linux-22-res.yml 44e52af36236879f... ed4680968e93faa5...
    bitcoin-core-osx-22-res.yml 71d5fb0e6642d6b0... 127fc6fd6c335c03...
    bitcoin-core-win-22-res.yml 0148a2f5e7295184... a56ae5d4153d1349...
    *-aarch64-linux-gnu-debug.tar.gz 383e84aaa96f342a... 26fbc2d21bada83d...
    *-aarch64-linux-gnu.tar.gz 3558211891c0484a... a35fd2ae3605c54f...
    *-arm-linux-gnueabihf-debug.tar.gz 5ebbb475a590480a... 52568603abb0f82d...
    *-arm-linux-gnueabihf.tar.gz 60951d893e2688b6... b7c3b23abd9e83d9...
    *-osx-unsigned.dmg 7cba6edda598c96c... 56e862ccf0d2b4eb...
    *-osx64.tar.gz ea2ab5c893780a2e... ba06f4bfd21a3d07...
    *-riscv64-linux-gnu-debug.tar.gz 0c20a43923efcd5f... 500c8c3ec67bcaad...
    *-riscv64-linux-gnu.tar.gz 8d3632d81c77eff1... ba164e9b54dbb098...
    *-win64-debug.zip 72c35e133cf9a0db... c0afdc8cd087028a...
    *-win64-setup-unsigned.exe 55c17bfff54273e5... 3716ec12171f7369...
    *-win64.zip f7bf6737c7f741c3... 19e38e38b95c53b1...
    *-x86_64-linux-gnu-debug.tar.gz abf91baef5c44686... 03794e9328c8b547...
    *-x86_64-linux-gnu.tar.gz 75fda490174df335... ab84f605ba4d0716...
    *.tar.gz 793a2c7af39452d5... e07ea56a79dd81a3...
    linux-build.log 9c5461a914e1f4c0... 1fbb119f2f0ef4ac...
    osx-build.log 5adaf67afbaf00fc... 85dd7dace784a1a6...
    win-build.log 756101ecccffabf0... 062b62cf4af16bc9...
    bitcoin-core-linux-22-res.yml.diff 2edae2388d844498...
    bitcoin-core-osx-22-res.yml.diff f0b350f311f376f3...
    bitcoin-core-win-22-res.yml.diff cb6211ee06a2ddc9...
    linux-build.log.diff 7b880e3e3f0990cf...
    osx-build.log.diff 291601d4a5cea40e...
    win-build.log.diff e7b782648941c2c2...
  30. DrahtBot removed the label Needs gitian build on Dec 1, 2020
  31. laanwj commented at 11:38 am on December 3, 2020: member
    Good to see that both guix and gitian builds (well, the checks) passed for all platforms.
  32. laanwj commented at 10:07 am on December 7, 2020: member
    Closing and reopening to re-trigger Travis with #20476 in.
  33. laanwj closed this on Dec 7, 2020

  34. laanwj reopened this on Dec 7, 2020

  35. MarcoFalke commented at 10:36 am on December 7, 2020: member
    cirrus won’t re-trigger on open-close, but on a push only (e.g. rebase or other any other git push)
  36. laanwj force-pushed on Dec 7, 2020
  37. laanwj commented at 8:07 pm on December 7, 2020: member
    sigh okay, added an empty commit to re-trgger CI, please remind me to remove it before merge. Edit: it passed, empty commit removed again.
  38. laanwj force-pushed on Dec 8, 2020
  39. fanquake commented at 8:47 am on December 14, 2020: member

    Just wanted to note that this will fail the RELRO check if run against binaries older than 0.17.x, because they have the deprecated DT_BIND_NOW array tag:

    0/usr/local/opt/binutils/bin/readelf -dw ~/Downloads/bitcoin-0.16.3/bin/bitcoind | rg -i bind
    1...
    2 0x0000000000000009 (RELAENT)            24 (bytes)
    3 0x0000000000000018 (BIND_NOW)           
    4 0x000000006ffffffb (FLAGS_1)            Flags: NOW
    

    rather than the DF_BIND_NOW flag:

    0/usr/local/opt/binutils/bin/readelf -dw ~/Downloads/bitcoin-0.17.2/bin/bitcoind | rg -i bind
    1...
    2 0x0000000000000009 (RELAENT)            24 (bytes)
    3 0x000000000000001e (FLAGS)              BIND_NOW
    4 0x000000006ffffffb (FLAGS_1)            Flags: NOW PIE
    

    and our current script checks for either. Not that it’s a blocker, just noticed while testing.

  40. laanwj commented at 3:49 pm on December 14, 2020: member

    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.

    Edit: or should we check for both to be present because old platforms might ignore the new flag? No, because new toolchains don’t set the old flag.

  41. sipa commented at 9:01 pm on December 15, 2020: member
    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.
  42. laanwj commented at 12:19 pm on December 17, 2020: member

    @sipa Thanks for testing and looking at it!

    FWIW, verifying it against elf.h is probably easier than verifying it against the ELF spec as it follows a similar structure. But I hope the security and symbol-check tests are enough to give confidence that this has the same functionality as the elfread based implementation. (apart from the deprecated DT_BIND_NOW check)

  43. laanwj commented at 11:16 am on December 18, 2020: member
    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).
  44. laanwj merged this on Dec 18, 2020
  45. laanwj closed this on Dec 18, 2020

  46. sidhujag referenced this in commit 9700b9a77e on Dec 18, 2020
  47. laanwj referenced this in commit 7f9ae87011 on Dec 28, 2020
  48. sidhujag referenced this in commit 93ce1b2cd9 on Dec 28, 2020
  49. laanwj referenced this in commit 6a726cb534 on Jan 28, 2021
  50. DrahtBot locked this on Feb 15, 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-11-17 06:12 UTC

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