-
Notifications
You must be signed in to change notification settings - Fork 11
[PM-18100] Add mlock
and memfd_secret
implementations
#125
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
base: main
Are you sure you want to change the base?
Conversation
Great job, no security vulnerabilities found in this Pull Request |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #125 +/- ##
==========================================
+ Coverage 70.38% 70.58% +0.19%
==========================================
Files 215 218 +3
Lines 17043 17250 +207
==========================================
+ Hits 11996 12176 +180
- Misses 5047 5074 +27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e4906dc
to
0aa0b89
Compare
0aa0b89
to
c52afa4
Compare
mlock
and memfd_secret
implementationsmlock
and memfd_secret
implementations
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dani-garcia In the description you say
Note that for this implementation to be useful, we need to revert the boxing of keys so they are stack allocated. This would mean either not exposing them publicly, or having a separate internal key type.
Does this mean that we need to start using the KeyRef
traits? Why can't these be exposed publicly?
// Check that the pointer is readable and writable | ||
let result = unsafe { | ||
let ptr = ptr.as_ptr() as *mut u8; | ||
*ptr = 30; | ||
*ptr += 107; | ||
*ptr == 137 | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: what happens if it isn't? What is returned as ptr
if memfd_secret
isn't supported? Trying to write to an invalid pointer/pointer to memory outside of the process should cause a segmentation fault, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory it shouldn't happen, the memfd_secret_sized
function should return None
if it can't allocate or the feature is not supported, and if it returns Some
, then the file descripto is valid and open for read/write according to the API: https://man7.org/linux/man-pages/man2/memfd_secret.2.html#DESCRIPTION.
This was added more as a sanity check during development, so it can probably be removed.
return Err(AllocError); | ||
}; | ||
|
||
// Check that the pointer is aligned to the requested alignment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: is aligned
or isn't aligned
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to mean validate that the pointer is aligned, return an error if not
but I can see how that's a bit confusing.
So the case inside the if, we check if it's not aligned to return an error, and only continue after the if
when the pointer is valid. I'll try to rewrite the comment to be more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh yeah ok that makes sense, interesting how that can be interpreted differently :D Thanks!
So what these memory protection schemes do is protect a region of memory where we place the keys. In this case inside a hashmap. The reason why we apply the protection to the hashmap and not the individual keys is that some of these operations are slow, and others require page alignment, which would be wasteful if we did it per key. The problem is that if the keys are not stack allocated, then the only thing we're protecting is the pointers to the keys, as the keys would be allocated somewhere else. If you check the definition of our keys, they are all boxed inside: struct Aes256CbcKey {
enc_key: Pin<Box<GenericArray<u8, U32>>>,
} This was done initially to avoid the keys being stack allocated, which makes it easier to accidentally make multiple copies in memory when moving the keys around, but it also invalidates the use of combined memory protections like these. If we remove this boxing to make the This leaves us with a few options if we want these protections to be effective:
|
# Conflicts: # Cargo.lock # Cargo.toml
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-18100
📔 Objective
Implement some new backends for the KeyStore that use the OS memory protections:
memfd_secret
: On newer Linux only, but should protect any content in the store from being read from external programs, even if running as root. It also provides some minor protection against malicious kernel modules, but that can be bypassed.mlock
: Supported on Unix. Doesn't provide any real protection from reads, but ensures that the keys aren't swapped to disk. Note that Windows also supports a similarVirtualLock
API, but I've had some trouble with the crates depending on an olderwindows
crate, so it's not enabled for Windows yet.Note that for this implementation to be useful, we need to revert the boxing of keys so they are stack allocated. This would mean either not exposing them publicly, or having a separate internal key type.
This requires a MSRV bump to 1.80 for the use of LazyLock, but #256 requires 1.82 anyway.
For this implementation we're using the
hashbrown
crate, which allows us to create a HashMap that uses a custom allocator. I've implemented a custom allocator for the two protections mentioned above.To ensure that this works correctly, I've implemented a simple fuzzing test that executes thousands of random operations and compares the results and contents of these new backeds against the basic rust hashmap that we're using now.
⏰ 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