Skip to content
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

security-check: test for _FORTIFY_SOURCE usage in release binaries #27038

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

security-check: test for _FORTIFY_SOURCE usage in release binaries #27038

wants to merge 1 commit into from
+45 −24

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Feb 3, 2023 •
edited

Test for the existence of fortified functions in the ELF release binaries. Related to #27027.
Can't be done yet because we don't end up with any fortified funcs in bitcoin-util.

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 3, 2023 •
edited

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK hebasto, laanwj

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #29987 (guix: build with glibc 2.31 by fanquake)
  • #24123 (guix: Pointer Authentication and Branch Target Identification for aarch64 Linux (Guix) by fanquake)

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.

@fanquake fanquake mentioned this pull request Feb 5, 2023
Merged
@fanquake
Copy link
Member Author

we don't end up with any foritfied funcs in bitcoin-util or bitcoin-cli.

bitcoin-cli is fixed once we fortify libevent.

@fanquake fanquake mentioned this pull request Feb 17, 2023
Merged
@fanquake
Copy link
Member Author

bitcoin-cli is fixed once we fortify libevent.

Rebased on #27118

@DrahtBot DrahtBot mentioned this pull request Feb 17, 2023
Draft
fanquake added a commit that referenced this pull request Feb 28, 2023
@fanquake
Merge #27118: depends: harden libevent
e60a58f 
ff4a73a depends: use FORTIFY_SOURCE=3 with libevent (fanquake)

Pull request description:

  Use `FORTIFY_SOURCE=3` when building libevent in depends. I've upstreamed a change to switch libevent from using =2 to =3 as well: libevent/libevent#1418.

  Solves half of #27038, by giving us some fortified funcs in `bitcoin-cli`.

ACKs for top commit:
  TheCharlatan:
    ACK ff4a73a

Tree-SHA512: eaf692ec92b288f0cb524c011fc81529f58efa4c43d418a7b3ae7108eba2bccba708a81a28ac6d063267be80ca615637c6e3fccc02497d7367af2eaae0e8d812
@fanquake
Copy link
Member Author

Rebased post #27118.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 28, 2023
@fanquake
Merge bitcoin#27118: depends: harden libevent
5f087db 
ff4a73a depends: use FORTIFY_SOURCE=3 with libevent (fanquake)

Pull request description:

  Use `FORTIFY_SOURCE=3` when building libevent in depends. I've upstreamed a change to switch libevent from using =2 to =3 as well: libevent/libevent#1418.

  Solves half of bitcoin#27038, by giving us some fortified funcs in `bitcoin-cli`.

ACKs for top commit:
  TheCharlatan:
    ACK ff4a73a

Tree-SHA512: eaf692ec92b288f0cb524c011fc81529f58efa4c43d418a7b3ae7108eba2bccba708a81a28ac6d063267be80ca615637c6e3fccc02497d7367af2eaae0e8d812
@hebasto
Copy link
Member

hebasto commented Oct 25, 2023

Concept ACK.

@hebasto
Copy link
Member

hebasto commented Oct 25, 2023

Can't be done yet because we don't end up with any fortified funcs in bitcoin-util.

Does it make sense to provide a list of expected symbols for every binary been tested? It would be an empty one for bitcoin-util.

@fanquake
Copy link
Member Author

Does it make sense to provide a list of expected symbols for every binary been tested?

I don't think so, and that would likely require constant maintenance (plus guix builds to be run on every code change).

@laanwj
Copy link
Member

laanwj commented Apr 24, 2024

Concept ACK

laanwj
laanwj reviewed Apr 24, 2024
View reviewed changes
contrib/devtools/security-check.py
chk_funcs = set()

for sym in binary.symbols:
match = re.search(r'__[a-z]*_chk', sym.name)
Copy link
Member

Choose a reason for hiding this comment

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

Might want to check .imported to make sure it's an imported symbol, just to be sure.

@laanwj
Copy link
Member

laanwj commented Apr 24, 2024

i'd be okay with skipping the check for bitcoin-util: it's the least relevant binary for fortification (no network access, not even file format access). Could reconsider it later if it actually gains some useful functionality 😄

@DrahtBot DrahtBot mentioned this pull request Apr 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laanwj laanwj laanwj left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@fanquake @DrahtBot @hebasto @laanwj