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

secrecy: consider adding mlock feature for SecretVec and SecretString #480

Open
ordian opened this issue Jul 28, 2020 · 10 comments
Open

secrecy: consider adding mlock feature for SecretVec and SecretString #480

ordian opened this issue Jul 28, 2020 · 10 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@ordian
Copy link

ordian commented Jul 28, 2020

//! Presently this crate favors a simple, no_std-friendly, safe i.e.
//! forbid(unsafe_code)-based implementation and does not provide more advanced
//! memory protection mechanisms e.g. ones based on mlock(2)/mprotect(2).
//! We may explore more advanced protection mechanisms in the future.

There is a rust crate https://github.com/darfink/region-rs that acts as a safe cross-platform wrapper for mlock.
Would you accept a PR to add optional mlock support for SecretVec and SecretString?

@tony-iqlusion
Copy link
Member

Potentially, however there are alternatives to consider, and the threat model that benefits from mlock() (attacker with access to swap and only swap, but somehow not /dev/mem, /dev/kmem, or a process's memory) is a somewhat dubious one and also one which can be mitigated with other strategies. Depending on the nature of the program, mlock() may also have unintended side effects as it works at a page-level granularity (this post is a fun read on that subject, and posits a probably unrealistic scenario of mlock misuse causing every logline to flow through swap accidentally).

Another alternative to consider is encrypting secrets in-place when they're not in use, decrypting them for access, then re-encrypting them when they're no longer borrowed. This approach likewise has some questionable parts, like "where do you store the key-encrypting-key (KEK)?" and under what threat model having encrypted secrets helps where an attacker can't access the KEK (which is mostly accidental exposure through exploitation of memory unsafety vulnerabilities).

It might be interesting to parameterize Secret<T> with pluggable strategies (e.g. as a generic parameter) to afford some choice as to what approach is used.

@tony-iqlusion tony-iqlusion added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Sep 30, 2020
@tony-iqlusion tony-iqlusion changed the title [secrecy] consider adding mlock feature for SecretVec and SecretString secrecy: consider adding mlock feature for SecretVec and SecretString Sep 30, 2020
@ordian
Copy link
Author

ordian commented Dec 17, 2020

Related to that, sequoia-openpgp uses encrypted memory for storing secret keys link:

This implementation on the other hand, derives a sealing key from a large area of memory, the "pre-key", using a key derivation function. Now, any single bitflip in the readout of the pre-key will avalanche through all the bits in the sealing key, rendering it unusable with no indication of where the error occurred.

@charleschege
Copy link
Contributor

@tony-iqlusion @ordian
Does this mean that the only implementing encryption like sequoia_openpgp does can solve the issue of secrets being read, without necessarily implementing mlock?

@tony-iqlusion
Copy link
Member

I think an approach like that which keeps secrets encrypted in memory and decrypts them on-the-fly in order to access them (re-encrypting them when done) is an interesting one, yes

@insanitybit
Copy link

insanitybit commented Sep 29, 2023

Interesting excerpt here:
https://msrc.microsoft.com/blog/2023/09/results-of-major-technical-investigations-for-storm-0558-key-acquisition/

Our investigation found that a consumer signing system crash in April of 2021 resulted in a snapshot of the crashed process (“crash dump”).

We found that this crash dump, believed at the time not to contain key material, was subsequently moved from the isolated production network into our debugging environment on the internet connected corporate network.

TBH I actually think this is a reasonable scenario to care about - crash information, etc, is often moved to other systems that may not be as hardened, and the attacker could not have just ptrace'd the process etc. Here it led to a major breach.

I also wonder, if not mlock, memfd_secret[0] could be a really interesting option. This addresses some aspects of the weaknesses with mlock given a threat model where the attacker position is on the same system as the process holding onto a secret. FWIW I think ptrace restrictions are increasingly common, as are basic proc acls, but ultimately the "we shipped secrets from a crash dump" issue is probably the more compelling case.

[0] https://www.man7.org/linux/man-pages//man2/memfd_secret.2.html

@tony-iqlusion
Copy link
Member

tony-iqlusion commented Oct 3, 2023

TBH I actually think this is a reasonable scenario to care about - crash information, etc, is often moved to other systems that may not be as hardened, and the attacker could not have just ptrace'd the process etc. Here it led to a major breach.

@insanitybit I can't speak to Windows specifically, but on Linux for cases where you want to prevent secrets from being swapped or included in crash dumps it would be better to use mlockall(2) which protects the entire process's memory including all transient secrets with a single simple system call, rather than trying to build a complicated unsafe code-ridden fine-grained secret arena using mlock(2) which only protects secrets stored in that arena.

Also, we're well aware of memfd_secret, however it hasn't been a priority due to its fairly unique model and requirements of a very modern Linux kernel. I'm also curious if it could be used to protect an an entire process's memory rather than using it as a secret arena.

I still think it might be interesting to include pluggable/configurable secret storage mechanisms using e.g. a generic parameter with a default strategy of zeroization, but I don't think it makes sense to make the default strategy one which swaddles secrets in a blanket of large amounts of unsafe code.

@insanitybit
Copy link

Wouldn't mlockall prevent crashdumps entirely, though? I assume crashdumps are still something that people want, they just want them to not include secret information.

AFAIK memfd_secret isn't for protecting a whole process, but I could be wrong there.

I like the pluggable idea. I wonder if an Allocator-like trait would work well.

@NuSkooler
Copy link

Any update on this?

@tony-iqlusion
Copy link
Member

If there were updates, they would be in this issue

@wiktor-k
Copy link

I also wonder, if not mlock, memfd_secret[0] could be a really interesting option.

Sadly it seems memfd_secret is only available if secretmem.enable=y kernel argument is passed (thus it's not a general solution).

This approach likewise has some questionable parts, like "where do you store the key-encrypting-key (KEK)?"

One thing I've been experimenting with is storing the KEK in an ephemeral TPM space (https://github.com/wiktor-k/tpm-box). This way not only it's not part of the memory space but additionally rebooting the computer erases it irreversibly.

Of course it's not a silver-bullet (an attacker with online access to the process memory may retrieve a KEK handle and use it to decrypt data) and it's rather heavy-weight but it may be a good fit in some scenarios.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

6 participants