poanetwork/threshold_crypto

Efficiently allocate memory for secrets in locked pages.

afck opened this issue · 16 comments

afck commented

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 #34, specifically #34 (comment), for more details and an initial suggestion of how such an allocator could look.

/cc @mbr, @DrPeterVanNostrand

c0gent commented

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 commented

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 commented

@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 commented

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 commented

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

gitcoinbot commented

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.

lamafab commented

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.

  • 👍1
lamafab commented

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

afck commented

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 commented

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

  • 👍1
lamafab commented

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.

  • 👍1
collinalexbell commented

@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.

  • 👍1
gitcoinbot commented

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 - 8f6dce1 - 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 commented

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

afck commented

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

gitcoinbot commented

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