Skip to content

tests: add a test-security target and run it in CI #18434

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
laanwj merged 2 commits into from
Jun 16, 2020
Merged

tests: add a test-security target and run it in CI #18434

laanwj merged 2 commits into from
Jun 16, 2020

Conversation

@fanquake
Copy link
Member

@fanquake fanquake commented Mar 26, 2020 •
edited
Loading

Wladimir asked about running the test-security-check.py script in our CI. This PR adds a target for that: make test-security and adds it to a few CI jobs.

@fanquake fanquake force-pushed the run_security_check_ci branch 2 times, most recently from ba89275 to 210bbfd Compare March 26, 2020 04:32
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 26, 2020 •
edited
Loading

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

Conflicts

Reviewers, 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.

This was referenced Mar 26, 2020
Merged
Draft
maflcko
maflcko reviewed Mar 26, 2020
View reviewed changes
@practicalswift
Copy link
Contributor

practicalswift commented Mar 26, 2020

Concept ACK: enforcement via automation is good

@maflcko
Copy link
Member

maflcko commented Mar 26, 2020

The same is already run via gitian, right?

@DrahtBot DrahtBot mentioned this pull request Apr 17, 2020
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 17, 2020

🐙 This pull request conflicts with the target branch and needs rebase.

@fanquake fanquake mentioned this pull request Apr 21, 2020
Merged
fanquake added a commit that referenced this pull request Apr 22, 2020
@fanquake
Merge #18713: scripts: Add MACHO stack canary check to security-check.py
c90a9e6 
8334ee3 scripts: add MACHO LAZY_BINDINGS test to test-security-check.py (fanquake)
7b99c74 scripts: add MACHO Canary check to security-check.py (fanquake)

Pull request description:

  7b99c74 uses `otool -Iv` to check for `___stack_chk_fail` in the macOS binaries. Similar to the [ELF check](https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/security-check.py#L105). Note that looking for a triple underscore prefixed function (as opposed to two for ELF) is correct for the macOS binaries. i.e:
  ```bash
  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
  ```

  8334ee3 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.

ACKs for top commit:
  practicalswift:
    ACK 8334ee3: Mitigations are important. Important things are worth asserting :)
  jonasschnelli:
    utACK 8334ee3.

Tree-SHA512: 1aa5ded34bbd187eddb112b27278deb328bfc21ac82316b20fab6ad894f223b239a76b53dab0ac1770d194c1760fcc40d4da91ec09959ba4fc8eadedb173936a
@laanwj
Copy link
Member

laanwj commented May 14, 2020

Concept ACK

maflcko
maflcko reviewed May 14, 2020
View reviewed changes
@fanquake fanquake force-pushed the run_security_check_ci branch from 210bbfd to fa07027 Compare May 15, 2020 03:12
@fanquake fanquake force-pushed the run_security_check_ci branch from fa07027 to 8a05476 Compare May 15, 2020 03:17
@fanquake
Copy link
Member Author

fanquake commented May 15, 2020

Have rebased and should have fixed up all the issues here.

The same is already run via gitian, right?

We run the symbol and security checks in gitian, but not the security-check test.

@maflcko
Copy link
Member

maflcko commented May 15, 2020

Concept ACK (have not reviewed the code beside a cursory glance on the ci config files)

@DrahtBot DrahtBot mentioned this pull request May 19, 2020
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented May 22, 2020

🐙 This pull request conflicts with the target branch and needs rebase.

@fanquake fanquake force-pushed the run_security_check_ci branch from 8a05476 to 11e09a4 Compare May 22, 2020 12:19
@fanquake fanquake force-pushed the run_security_check_ci branch from 11e09a4 to cb3be57 Compare May 22, 2020 13:17
@fanquake fanquake requested a review from laanwj May 23, 2020 01:51
@DrahtBot DrahtBot mentioned this pull request May 27, 2020
Closed
@fjahr
Copy link
Contributor

fjahr commented Jun 12, 2020

Concept ACK

Also looked at the code and it looks good to me, but I don't feel qualified enough in this area to give a full ACK at the moment.

@practicalswift
Copy link
Contributor

practicalswift commented Jun 12, 2020

ACK cb3be57a83aa2065061e6cb77cec16167f133d3c -- patch looks correct and Travis is happy

laanwj
laanwj reviewed Jun 15, 2020
View reviewed changes
Makefile.am Outdated
Copy link
Member

@laanwj laanwj Jun 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe test-security-check target instead of test-security? I mean, it's a test for the binary security check. It doesn't check the security of the actual binaries.

Copy link
Member Author

@fanquake fanquake Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I've updated the target to test-security-check.

@fanquake fanquake force-pushed the run_security_check_ci branch from cb3be57 to 9fe71a5 Compare June 16, 2020 11:58
@laanwj
Copy link
Member

laanwj commented Jun 16, 2020

ACK 9fe71a5

@laanwj laanwj merged commit f658c15 into bitcoin:master Jun 16, 2020
@fanquake fanquake deleted the run_security_check_ci branch June 17, 2020 01:15
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj left review comments

+1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Build system Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@fanquake @DrahtBot @practicalswift @maflcko @laanwj @fjahr