Efficiently allocate memory for secrets in locked pages.

[afck]

~The current mlock functionality—locking each memory page that happens to contain a secret key—causes problems with tests (and possibly also in production) because it quickly reaches the limit of locked memory pages.~

We need a special allocator that mlocks, zeroes and munlocks dedicated memory pages and allocates space for secrets in there: locked pages are a scarce resource and should be used exclusively for secrets.

See the discussion on https://github.com/poanetwork/threshold_crypto/pull/34, specifically https://github.com/poanetwork/threshold_crypto/pull/34#issuecomment-422108659, for more details and an initial suggestion of how such an allocator could look.

/cc @mbr, @DrPeterVanNostrand

[c0gent]

I do not think we should spend time on mlock at this time. It is not used in Parity and because of that is somewhat pointless for our primary use case.

I think we should disable it by default and put it on the backburner for now. I'm sure it will become something we'll want to pursue later on.

[afck]

I just wanted to have this in an issue instead of a PR discussion, for later. I agree: I'd also remove the whole mlock feature for now, until it works reliably.

[DemiMarie]

@c0gent should we suggest that users disable swap?

Ideally swap would be encrypted with a key generated fresh at each boot, making swap a non-issue from a security perspective. Is there a way to set this up?

[c0gent]

Disable their swap files? I'm not sure why we would suggest that. The swap file is only one level in a hierarchy of memory caches so there would still be potential vulnerabilities elsewhere even if it were encrypted.

Forgive me if I'm misunderstanding you.

[DemiMarie]

The swap file is the only one of those caches that persists across reboots, if I understand correctly.

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

This issue now has a funding of 250.0 DAI (250.0 USD @ $1.0/DAI) attached to it.

• If you would like to work on this issue you can 'start work' on the Gitcoin Issue Details page.
• Want to chip in? Add your own contribution here.
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $87,428.32 more funded OSS Work available on the Gitcoin Issue Explorer

[lamafab]

Hi

I can do this. Looking at the code I assume the allocator should hold the types that implement the trait ContainsSecret? I'm still not sure what the desired outcome is:

• Create a (global) manager that can be shared between threads and should be used by the implementers, but does not have to. The manager will allocate more memory if required. or...
• Types like SecretKey, (Bivar)Poly and Safe will hold the allocator (which includes the private data, of course) in an internal field. This would simplify memory allocation, since it does not have to grow. One does not have to care about how the secret data is handled internally -> mlock is used by design.

Whatever the decision is, this follow up questions must be answered, too:

• Do you just want an allocator which you can implement yourself or should it already be submitted implemented as a PR? The mockup code created by @mbr looks like a step in the right direction. Almost complete, actually.
• Read PAGE_SIZE or should we assume it's set at 4096?
• This library has memsec as a dependency, therefore memsec::mlock can be used. If PAGE_SIZE must be read than libc is required.

Let me know what you think. Also please do note that I can only work on this in my free time, but it shouldn't take too long.

[lamafab]

Any updates? I cannot start with this if your request hasn't been fully clarified.

[afck]

Sorry for the slow reply! I was traveling.

On a high level, we just want to make sure that secret values are only stored in locked pages and get zeroed after use, ideally without a significant performance hit.

Yes, the affected types should mostly be the ContainsSecret ones. But maybe the existing mechanism should be removed, and zeroing done by the allocator? (Not sure whether that's a good idea.) However, I wonder whether we should also protect temporary values in computations, and those could be anything, even types like Fr, provided by external crates.

I'm not sure whether it's an option to make the types hold the allocator as a field. Sure, we could add wrappers for e.g. Fr, and add that field to SecretKey; but how would we initialize that field e.g. when deserializing?

Yes, if possible I'd use @mbr's code; no need to duplicate the work!

Not sure about memsec and PAGE_SIZE; we do want to be Windows-compatible, too, in the long run, though.

[lamafab]

Hello @afck, welcome back. Thank you for clarifying. I will dig through the codebase and come up with a suggestion and PR.

[lamafab]

Hello @afck, in a few weeks I'll start a new job at a new company and I'm currently preparing myself for that position. You can leave this issue assigned to me, but I cannot guarantee you when/if a PR will follow. I'd have to work on this in my free time, which I can do once I get familiar and settled with my new job. Your clarification might be useful to the next person who works on this issue.

Thank you for understanding.

[collinalexbell]

@afck , currently I am working on a blockscout issue, but once that is finished and the PR is accepted I think I'll submit a proposal for this issue.

[gitcoinbot]

Issue Status: 1. Open 2. Cancelled

---

Workers have applied to start work.

These users each claimed they can complete the work by 1 year, 8 months ago. Please review their action plans below:

1) veloscillator has applied to start work (Funders only: approve worker | reject worker).

Hi all, I'd love to tackle this if still available. I'm an experienced C/C++ dev trying to get more real-world experience with rust by contributing to open source. Here's my plan:

1. Expand on @mbr's allocator. In particular I'd add: a. Slightly more efficient free list tracking. Probably just a bit array. Can also do a O(1) impl with linked list if desired, but might be overkill. b. Automatically allocate another slab once the current one becomes full. If mem usage is relatively static, this is probabaly not necessary. c. mlocking, etc.
2. Basically redo this change - https://github.com/poanetwork/threshold_crypto/commit/8f6dce18f28141cbcc79dda90dc2e1f587f60f0f - with the new way of allocating secure memory.
3. Tests.

Should be able to complete by the end of the weekend.

Learn more on the Gitcoin Issue Details page.

[veloscillator]

@afck Would be glad to take this on if you could approve ^ the above.

[afck]

Sorry, that's not my decision, but @igorbarinov's.

[gitcoinbot]

Issue Status: 1. Open 2. Cancelled

---

The funding of 250.0 SAI (250.0 USD @ $1.0/SAI) attached to this issue has been cancelled by the bounty submitter

• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $85,729.13 more funded OSS Work available on the Gitcoin Issue Explorer