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.

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

KES secure forgetting #255

Merged
merged 2 commits into from Apr 12, 2023
Merged

KES secure forgetting #255

merged 2 commits into from Apr 12, 2023

Conversation

tdammers
Copy link
Contributor

@tdammers tdammers commented Jan 24, 2022

This adds "secure forgetting" to the KES signature schemes.

The purpose of KES is to enable forward security, by regularly "evolving" signing keys, such that existing verification keys still work on signatures produced by the new signing key, but the old signing key cannot be inferred from the new one. This means that once a signing key has been evolved, and the old key has been deleted, leaking a new signing key can no longer compromise the integrity of signatures produced by the older key.

For this to actually work, however, it is crucial that absolutely no copies of the old key remain stored anywhere, otherwise there is still a nonzero possibility of leaking them.

This is where "secure forgetting" comes in.

In order to guarantee that sign keys remain in a confined storage location, from which they cannot accidentally leak, and that they are properly erased in a controlled and deterministic fashion, we do the following:

  1. Sign keys, and the seeds used to generate them, are stored in "MLocked" memory. The mlock API pins memory into physical RAM, preventing it to be swapped out to disk (this is important, because "deleting from disk" is not reliably possible on modern hardware).
  2. All manipulation of that data, including key generation and evolution, as well as the allocation, deallocation, and mlocking of key storage memory, happens through FFI calls into libsodium; on the Haskell side, we only hold raw pointers to C memory, so that the secrets never end up on the Haskell stack.
  3. To provide deterministic forgetting and correct sequencing of operations in the face of a non-strict language (Haskell), we force all access to signing keys and seeds through a monadic API, and provide instances for IO. (Future PRs may provide additional instances, e.g. IOSim for testing purposes).
  4. As an extra safeguard, we wrap the raw mlocked memory in ForeignPtrs, and set them up to perform a secure forgetting in the finalizer. This is not intended to be the normal use case, because we cannot be sure when the GC will run, if at all, but it helps mitigate potential programming errors that leave secrets un-forgotten.

NOTE that the current implementation as it is here has one loophole: signing keys and seeds are still serializable, and once serialized, they are no longer stored in mlocked memory, and so the protection does not work when the serialization features are used.

@tdammers tdammers force-pushed the tdammers/kes-forgetting branch 2 times, most recently from d51c39b to 5305dd1 Compare October 24, 2022 09:12
lehins added a commit that referenced this pull request Nov 9, 2022
Added cardano-mempool package to the repo, which will later be used in KES secure forgetting and mlocking the memory. See #255
@tdammers tdammers force-pushed the tdammers/kes-forgetting branch 2 times, most recently from 242c7ca to b94dc60 Compare November 10, 2022 12:40
iquerejeta pushed a commit that referenced this pull request Nov 21, 2022
Added cardano-mempool package to the repo, which will later be used in KES secure forgetting and mlocking the memory. See #255
@tdammers tdammers requested a review from lehins December 1, 2022 10:35
Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

This is looking good. I did review only a portion of this PR, I'll do a follow up review at later point.

cardano-crypto-class/cbits/mlocked-pool.c Outdated Show resolved Hide resolved
cardano-crypto-class/src/Cardano/Crypto/DSIGNM/Class.hs Outdated Show resolved Hide resolved
cardano-crypto-class/src/Cardano/Crypto/KES/Class.hs Outdated Show resolved Hide resolved
cardano-crypto-class/src/Cardano/Crypto/KES/ForgetMock.hs Outdated Show resolved Hide resolved
cardano-crypto-class/src/Cardano/Crypto/MonadSodium.hs Outdated Show resolved Hide resolved
Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

I went through the full implementation. It looks really good. Left some feedback, but nothing critical. I'll go through the test suite in the next day or two.

It is awesome that you were able to move all the logging into the test suite!

I didn't go into test in too much detail at the moment, but I noticed all the KES tests use the same lock. Is the idea to run each test in isolation? From what I know the implementation is totally thread safe, so by adding a lock we aren't making our tests any better. As far as mempool is concerned this lock does nothing for it, because GC is not affected by the lock and can run at any time. Unless I am missing something I'd suggest removing using locks in testing

cardano-crypto-class/src/Cardano/Crypto/DSIGN/Ed25519ML.hs Outdated Show resolved Hide resolved
cardano-crypto-tests/src/Test/Crypto/Instances.hs Outdated Show resolved Hide resolved
cardano-crypto-tests/cardano-crypto-tests.cabal Outdated Show resolved Hide resolved
cardano-crypto-tests/src/Bench/Crypto/KES.hs Outdated Show resolved Hide resolved
cardano-crypto-tests/src/Bench/Crypto/KES.hs Outdated Show resolved Hide resolved
cardano-crypto-tests/src/Test/Crypto/Util.hs Outdated Show resolved Hide resolved
cardano-crypto-tests/src/Test/Crypto/Util.hs Outdated Show resolved Hide resolved
@tdammers
Copy link
Contributor Author

I've made some more changes, and I think things should be in pretty good shape now. Specifically:

  • Addressed all the review comments
  • Added some more NoThunks tests
  • Changed SimpleKES to use DSIGNM, just like the other KES implementations
  • Added explicit mlocking in sodium_malloc, and error handling to detect when mlocking fails (plain sodium_malloc doesn't do this!)
  • Fixed a slew of previously undetected bugs caused by incorrect usage of mlocked memory (no explicit forgetting)
  • Cleaned up the testsuite
  • Changed some tests to be more efficient about mlocked memory
  • Reinstated the locks; it turns out that running too many tests concurrently does lead to exceeding the mlock limit
  • Added a small thing to the CI workflow that reports the current mlock limit on Ubuntu (useful for debugging purposes)

Windows builds are unfortunately still failing on the setup steps, but that is beyond my control and needs to be fixed on master.

@tdammers tdammers force-pushed the tdammers/kes-forgetting branch 2 times, most recently from 462e539 to dea754d Compare January 31, 2023 10:05
Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

Great piece of work.

My comments fall into three categories, but none of them are really blocking this PR, except only maybe the first point:

  • In KES newly allocated seed is not zeroed out
  • Suggestions with minor improvements
  • Some questions about unfinished business in tests

cardano-crypto-class/src/Cardano/Crypto/DSIGN/Ed25519.hs Outdated Show resolved Hide resolved
cardano-crypto-class/src/Cardano/Crypto/DSIGN/Ed25519ML.hs Outdated Show resolved Hide resolved
cardano-crypto-class/src/Cardano/Crypto/DSIGN/Ed25519ML.hs Outdated Show resolved Hide resolved
cardano-crypto-tests/src/Test/Crypto/DSIGN.hs Outdated Show resolved Hide resolved
cardano-crypto-tests/src/Test/Crypto/KES.hs Outdated Show resolved Hide resolved
cardano-crypto-tests/src/Test/Crypto/KES.hs Outdated Show resolved Hide resolved
cardano-crypto-tests/src/Test/Crypto/KES.hs Outdated Show resolved Hide resolved
cardano-crypto-tests/src/Test/Crypto/KES.hs Outdated Show resolved Hide resolved
Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

Very nice.

abailly-iohk added a commit to IntersectMBO/ouroboros-network that referenced this pull request Feb 9, 2023
Due to IntersectMBO/cardano-base#255 we must set
tighter upper bounds to unsure we don't run into 'cabal update' issues as
the interfaces have changed heavily
iohk-bors bot added a commit to IntersectMBO/ouroboros-network that referenced this pull request Feb 28, 2023
4356: Fix upper bound for cardano-crypto-xxx packages r=dnadales a=abailly-iohk

# Description

Due to IntersectMBO/cardano-base#255 we must set tighter upper bounds to unsure we don't run into 'cabal update' issues as the interfaces have changed heavily.



Co-authored-by: Arnaud Bailly <arnaud.bailly@iohk.io>
iohk-bors bot added a commit to IntersectMBO/ouroboros-network that referenced this pull request Mar 2, 2023
4356: Fix upper bound for cardano-crypto-xxx packages r=dnadales a=abailly-iohk

# Description

Due to IntersectMBO/cardano-base#255 we must set tighter upper bounds to unsure we don't run into 'cabal update' issues as the interfaces have changed heavily.



Co-authored-by: Arnaud Bailly <arnaud.bailly@iohk.io>
@tdammers tdammers merged commit a8762b6 into master Apr 12, 2023
153 of 155 checks passed
angerman added a commit that referenced this pull request May 12, 2023
In #255 we somehow removed the
```
c-sources: cbits/blst_util.c
```
from the library. This included the symbol: `size_blst_p1`, which _is_ used in `cardano-crypto-class` 😱 .

E.g. https://github.com/input-output-hk/cardano-base/blob/e48f2ec561713f7a0863e58e34004b8099079cab/cardano-crypto-class/src/Cardano/Crypto/EllipticCurve/BLS12_381/Internal.hs#L372

And thus we now run into undefined symbols issues 🤯
angerman added a commit that referenced this pull request May 12, 2023
In #255 we somehow removed the
```
c-sources: cbits/blst_util.c
```
from the library. This included the symbol: `size_blst_p1`, which _is_ used in `cardano-crypto-class` 😱 .

E.g. https://github.com/input-output-hk/cardano-base/blob/e48f2ec561713f7a0863e58e34004b8099079cab/cardano-crypto-class/src/Cardano/Crypto/EllipticCurve/BLS12_381/Internal.hs#L372

And thus we now run into undefined symbols issues 🤯
lehins pushed a commit that referenced this pull request May 18, 2023
In #255 we somehow removed the
```
c-sources: cbits/blst_util.c
```
from the library. This included the symbol: `size_blst_p1`, which _is_ used in `cardano-crypto-class` 😱 .

E.g. https://github.com/input-output-hk/cardano-base/blob/e48f2ec561713f7a0863e58e34004b8099079cab/cardano-crypto-class/src/Cardano/Crypto/EllipticCurve/BLS12_381/Internal.hs#L372

And thus we now run into undefined symbols issues 🤯
lehins pushed a commit that referenced this pull request May 23, 2023
In #255 we somehow removed the
```
c-sources: cbits/blst_util.c
```
from the library. This included the symbol: `size_blst_p1`, which _is_ used in `cardano-crypto-class` 😱 .

E.g. https://github.com/input-output-hk/cardano-base/blob/e48f2ec561713f7a0863e58e34004b8099079cab/cardano-crypto-class/src/Cardano/Crypto/EllipticCurve/BLS12_381/Internal.hs#L372

And thus we now run into undefined symbols issues 🤯
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants