-
Notifications
You must be signed in to change notification settings - Fork 1
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
[PM-5693] CryptoService using memfd_secret on Linux #7
Open
dani-garcia
wants to merge
42
commits into
main
Choose a base branch
from
ps/secure-crypto-service
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,896
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Otherwise some Drop implementations can crash
New Issues
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7 +/- ##
==========================================
- Coverage 62.86% 62.23% -0.63%
==========================================
Files 179 185 +6
Lines 12685 13742 +1057
==========================================
+ Hits 7974 8553 +579
- Misses 4711 5189 +478 ☔ View full report in Codecov by Sentry. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-5693
📔 Objective
Migrated from bitwarden/sdk#979
I've been looking into using
memfd_secret
to protect keys in memory on Linux.The
memfd_secret
API is similar to malloc in that we get some memory range allocated for us, but this memory is only visible to the process that has access to the file descriptor, which should protect us from any memory dumping from anything other than a kernel driver.https://man.archlinux.org/man/memfd_secret.2.en
Using this requires changing how we store keys, so this is the perfect moment to go through removing the raw keys from the API surface.
Changes
KeyRef
trait andSymmetricKeyRef
/AsymmetricKeyRef
subtraits to represent the key types. Also a small macro to quickly implement them.KeyRef
implementations are user defined types that implementsEq+Hash+Copy
, and don't contain any key materialKeyEncryptable/KeyDecryptable
are nowDecryptable/Encryptable
, and instead of taking the key as parameter, they take aCryptoServiceContext
and aKeyRef
. This way keys are never exposed to the clients.KeyLocator
trait is replaced byUsesKey
. This trait only returns theKeyRef
of the key required to decrypt it, instead of fetching the key itself.KeyStore
trait, with some simple CRUD methods. We have two implementations, one based onmemfd_secret
for Linux, and another one based on a Rust boxed slice, that also appliesmlock
if possible.mlock
andmemfd_secret
apply their protection over a specified memory area, we need a Rust data structure that is laid out linearly and also doesn't reallocate by itself. Also becausememfd_secret
allocates the memory for us, we can't use a std type like Vec (reason is the Vec must be allocated by the Rust allocator [ref]). This basically only leaves us with a Rust slice, on top of which we'd need to implement insert/get/delete. To allow for reuse and easier testing I've createdSliceKeyStore
, which implements the CRUD methods from theKeyStore
trait on top of a plain slice. Then the actualmlock
andmemfd_secret
implementations just need to implement a trait casting their data to a slice. The data is stored as a slice ofOption<(KeyRef, KeyMaterial)>
, and the keys are unique and sorted in the slice for easier lookups.CryptoService
, which contains theKeyStore
s and some encrypt/decrypt utility functions. From aCryptoService
you can also initialize aCryptoServiceContext
CryptoServiceContext
contains a read only view of the keys inside theCryptoService
, plus a read-write ephemeral key store, for use byDecryptable/Encryptable
implementations when they need to temporarily store some keys (Cipher keys, attachment keys, send keys...).Migrated the codebase to use these changes in a separate PR: bitwarden/sdk#1117
📸 Screenshots
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes