scripts: Add MACHO stack canary check to security-check.py #18713
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I didn't add the relevant test in bitcoin#18295.
Contributor
|
utACK 8334ee3. |
Contributor
|
ACK 8334ee3: Mitigations are important. Important things are worth asserting :) Thanks for doing this! |
Empact
reviewed
Apr 21, 2020
| if p.returncode: | ||
| raise IOError('Error opening file') | ||
| ok = False | ||
| for line in stdout.splitlines(): |
Contributor
There was a problem hiding this comment.
Why splitlines? Seems you could test against stdout directly.
Member
Author
There was a problem hiding this comment.
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.
Contributor
Gitian builds
|
laanwj
added a commit
that referenced
this pull request
May 14, 2020
eacedfb scripts: add additional type annotations to security-check.py (fanquake) 83d063e scripts: add run_command to security-check.py (fanquake) 13f606b scripts: remove NONFATAL from security-check.py (fanquake) 061acf6 scripts: no-longer check for 32 bit windows in security-check.py (fanquake) Pull request description: * Remove 32-bit Windows checks. * Remove NONFATAL checking. Added in #8249, however unused since #13764. * Add `run_command` to de-duplicate all of the subprocess calls. Mentioned in #18713. * Add additional type annotations. * Print stderr when there is an issue running a command. ACKs for top commit: laanwj: ACK eacedfb Tree-SHA512: 69a7ccfdf346ee202b3e8f940634c5daed1d2b5a5d15ac9800252866ba3284ec66e391a66a0b341f5a4e5e8482fe1b614d4671e8e766112ff059405081184a85
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
7b99c74 uses
otool -Ivto check for___stack_chk_failin 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_guard8334ee3 is a follow up to #18295 and adds test cases to
test-security-check.pythat 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.