[PM-5693] CryptoService using memfd_secret on Linux #7

dani-garcia · 2024-10-22T16:11:54Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-5693

📔 Objective

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

Added a KeyRef trait and SymmetricKeyRef/AsymmetricKeyRef subtraits to represent the key types. Also a small macro to quickly implement them. KeyRef implementations are user defined types that implements Eq+Hash+Copy, and don't contain any key material
KeyEncryptable/KeyDecryptable are now Decryptable/Encryptable, and instead of taking the key as parameter, they take a CryptoServiceContext and a KeyRef. This way keys are never exposed to the clients.
KeyLocator trait is replaced by UsesKey. This trait only returns the KeyRef of the key required to decrypt it, instead of fetching the key itself.
Created a KeyStore trait, with some simple CRUD methods. We have two implementations, one based on memfd_secret for Linux, and another one based on a Rust boxed slice, that also applies mlock if possible.
Because both mlock and memfd_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 because memfd_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 created SliceKeyStore, which implements the CRUD methods from the KeyStore trait on top of a plain slice. Then the actual mlock and memfd_secret implementations just need to implement a trait casting their data to a slice. The data is stored as a slice of Option<(KeyRef, KeyMaterial)>, and the keys are unique and sorted in the slice for easier lookups.
Created CryptoService, which contains the KeyStores and some encrypt/decrypt utility functions. From a CryptoService you can also initialize a CryptoServiceContext
A CryptoServiceContext contains a read only view of the keys inside the CryptoService, plus a read-write ephemeral key store, for use by Decryptable/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-sm#1117

📸 Screenshots

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
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 confirmed
issue 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

Otherwise some Drop implementations can crash

github-actions · 2024-10-22T16:14:43Z

Checkmarx One – Scan Summary & Details – c50a09d3-37c7-4d91-9a9c-1923beada9ae

No New Or Fixed Issues Found

codecov · 2024-10-22T16:16:46Z

Codecov Report

Attention: Patch coverage is 78.51662% with 252 lines in your changes missing coverage. Please review.

Project coverage is 65.55%. Comparing base (a89b588) to head (275574f).

Files with missing lines	Patch %	Lines
crates/bitwarden-crypto/src/store/context.rs	48.43%	148 Missing ⚠️
crates/bitwarden-crypto/src/keys/encryptable.rs	27.88%	75 Missing ⚠️
crates/bitwarden-crypto/src/store/mod.rs	85.23%	22 Missing ⚠️
...itwarden-crypto/src/store/backend/slice_backend.rs	98.67%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main       #7      +/-   ##
==========================================
+ Coverage   64.43%   65.55%   +1.12%     
==========================================
  Files         188      195       +7     
  Lines       13832    15005    +1173     
==========================================
+ Hits         8912     9836     +924     
- Misses       4920     5169     +249

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Hinton

I've taken an initial look at this. I think the general concept is solid, there are some potential areas for improvements.

However the biggest pieces would be:

Documentation, we need to ensure we write solid documentation on the difference areas and ensure we are clear about how it's intended to be used.
Improve test coverage, since we're dealing with cryptography we want to be really sure nothing breaks in the future.

crates/bitwarden-crypto/Cargo.toml

crates/bitwarden-crypto/src/service/context.rs

crates/bitwarden-crypto/src/error.rs

crates/bitwarden-crypto/src/service/mod.rs

Hinton · 2024-11-04T10:24:13Z

crates/bitwarden-crypto/src/service/key_store/implementation/rust_slice.rs

Do we have any documentation for why we need to implement our own slice vs using built in?

Added some doc comments here:

sdk-internal/crates/bitwarden-crypto/src/store/backend/slice_backend.rs

Lines 40 to 44 in e7bfdc3

/// The reason why we're not using a Rust collection like `Vec` or `HashMap` is that those types

/// expect their memory to be allocated by Rust, and they will deallocate/reallocate it as needed.

/// That will not work for our usecases where we want to have control over allocations/deallocations

/// and where some of our strategies rely on using system-allocated secure memory for the storage,

/// like the Linux-only `memfd_secret` API.

crates/bitwarden-crypto/src/keys/encryptable.rs

crates/bitwarden-crypto/src/service/context.rs

crates/bitwarden-crypto/src/service/mod.rs

Hinton · 2024-11-04T14:09:06Z

crates/bitwarden-crypto/src/service/mod.rs

+use key_store::KeyStore;
+
+#[derive(Clone)]
+pub struct CryptoService<SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> {


question: We define a Keys struct which essentially acts as the KeyStore. Would it be a cleaner interface to define a KeyStore trait which both CryptoService and Keys implements, and CryptoService simply forwards calls to the inner store.

Initially I think I've tried to do something like that, but it was hard to get a good API when using a trait here and then also trying to be generic over key type / Encryptable implementation.

# Conflicts: # crates/bitwarden-crypto/src/lib.rs

dani-garcia · 2024-12-12T13:13:24Z

I've made a few changes while documenting this a bit, plus some of the review suggestions are included here as well:

memsec is disabled on windows for now, and we're not using my fork
There's been a few renames:
- CryptoService -> KeyStore. to move away from the service name
- Keys -> KeyStoreInner. This only exists to be wrapped with the RwLock, and everything in KeyStore is pretty much delegated to it, so it seems like a better name.
- CryptoServiceContext -> KeyStoreContext. To match the service rename
- KeyStore -> StoreBackend. To signify there are different backends per platform
- LinuxMemfdSecretKeyStore -> LinuxMemfdSecretBackend and RustStore -> RustBackend, to match the trait above.
Changed the generic params of the KeyStore to be a single KeyRefs, which makes the generic types everywhere much less verbose (Instead of having to do <SymmetricKeyRef, AsymmetricKeyRef> everywhere you can do <KeyRefs>.
Removed the generic type over KeyStoreContext global keys, and moved it to an enum.

The next steps for the crypto refactor after this are:

Move all the Encryptable implementations to the new trait: [PM-5693] Migrate SDK to CryptoService #8
Migrate existing APIs that deal with SymmetricCryptoKey/AsymmetricCryptoKey directly to deal with references instead (init_crypto functions, MasterKey, UserKey, PinKey, Key fingerprint etc).
Remove uses of KeyStoreContext::get_key|set_key from the non crypto crates, which shouldn't have any uses left after the previous step (maybe?).
Once no other uses of SymmetricCryptoKey/AsymmetricCryptoKey are left, make them private to bitwarden-crypto
Remove the now unnecessary boxing around SymmetricCryptoKey/AsymmetricCryptoKey

Some future API improvements:

Improve the design around local keys, instead of accepting any user type, create an opaque token and return it in the functions, so the only way for a user to get a valid local key ref is to obtain it from a function.
Currently the context implementation can be read-only or read-write over the global keys, depending on how it's initialized, and it's behavior changes slightly in both cases. I think it might be a better design to instead do something like SQL transactions. Contexts are always read-only, but they have a commit function that consumes them and applies the changes to the global KeyStore

Hinton

Still need to review the rust backend.

Hinton · 2025-01-13T11:29:11Z

crates/bitwarden-crypto/Cargo.toml

@@ -50,6 +50,9 @@ wasm-bindgen = { workspace = true, optional = true }
 zeroize = { version = ">=1.7.0, <2.0", features = ["derive", "aarch64"] }
 zeroizing-alloc = ">=0.1.0, <0.2"

+[target.'cfg(all(not(target_arch = "wasm32"), not(windows)))'.dependencies]
+memsec = { version = "0.7.0", features = ["alloc_ext"] }


Did we explore the alternative crate (memfd) which has more downloads?

Hinton · 2025-01-13T11:29:28Z

crates/bitwarden-crypto/Cargo.toml

@@ -50,6 +50,9 @@ wasm-bindgen = { workspace = true, optional = true }
 zeroize = { version = ">=1.7.0, <2.0", features = ["derive", "aarch64"] }
 zeroizing-alloc = ">=0.1.0, <0.2"

+[target.'cfg(all(not(target_arch = "wasm32"), not(windows)))'.dependencies]


Do we use memsec on mac?

Yeah, the mlock/munlock APIs work on any unix system, including Mac and BSDs. memfd_secret is Linux only

Hinton · 2025-01-13T11:30:04Z

crates/bitwarden-crypto/src/error.rs

@@ -23,6 +23,10 @@ pub enum CryptoError {
    MissingKey(Uuid),
    #[error("The item was missing a required field: {0}")]
    MissingField(&'static str),
+    #[error("Missing Key for Ref. {0}")]
+    MissingKey2(String),


Can we rename this to MissingKeyRef? Put it under MissingKey and add a comment to rename this once we refactor the usage.

Hinton · 2025-01-13T11:40:47Z

crates/bitwarden-crypto/src/lib.rs

@@ -80,6 +80,8 @@ mod util;
 pub use util::{generate_random_alphanumeric, generate_random_bytes, pbkdf2};
 mod wordlist;
 pub use wordlist::EFF_LONG_WORD_LIST;
+pub mod store;


question: Do we want to use pub mod here?

Hinton · 2025-01-13T11:41:56Z

crates/bitwarden-crypto/src/keys/mod.rs

@@ -1,5 +1,9 @@
 mod key_encryptable;
 pub use key_encryptable::{CryptoKey, KeyContainer, KeyDecryptable, KeyEncryptable, LocateKey};
+mod encryptable;
+pub use encryptable::{Decryptable, Encryptable, UsesKey};
+pub mod key_ref;


question: keys isn't public so key_ref should probably not be public either?

Suggested change

pub mod key_ref;

pub key_ref;

Hinton · 2025-01-13T13:54:25Z

crates/bitwarden-crypto/src/store/backend/implementation/linux_memfd_secret.rs

+
+// For Send+Sync to be safe, we need to ensure that the memory is only accessed mutably from one
+// thread. To do this, we have to make sure that any funcion in `MemfdSecretImplKeyData` that
+// accesses the pointer mutably is defined as &mut self, and that the pointer is never copied or


Suggested change

// accesses the pointer mutably is defined as &mut self, and that the pointer is never copied or

// accesses the pointer mutably is defined as `&mut self`, and that the pointer is never copied or

Hinton · 2025-01-13T13:58:20Z

crates/bitwarden-crypto/src/store/backend/implementation/linux_memfd_secret.rs

+            let ptr: NonNull<[u8]> = memsec::memfd_secret_sized(capacity * entry_size)
+                .expect("memfd_secret_sized failed");


When would memfd fail? If we run out of memory?

According to the docs (https://man7.org/linux/man-pages/man2/memfd_secret.2.html):

EINVAL flags included unknown bits. EMFILE The per-process limit on the number of open file descriptors has been reached. EMFILE The system-wide limit on the total number of open files has been reached. ENOMEM There was insufficient memory to create a new anonymous file. ENOSYS memfd_secret() is not implemented on this architecture, or has not been enabled on the kernel command-line with secretmem_enable=1.

So when the system is out of memory or the process has reached the file descriptor limit, or if the feature is disabled and not supported (but we check for that the first time)

Hinton · 2025-01-13T14:00:02Z

crates/bitwarden-crypto/src/store/backend/implementation/linux_memfd_secret.rs

+            // Initialize the array with Nones using MaybeUninit
+            let uninit_slice: &mut [MaybeUninit<_>] = std::slice::from_raw_parts_mut(
+                ptr.as_ptr() as *mut MaybeUninit<Option<(Key, Key::KeyValue)>>,
+                capacity,
+            );
+            for elem in uninit_slice {
+                elem.write(None);
+            }


This is probably unsafe? https://doc.rust-lang.org/std/slice/fn.from_raw_parts_mut.html

Hinton · 2025-01-13T14:02:17Z

crates/bitwarden-crypto/src/store/backend/implementation/linux_memfd_secret.rs

+        // SAFETY: The pointer is valid and points to a valid slice of the correct size.
+        // This function is &self so it only takes a immutable *const pointer.


Can you elaborate on how we know it's valid?

Hinton · 2025-01-13T14:05:50Z

crates/bitwarden-crypto/src/store/backend/implementation/mod.rs

+        return Box::new(key_store);
+    }
+
+    Box::new(rust::RustBackend::new().expect("RustKeyStore should always be available"))


Suggested change

Box::new(rust::RustBackend::new().expect("RustKeyStore should always be available"))

Box::new(rust::RustBackend::new().expect("RustKeyStore to always be available"))

dani-garcia added 30 commits August 19, 2024 14:02

Initial CryptoService impl

d7c7c3e

More work

6068e84

Refactor keystore to also handle resizes

50dc1b4

Merge branch 'main' into ps/secure-crypto-service

216eb25

Fix

4b846ed

Paralelization plus alternative to locatekey

7a01168

WASM support

c649bf0

Merge branch 'main' into ps/secure-crypto-service

b901ef7

Remove unnecessary trait

e2129ad

Inline cryptoengine

f708fcc

Impl KeyStore in Slice struct directly

30854f7

Reset the values to None after munlock has zeroized them

bed894a

Otherwise some Drop implementations can crash

Merge branch 'main' into ps/secure-crypto-service

709dfce

Fmt

f7eda88

Add benchmark

ce2343e

Respect no-memory-hardening flag

bcd712f

Fix cfg flags, silence warnings

38343d2

Export memfd correctly

f34ce02

Fix memtest

cc27320

Merge branch 'main' into ps/secure-crypto-service

281619d

Try fat LTO to fix windows

32d298f

Disable memsec on windows to fix it

c43aa08

Incorrect optional dep

dd37d1d

Try updating windows in memsec

45f3d32

Remove unnecessary bound

1f70169

Move store impl around a bit

22a8b17

Make KeyStore pub

c63f656

Merge branch 'main' into ps/secure-crypto-service-impl

db3f8d4

Some renames/simplifications

eb81684

Add some missing implementations

d279626

dani-garcia added 8 commits October 7, 2024 16:53

Merge branch 'main' into ps/secure-crypto-service

32088c7

Remove bench

f882fb2

Remove using

bec4786

Fix TODO

c2ffea6

Comment

c0d2b63

Merge branch 'main' into ps/secure-crypto-service

cacf4db

Fmt

f4ca816

Merge branch 'main' into ps/secure-crypto-service

748ba75

dani-garcia mentioned this pull request Oct 22, 2024

[PM-5693] Migrate SDK to CryptoService #8

Open

Hinton reviewed Nov 4, 2024

View reviewed changes

dani-garcia added 7 commits November 4, 2024 16:38

Merge branch 'main' into ps/secure-crypto-service

713ec80

Merge branch 'main' into ps/secure-crypto-service

8f8e378

# Conflicts: # crates/bitwarden-crypto/src/lib.rs

Documentation improvements, renames and refactors

14485e8

Fmt

84c0aca

Fix lints and tests

f38398e

Search/replace was a bit too powerful hah

fada9c1

Fix doclink

f9ed542

dani-garcia added 8 commits December 12, 2024 14:41

Add some more comments about why we're doing the custom slice

e7bfdc3

Add context doc

34b256e

Add more tests

fe6b28f

Add more docs

45c9ee2

Merge branch 'main' into ps/secure-crypto-service

ad8ed59

Fix wrong test

75b97fa

Merge branch 'main' into ps/secure-crypto-service

33ec078

Add ignore dep

275574f

Hinton self-requested a review January 13, 2025 11:28

Hinton reviewed Jan 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-5693] CryptoService using memfd_secret on Linux #7

[PM-5693] CryptoService using memfd_secret on Linux #7

dani-garcia commented Oct 22, 2024

github-actions bot commented Oct 22, 2024 •

edited

Loading

codecov bot commented Oct 22, 2024 •

edited

Loading

Hinton left a comment

Hinton Nov 4, 2024

dani-garcia Dec 12, 2024

Hinton Nov 4, 2024

dani-garcia Dec 12, 2024

dani-garcia commented Dec 12, 2024 •

edited

Loading

Hinton left a comment

Hinton Jan 13, 2025

Hinton Jan 13, 2025

dani-garcia Jan 13, 2025

Hinton Jan 13, 2025

Hinton Jan 13, 2025

Hinton Jan 13, 2025

Hinton Jan 13, 2025

Hinton Jan 13, 2025

dani-garcia Jan 13, 2025 •

edited

Loading

Hinton Jan 13, 2025

Hinton Jan 13, 2025

Hinton Jan 13, 2025

	/// The reason why we're not using a Rust collection like `Vec` or `HashMap` is that those types
	/// expect their memory to be allocated by Rust, and they will deallocate/reallocate it as needed.
	/// That will not work for our usecases where we want to have control over allocations/deallocations
	/// and where some of our strategies rely on using system-allocated secure memory for the storage,
	/// like the Linux-only `memfd_secret` API.

	// accesses the pointer mutably is defined as &mut self, and that the pointer is never copied or
	// accesses the pointer mutably is defined as `&mut self`, and that the pointer is never copied or

		let ptr: NonNull<[u8]> = memsec::memfd_secret_sized(capacity * entry_size)
		.expect("memfd_secret_sized failed");

		// SAFETY: The pointer is valid and points to a valid slice of the correct size.
		// This function is &self so it only takes a immutable *const pointer.

	Box::new(rust::RustBackend::new().expect("RustKeyStore should always be available"))
	Box::new(rust::RustBackend::new().expect("RustKeyStore to always be available"))

[PM-5693] CryptoService using memfd_secret on Linux #7

Are you sure you want to change the base?

[PM-5693] CryptoService using memfd_secret on Linux #7

Conversation

dani-garcia commented Oct 22, 2024

🎟️ Tracking

📔 Objective

Changes

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

github-actions bot commented Oct 22, 2024 • edited Loading

No New Or Fixed Issues Found

codecov bot commented Oct 22, 2024 • edited Loading

Codecov Report

Hinton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dani-garcia commented Dec 12, 2024 • edited Loading

Hinton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dani-garcia Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 22, 2024 •

edited

Loading

codecov bot commented Oct 22, 2024 •

edited

Loading

dani-garcia commented Dec 12, 2024 •

edited

Loading

dani-garcia Jan 13, 2025 •

edited

Loading