Skip to content

lockedpool: avoid sensitive data in core files (FreeBSD) #18443

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 1 commit into from
May 4, 2020
Merged

lockedpool: avoid sensitive data in core files (FreeBSD) #18443

laanwj merged 1 commit into from
May 4, 2020

Conversation

@vasild
Copy link
Contributor

@vasild vasild commented Mar 26, 2020

This is a followup to
23991ee / #15600
to also use madvise(2) on FreeBSD to avoid sensitive data allocated
with secure_allocator ending up in core files in addition to preventing
it from going to the swap.

@vasild
lockedpool: avoid sensitive data in core files (FreeBSD)
f852030 
This is a followup to
23991ee / bitcoin#15600
to also use madvise(2) on FreeBSD to avoid sensitive data allocated
with secure_allocator ending up in core files in addition to preventing
it from going to the swap.
luke-jr
luke-jr reviewed Mar 26, 2020
View reviewed changes
Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

Concept ACK

src/support/lockedpool.cpp
#ifdef MADV_DONTDUMP
#if defined(MADV_DONTDUMP) // Linux
madvise(addr, len, MADV_DONTDUMP);
#elif defined(MADV_NOCORE) // FreeBSD
Copy link
Member

@luke-jr luke-jr Mar 26, 2020

Choose a reason for hiding this comment

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

I think it would be preferable to use a new #ifdef here, just in case there's ever a system with both defined, but only one supported at runtime...

Copy link
Member

@sipa sipa Mar 26, 2020

Choose a reason for hiding this comment

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

As there are currently no such systems, and the flags do exactly the same thing, it seems unlikely that that would ever be an issue.

Copy link
Member

@laanwj laanwj Mar 27, 2020

Choose a reason for hiding this comment

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

even more if both defined this is "bizarro" territory and it might be better to fail the compile to reconsider what to do here, i don't think naively calling both in order (in what order?) is ever a good idea …
(but to be clear: leaving it like this is fine with me)

Copy link
Contributor Author

@vasild vasild Mar 27, 2020

Choose a reason for hiding this comment

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

I think if there is a system that defines MADV_DONTDUMP, but it is not supported at runtime (!?) then that system is badly broken. Regardless if it also defines MADV_NOCORE that works.

@sipa
Copy link
Member

sipa commented Mar 26, 2020

ACK f852030 if someone verifies this works as intended on *BSD.

@laanwj
Copy link
Member

laanwj commented Mar 27, 2020 •
edited
Loading

ACK f852030
I'd assume the MADV_ constants are guaranteed to be in the same header as mmap, sys/mman.h?

Edit: I've verified that the call to madvise is done on FreeBSD. I have not test whether it is effective.

@vasild
Copy link
Contributor Author

vasild commented Mar 27, 2020

I verified this works as intended on FreeBSD 12.

Yes, the MADV_ constants are in sys/mman.h:
https://www.freebsd.org/cgi/man.cgi?query=madvise
https://linux.die.net/man/2/madvise

@laanwj
Copy link
Member

laanwj commented Mar 27, 2020

Yes, the MADV_ constants are in sys/mman.h:

Ok good to know—if this was not the case, it would be better to have an autotools check instead.

@practicalswift
Copy link
Contributor

practicalswift commented Mar 29, 2020

Code-review ACK f852030 assuming a reviewer with FreeBSD access verifies that the PR goal is achieved :)

@laanwj laanwj merged commit b549cb1 into bitcoin:master May 4, 2020
@vasild vasild deleted the madvise branch May 4, 2020 14:32
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 4, 2020
@laanwj
Merge bitcoin#18443: lockedpool: avoid sensitive data in core files (…
5df8852 
…FreeBSD)

f852030 lockedpool: avoid sensitive data in core files (FreeBSD) (Vasil Dimov)

Pull request description:

  This is a followup to
  23991ee / bitcoin#15600
  to also use madvise(2) on FreeBSD to avoid sensitive data allocated
  with secure_allocator ending up in core files in addition to preventing
  it from going to the swap.

ACKs for top commit:
  sipa:
    ACK f852030 if someone verifies this works as intended on *BSD.
  laanwj:
    ACK f852030
  practicalswift:
    Code-review ACK f852030 assuming a reviewer with FreeBSD access verifies that the PR goal is achieved :)

Tree-SHA512: 2e6d4ab6a9fbe18732c8ba530eacc17f58128c97140758b80c905b5b838922a2bcaa5f9abc45ab69d5a1a2baa0cba322f006048b60a877228e089c7e64dadd2a
@str4d str4d mentioned this pull request Jul 31, 2020
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Sep 29, 2020
@zkbot
Auto merge of #4643 - str4d:locked-memory-manager, r=str4d
7d94064 
Locked memory manager

Add a pool for locked memory chunks, replacing `LockedPageManager`.

Cherry-picked from the following upstream PRs:
- bitcoin/bitcoin#8321
- bitcoin/bitcoin#8753
- bitcoin/bitcoin#9063
- bitcoin/bitcoin#9070
- bitcoin/bitcoin#11385
- bitcoin/bitcoin#12048
  - Excludes change to benchmark.
- bitcoin/bitcoin#15117
- bitcoin/bitcoin#16161
  - Excludes Travis CI changes.
  - Includes change from bitcoin/bitcoin#13163
- bitcoin/bitcoin#15600
- bitcoin/bitcoin#18443
- Assorted small changes from:
  - bitcoin/bitcoin#9233
  - bitcoin/bitcoin#10483
  - bitcoin/bitcoin#10645
  - bitcoin/bitcoin#10969
  - bitcoin/bitcoin#11351
- bitcoin/bitcoin#19111
  - Excludes change to `src/rpc/server.cpp`
- bitcoin/bitcoin#9804
  - Only the commit for `src/key.cpp`
- bitcoin/bitcoin#9598
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 12, 2021
@vasild @PiRK
lockedpool: avoid sensitive data in core files (FreeBSD)
72559fd 
Summary:
> This is a followup to [[bitcoin/bitcoin#15600 | PR15600]] to also use madvise(2) on FreeBSD
> to avoid sensitive data allocated with secure_allocator ending up
> in core files in addition to preventing it from going to the swap.

This is a backport of Core [[bitcoin/bitcoin#18443 | PR18443]]

Depends on D8879

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, deadalnix, Fabien

Reviewed By: #bitcoin_abc, deadalnix, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D8880
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#18443: lockedpool: avoid sensitive data in core files (…
425a7c8 
…FreeBSD)

f852030 lockedpool: avoid sensitive data in core files (FreeBSD) (Vasil Dimov)

Pull request description:

  This is a followup to
  23991ee / bitcoin#15600
  to also use madvise(2) on FreeBSD to avoid sensitive data allocated
  with secure_allocator ending up in core files in addition to preventing
  it from going to the swap.

ACKs for top commit:
  sipa:
    ACK f852030 if someone verifies this works as intended on *BSD.
  laanwj:
    ACK f852030
  practicalswift:
    Code-review ACK f852030 assuming a reviewer with FreeBSD access verifies that the PR goal is achieved :)

Tree-SHA512: 2e6d4ab6a9fbe18732c8ba530eacc17f58128c97140758b80c905b5b838922a2bcaa5f9abc45ab69d5a1a2baa0cba322f006048b60a877228e089c7e64dadd2a
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 15, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#18443: lockedpool: avoid sensitive data in core files (…
b566c46 
…FreeBSD)

f852030 lockedpool: avoid sensitive data in core files (FreeBSD) (Vasil Dimov)

Pull request description:

  This is a followup to
  23991ee / bitcoin#15600
  to also use madvise(2) on FreeBSD to avoid sensitive data allocated
  with secure_allocator ending up in core files in addition to preventing
  it from going to the swap.

ACKs for top commit:
  sipa:
    ACK f852030 if someone verifies this works as intended on *BSD.
  laanwj:
    ACK f852030
  practicalswift:
    Code-review ACK f852030 assuming a reviewer with FreeBSD access verifies that the PR goal is achieved :)

Tree-SHA512: 2e6d4ab6a9fbe18732c8ba530eacc17f58128c97140758b80c905b5b838922a2bcaa5f9abc45ab69d5a1a2baa0cba322f006048b60a877228e089c7e64dadd2a
@random-zebra random-zebra mentioned this pull request Dec 27, 2021
Merged
furszy added a commit to PIVX-Project/PIVX that referenced this pull request Jan 3, 2022
@furszy
Merge #2700: lockedpool: avoid sensitive data in core dumps
771c84b 
353346c lockedpool: avoid sensitive data in core files (FreeBSD) (Vasil Dimov)
18ca764 lockedpool: When possible, use madvise to avoid including sensitive information in core dumps (Luke Dashjr)

Pull request description:

  Backports of bitcoin#15600 and bitcoin#18443 patching [CVE-2019-15947](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-15947).

ACKs for top commit:
  Fuzzbawls:
    utACK 353346c
  furszy:
    utACK 353346c and merging..

Tree-SHA512: 4e5108803be2881b2b25ed135ce498c04d0ee9b5444d506790f323a32d556497539e6bb139840eea101e33013475e0d2002d5fc4a0bbf03e3e5e388eeb7564c3
@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

@sipa sipa sipa left review comments

+1 more reviewer

@luke-jr luke-jr luke-jr left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Utils/log/libs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vasild @sipa @laanwj @practicalswift @luke-jr