dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.58k stars 4.55k forks source link

Add MemoryLockedBytes for critical pieces of data #27405

Closed pgolebiowski closed 4 years ago

pgolebiowski commented 5 years ago

Generic

mlock is a necessary feature when it comes to real security. Quote:

Cryptographic security software often handles critical bytes like passwords or secret keys as data structures. As a result of paging, these secrets could be transferred onto a persistent swap store medium, where they might be accessible to the enemy long after the security software has erased the secrets in RAM and terminated.

There is SecureString, but it's not really good to work with raw bytes (and actually strings in general, but it was introduced long before ReadOnlySpan<byte>). My uneducated guess is that having a SecureBytes equivalent, one could use ReadOnlySpan<byte>, as it is basically a pointer to real data. If this data is not swapped to the disk, then the mlock functionality would be provided.

This is basically a follow-up after https://github.com/dotnet/corefx/pull/31389.

Particular scenario

  1. The user provides the master key, stored as SecureString.
  2. The master key is used to decrypt another key in memory.
  3. ReadOnlySpan<byte> of this another key is used to initialize a new AesGcm instance.

Any thoughts how to do this securely?

@bartonjs @krwq @vcsjones @karelz

Drawaes commented 5 years ago

You mean like an emphemeral buffer that zeros and mlocks on Linux (and marks for no dumping) and just locks on windows?

https://github.com/Drawaes/CondenserDotNet.Leto?files=1

pgolebiowski commented 5 years ago

Yes, something in this area. Exemplary core part of the API:

public class MemoryLockedBytes
{
  public MemoryLockedBytes(int size);
  public MemoryLockedBytes(ReadOnlySpan<byte> bytes);

  public Span<byte> WritableSpan { get; }
  public ReadOnlySpan<byte> ReadOnlySpan { get; }

  // makes future calls of WritableSpan throw an exception
  public void MakeReadOnly();
}

Spending 5 minutes with your API does not make the usage obvious.

krwq commented 5 years ago

I think it might be worth to reuse ProtectedMemory here if we are going to be doing this.

Related: https://github.com/dotnet/corefx/issues/9058 cc: @GrabYourPitchforks

bartonjs commented 5 years ago

@krwq ProtectedMemory is a Windows proprietary algorithm, and we won't make it cross-platform.

bartonjs commented 5 years ago

@pgolebiowski I don't think it would help. Presumably the thing you're worried about is cryptographic keys; but on Linux our keys are given to OpenSSL where they make a copy. So the real place you need to use mlock memory is OpenSSL.

We use mlock for backing SecureString on Linux, but non-root users are rather limited in how much mlocking they can do. As a userspace API it would be fine, I guess, but we can't really depend on it internally for anything other than SecureString, because there are too many failure modes with it. (SecureString's okay because almost no one uses it, and we kinda discourage people from using it, so it doesn't really impact the mlock limits)

karelz commented 5 years ago

SecureString is deprecated: https://github.com/dotnet/platform-compat/blob/532b760d5e2016ef84696dfd2f0132eb04693651/docs/DE0001.md @blowdart @morganbr @GrabYourPitchforks will probably have opinions in the space ...

vcsjones commented 5 years ago

My 2c is that this is rather difficult to do in a way that fits going in to a framework.

  1. This is hard to do xplat and each platform behaves different ways. I'm not aware of which groups and users by default in Windows get the SeLockMemoryPrivilege. I don't believe normal users get it. Regardless, if this were to be an API, it should be designed in such a way that failure to lock pages in memory is expected to happen.
  2. It isn't complete. Keeping data off disk is hard to do in a way that is guaranteed. The swap file is one case, but what about a crashdump, or hibernate file? I feel that a consumer of this kind of API would not be fully aware of the "buts" when using this.
  3. Very few in my experience has used SecureString correctly. They would either construct the SecureString from an insecure string in the first place, or leak copies around, or incorrectly zero it, etc. It has a super narrow use case and usually results in misuse. The downside to that misuse is the API consumer believes they are protected from something when they really aren't. I feel that this API would fall in to a similar category.

I do believe this kind of functionality has value, but each use is specific with specific threat models and assumptions that would make it hard to do in a framework for multiple platforms.

Drawaes commented 5 years ago

I agree as with anything it's a risk based approach and there are tradeoffs not magic bullets. For instance I know in Linux I can keep it out of the crash dump but not windows. But then if someone has access to the crash dump they probably have access to the machine keys to decrypting memory anyway so you are on a hiding to nothing.

Having a "use this and all your problems melt away" (which is what people will assume if it's framework) isn't a great idea.

GrabYourPitchforks commented 5 years ago

Generally speaking, .NET can't provide any sort of guarantee about what data a region of memory contains at any given time. Even if you have a portion of memory locked / non-swappable, the underlying framework could have made a copy of that data in a stack-based buffer, an ArrayPool-based buffer (which may be copied), or even on the native heap.

Additionally, when we perform crypto operations, we go through the Windows CNG layer, which as an internal implementation detail simply mallocs a buffer as normal and shoves the secret directly in there. The secret data is cleared before free, but there's no CryptProtectMemory-like encryption applied and there's no guard against the data being paged out. Any dump file of your process would have these keys in cleartext.

Recent guidance has been that if you're concerned about the memory space of your application being compromised (perhaps through rogue code running in-process or a rogue user getting access to the process's memory dump), you should strive not to have the secret ever exist in-proc in the first place. This could mean using HSMs, delegating security-sensitive work to a more hardened process or enclave, and so on.

If for whatever reason you need something akin to desktop's ProtectedMemory right now, you could use ASP.NET Core's Secret class. But it's not meant for protection from rogue accesses per se. The scenario there is that you trust both the application code and the machine on which it's running, and you're just trying to get some extra hygiene. There was discussion about not even using it internally in the future because it doesn't add much value.

GrabYourPitchforks commented 4 years ago

SecureString-like functionality is not recommended for modern application development, and the Framework cannot reliably provide APIs which guarantee protection / non-pageability of sensitive in-proc data. See https://github.com/dotnet/platform-compat/blob/master/docs/DE0001.md for more information.

The recommended course of action for dealing with sensitive data would be to make sure that truly sensitive data is only operated on by processes specially-crafted for that purpose on machines which are locked-down. One potential course of action involves setting up a backend server with appropriate access controls and whole-disk encryption and where crash dumps have been disabled. The frontend would forward the encrypted data to the protected backend server (so the frontend never even has the decryption key) for processing. The backend application could be written in whatever language and Framework you desire, including .NET Framework or .NET Core, as long as standard machine-level hygienic practices are followed.

Closing this issue.