scripts: Add MACHO stack canary check to security-check.py #18713

pull fanquake wants to merge 2 commits into bitcoin:master from fanquake:check_MACHO_canary changing 2 files +26 −7

fanquake commented at 4:45 AM on April 21, 2020: member
7b99c7454cdb74cd9cd7a5eedc2fb9d0a19df456 uses otool -Iv to check for ___stack_chk_fail in the macOS binaries. Similar to the ELF check. Note that looking for a triple underscore prefixed function (as opposed to two for ELF) is correct for the macOS binaries. i.e:
```
otool -Iv bitcoind | grep chk
0x00000001006715b8   509 ___memcpy_chk
0x00000001006715be   510 ___snprintf_chk
0x00000001006715c4   511 ___sprintf_chk
0x00000001006715ca   512 ___stack_chk_fail
0x00000001006715d6   517 ___vsnprintf_chk
0x0000000100787898   513 ___stack_chk_guard
```
8334ee31f868f0f9baf0920d14d20174ed889dbe is a follow up to #18295 and adds test cases to test-security-check.py that for some reason I didn't add at the time. I'll sort out #18434 so that we can run these tests in the CI.
scripts: add MACHO Canary check to security-check.py 7b99c7454c
scripts: add MACHO LAZY_BINDINGS test to test-security-check.py
I didn't add the relevant test in #18295.
8334ee31f8
fanquake added the label Scripts and tools on Apr 21, 2020
fanquake added the label Needs gitian build on Apr 21, 2020
jonasschnelli commented at 8:08 AM on April 21, 2020: contributor

utACK 8334ee31f868f0f9baf0920d14d20174ed889dbe. note-nit: the subprocess.Popen with manual line per line "grep-ing" could probably be refactored (not in this PR). Or use shell grep.
practicalswift commented at 2:59 PM on April 21, 2020: contributor

ACK 8334ee31f868f0f9baf0920d14d20174ed889dbe: Mitigations are important. Important things are worth asserting :)

Thanks for doing this!
in contrib/devtools/security-check.py:235 in 8334ee31f8
```
 230 | +    p = subprocess.Popen([OTOOL_CMD, '-Iv', executable], stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE, universal_newlines=True)
 231 | +    (stdout, stderr) = p.communicate()
 232 | +    if p.returncode:
 233 | +        raise IOError('Error opening file')
 234 | +    ok = False
 235 | +    for line in stdout.splitlines():
```
Empact commented at 5:31 PM on April 21, 2020:

Why splitlines? Seems you could test against stdout directly.

fanquake commented at 6:51 AM on April 22, 2020:

To mirror the other tests. As Jonas mentioned, we can probably refactor these checks to remove some duplicated code, so I can look at changing these (where it makes sense) as well.

DrahtBot commented at 8:24 PM on April 21, 2020: member

Gitian builds

File	commit c4c3f110eb93243fc8f740070240f50b0008f206<br>(master)	commit 3121daaa89cd73c8ee011721a4e575e3fc46548a<br>(master and this pull)
bitcoin-0.20.99-aarch64-linux-gnu-debug.tar.gz	`20cf5b8b49af8b5c...`	`cbb05724b3c7f2ba...`
bitcoin-0.20.99-aarch64-linux-gnu.tar.gz	`b48063c771a930f3...`	`6a6091c701741e29...`
bitcoin-0.20.99-arm-linux-gnueabihf-debug.tar.gz	`05ac72341a09394b...`	`a92dbea924618425...`
bitcoin-0.20.99-arm-linux-gnueabihf.tar.gz	`99f7d8562c5a11d4...`	`110e667504a23de0...`
bitcoin-0.20.99-osx-unsigned.dmg	`e51b038384a301d9...`	`e6d168e3d4fb160b...`
bitcoin-0.20.99-osx64.tar.gz	`e298696bfb0ec235...`	`2f6402c5e7dd72c2...`
bitcoin-0.20.99-riscv64-linux-gnu-debug.tar.gz	`08fb295e433150b4...`	`e7fd293b184ba00b...`
bitcoin-0.20.99-riscv64-linux-gnu.tar.gz	`719a37899dc38aec...`	`70c86da5f5c8d9ad...`
bitcoin-0.20.99-win64-debug.zip	`e28f048e991270c9...`	`5802a988f42be41c...`
bitcoin-0.20.99-win64-setup-unsigned.exe	`06210bcff7c38230...`	`da404edd2e0ebd27...`
bitcoin-0.20.99-win64.zip	`5533c39402aeffd7...`	`b0aaf54555ab4814...`
bitcoin-0.20.99-x86_64-linux-gnu-debug.tar.gz	`f96c6dbe96e7b94d...`	`3d236162c84a9e4f...`
bitcoin-0.20.99-x86_64-linux-gnu.tar.gz	`6c2a8db2c7d89bc9...`	`961a8c479e112e4d...`
bitcoin-0.20.99.tar.gz	`898009f5d6825797...`	`9ca909e5e23d9e21...`
bitcoin-core-linux-0.21-res.yml	`f72f6d19b77b8a5f...`	`893b2b44ca2d48bc...`
bitcoin-core-osx-0.21-res.yml	`1a4e78200ae822a6...`	`beb75651b8df1833...`
bitcoin-core-win-0.21-res.yml	`4a6c3a64a5e7f7db...`	`0b54ab0ce86327ab...`
linux-build.log	`41c3e184d5c072ee...`	`eaccfe1e8802d33d...`
osx-build.log	`a2d9610ccc240be4...`	`aa856a1726a5988c...`
win-build.log	`13a83af36bb2074c...`	`30d7f01c046e1534...`
bitcoin-core-linux-0.21-res.yml.diff		`f492d279e968a6cf...`
bitcoin-core-osx-0.21-res.yml.diff		`b0457415d7039d53...`
bitcoin-core-win-0.21-res.yml.diff		`d6c19564d257d03b...`
linux-build.log.diff		`2f9195dc21eeabd7...`
osx-build.log.diff		`530e94449a57258d...`
win-build.log.diff		`7bfc531d669392f8...`

DrahtBot removed the label Needs gitian build on Apr 21, 2020
fanquake merged this on Apr 22, 2020
fanquake closed this on Apr 22, 2020
fanquake deleted the branch on Apr 22, 2020
laanwj referenced this in commit 2d7489be8f on May 14, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#18295 scripts: add MACHO lazy bindings check to security-check.py #18434 tests: add a test-security target and run it in CI #18796 scripts: security-check.py refactors