designs icon indicating copy to clipboard operation
designs copied to clipboard

SecureString obsoletions and shrouded buffer proposal

Open GrabYourPitchforks opened this issue 5 years ago • 55 comments

This document addresses obsoleting certain SecureString APIs for .NET 6. It is a type we have wanted to wind down for a while, and we have some outstanding work items that can address the reasons devs used SecureString in the first place.

This document also proposes a new API ShroudedBuffer<T> that is a more flexible and usable form of SecureString. There is a link to the work (already done) of rewriting SecureString in terms of this API as an implementation detail.

Related: https://github.com/dotnet/runtime/issues/30612

/cc @terrajobst @blowdart @bartonjs @jeffhandley

GrabYourPitchforks avatar Jul 21 '20 19:07 GrabYourPitchforks

And yes, the proposed API name is horrible. I leave it to you smart folks to figure out a better name. :)

GrabYourPitchforks avatar Jul 21 '20 19:07 GrabYourPitchforks

Video

  • We think we need to make decision whether or not the framework is interested in providing building blocks like ShroudedBuffer<T> in order for a customer to build hygienic solutions around handling privileged information, which also includes enables customers to build a PCI compliant solution. This requires and end-to-end how privileged is read into a process and how it leaves the process (file I/O, networking API, ASP.NET buffer pools, etc). This also includes GC behavior around copying memory, crash dumps, and logging.
  • We should think through what it means to obsoleted SecureString for APIs that can only be used be used with it, such as many PowerShell Cmdlets and some of our networking/crypto APIs.

terrajobst avatar Jul 28 '20 19:07 terrajobst

Also from the discussion: also consider whether the solution would be to obsolete the API + provide guidance about how to accomplish the scenarios. It's not always appropriate (or possible!) to provide a replacement one-size-fits-all API.

GrabYourPitchforks avatar Jul 28 '20 19:07 GrabYourPitchforks

One alternative is that we don't obsolete SecureString itself (since PS relies on it so heavily), but we do obsolete .NET APIs that interface with SecureString. That includes Process.Start, NetworkCredential.SecurePassword, etc. Then SecureString becomes an exchange type solely for PS's own use.

Ideally PS would be able to introduce their own PSSecureString type as part of their own SDK, and they'd be able to convert back and forth between that type and SecureString as needed. I don't quite know what this would mean for existing .ps1 or .ps1d files, though.

GrabYourPitchforks avatar Apr 01 '21 22:04 GrabYourPitchforks

Ideally PS would be able to introduce their own PSSecureString type as part of their own SDK,

I wonder if it is not "ideally" for .Net why do this ideally for PowerShell? Main PowerShell policy is to follow .Net. All power of PowerShell is in a power of .Net. If .Net consider to introduce new approach instead of SecureString PowerShell should be the first to benefit from it.

If SecureString will be reimplemented on SensitiveData<T> PowerSHell gets this automatically. Other applications too. It would be good step in .Net 6.0 milestone.

Next step obsolete SecureString? Obviously this is a big problem for the community without first creating a clear and modern alternative.

Alternative for what scenarios if we consider console applications including PowerShell?

  1. Using clear text passwords 1.1. Reading user input. 1.2. Keeping internally 1.3. Storing in an vault store 1.4. Passing to an OS API 1.5. Passing to remote system/service
  2. Using modern authentication tokens (for above scenarios) Can we use two factor authentication in console applications? Biometric? Windows Hello? Certificates/SmartCard tokens?

So main question is - can .Net introduce new API to address these scenarios in modern way? If yes, obsoletion of SecureString is not a problem - .Net (and PowerShell) community will enthusiastically switch to the new API.

iSazonov avatar Apr 02 '21 17:04 iSazonov

without first creating a clear and modern alternative.

The problem with coming up with any alternative is first describing what the type is supposed to defend against.

1.1. Reading user input.

I feel like PowerShell does have a mode that builds a SecureString correctly as part of input (Read-Host -AsSecureString). But I've seen quite a number of scripts that use Convert-ToSecureString... which means they already had the text in a .NET System.String, and (depending on what you're defending against) you've already lost the battle... or at least a non-predictable amount of time in a "you've lost" state.

1.2. Keeping internally

Again, what the right answer would be depends on what you think the threats are, and what costs you're willing to pay to defend against them. Also, how long are you keeping it, and how long does it stay loaded but unused?

If you have a SecureString that's being used for loading a PFX and you load that PFX 1000 times per second across parallel threads, you may as well have just kept it as a System.String, because there's going to be at least one plaintext copy in memory at any given time. On the other hand, if you only use the password once every 14 days, and you loaded it correctly, then you have pretty well minimized the risk that it shows up in a memory dump.

1.3. Storing in an vault store

You'll have had to upload it to the vault store, which probably makes it a plaintext message (then encrypted as part of a TLS message). Or it'll be encrypted using something like CMS enveloping... but had to be plaintext input (as bytes) to that. So, again, it had to exist as plaintext, why are you bothering with encrypt a character, decrypt it, add another character, re-encrypt the two characters, re-decrypt them, add a third, re-encrypt the three, etc?

If the upload to the vault was plaintext, have the contents in all the various buffers between you and the TLS encrypt operation been erased? Probably not.

Lets look at the other direction, reading it from a vault store. All the same things apply, plus the value possibly got loaded into a System.String instance, so now there's another ghost somewhere waiting for the memory to be reclaimed, reissued, and overwritten.

1.4. Passing to an OS API

SecureString isn't understood by the OS. Any time that a system call is made the secure string is decrypted. That's why if you're doing 1000 PFX loads per second there's going to be at least one plaintext copy in memory.

1.5. Passing to remote system/service

Same as the vault store... it has to be plaintext in memory somewhere.

Can we use two factor authentication in console applications? Biometric? Windows Hello? Certificates/SmartCard tokens?

Certs and SmartCards, totally. X509Certificate2 and X509Store.


Basically, we have a type. It has the word Secure in the name, so it must be good, right? Does it defend against anything you care about? Strong Maybe. Does it defend against it well? Weak Maybe. Does it defend against it in practice? Rarely (because the context where it got used bypassed the protections). Does it, on net, provide positive value? My opinion (which I believe is shared with the rest of .NET Security) is no: it provides marginal value some of the time, a false sense of blind confidence too much of the time, and is too cumbersome all of the time (and can't be made less cumbersome or it essentially never provides value).

What's a good replacement? I don't think there is one. The "SensitiveData" type in the proposal is largely just a code review marker, and a blocker for the ToString method. It's not a "replacement" for SecureString, but rather an acknowledgment that there can't be a good/universal one, but that sometimes "use good hygiene here" is good enough.

bartonjs avatar Apr 02 '21 18:04 bartonjs

Just curious, is there a recommended pattern in native code - if you zero out your password buffer before deleting it, can you generally assume that theOS or library API you pass the buffer to will consistently do the same?

danmoseley avatar Apr 02 '21 22:04 danmoseley

@bartonjs Thanks for your comments! I may be bad at writing...

I see that this is all about sensitive data (that is not only plain text passwords). And before killing SecureString , I would expect .Net team to do a full revision of all APIs where sensitive data is present and add a new API to close the omissions. To be concrete, have we API Console.ReadLineAsSensetiveData() to use in Read-Host -AsSecureString? How could we pass the data after reading to HttpClient or AccountManagement? Does the library generally treat URLs like able to have a sensitive data? How will we annotate sensitive data? Will we have analyzer for best practice? This is what I mean by alternative. If such alternatives are offered, then everyone will be indifferent to the lack of SecureString. If we remove SecureString before that, it will look like extortion (it's a joke) of money for reworking a huge number of applications. :-)

iSazonov avatar Apr 03 '21 09:04 iSazonov

@iSazonov I believe @bartonjs explained that it is not clear how such a type might be implemented. They are not pushing back on the concept, just that it isn't possible.

danmoseley avatar Apr 03 '21 15:04 danmoseley

@iSazonov I believe @bartonjs explained that it is not clear how such a type might be implemented. They are not pushing back on the concept, just that it isn't possible.

@danmoseley The discrepancy arises because MSFT team is in the context of SecureString type, and I am in the context of an application. In other words, I see that the MSFT team is developing a new approach to working with sensitive information everywhere in .Net. And until this is done, it would be very strange to get Obsolete attribute on SecureString before this approach is implemented inside .Net, not to mention creating a new public APIs.

iSazonov avatar Apr 04 '21 06:04 iSazonov

@iSazonov Your concerns are already addressed by the comment at https://github.com/dotnet/runtime/issues/30612#issuecomment-523534346. In particular, .NET cannot by itself provide support for secure handling of secret data. This is a facility that must be provided by the operating system. Depending on your threat model, OS-based solutions might include: (a) taking your .NET-based workload and moving it inside a shielded VM; (b) sharding your application across a normal-world frontend and an enclave-based backend; or (c) simply running your workload under an isolated user account and configuring the OS to disallow memory dumps of this process.

None of these solutions is .NET-specific. There is no opportunity for .NET to expose additional APIs to configure these mechanisms. They're all configured either by the hosting provider or by the operating system. Quite literally, there is no work for the .NET team to perform. All of these solutions are available to developers - today - for applications running any version of .NET.

GrabYourPitchforks avatar Apr 05 '21 18:04 GrabYourPitchforks

Responding to some other questions that came in over the weekend..

Just curious, is there a recommended pattern in native code - if you zero out your password buffer before deleting it, can you generally assume that theOS or library API you pass the buffer to will consistently do the same?

@danmoseley No. Some APIs zero out their own internal buffers on a best-effort basis, but none of these are guaranteed. For example, Windows's bcrypt (core cryptographic) APIs allocate memory to hold key material, and they'll zero out that memory before freeing the internal buffers. However, the ncrypt (delegated cryptographic) APIs, primarily responsible for certificate management, use RPC under the covers. And the RPC layer doesn't make strong guarantees about temporary buffers being zeroed out before they're freed.

How will we annotate sensitive data?

@iSazonov That's why as part of obsoleting SecureString, we're also proposing adding a SensitiveData<T> type. It's self-annotating. If you have a field or a local of this type, that should indicate to callers "this data is sensitive."

Main PowerShell policy is to follow .Net. All power of PowerShell is in a power of .Net.

@iSazonov That's not the case today. PowerShell's usage of SecureString is already opinionated. For example, PS provides ConvertFrom-SecureString and ConvertTo-SecureString APIs, where they control the conversion between SecureString and "encrypted string" themselves. These "value add" behaviors are not based on any .NET-provided out-of-the-box functionality for SecureString.

GrabYourPitchforks avatar Apr 05 '21 18:04 GrabYourPitchforks

I've also seen some commentary (here and other issues) that we should provide guidance on how people should think of secrets management and to offer best practices. While I agree that would be optimal, to be fair I don't think we're the right team to provide that guidance, nor do I think .NET docs is the right place to contain such guidance. This guidance would heavily depend on the threats to your application.

As an example, in the high-security services I've written over my career, we drew a trust distinction between the frontend web server and the backend. In this model, the client would generate a message encrypted to a key held only by the backend. The client would then perform mutual SSL auth against the frontend and provide this encrypted message as the request payload. The end effect is that the frontend could authenticate the client, but it could not exfiltrate those creds (guarantee provided by http.sys on Windows), and it could not decrypt the actual request contents. The only thing the frontend could do is forward the message to the backend and proxy the response back to the client. In our threat model, a malicious frontend could drop messages, but this was a typical DoS no different from a network outage in the data center, so we weren't too interested in it.

In this scenario, all creds were fully protected at all times, even by a malicious actor who gained code execution permissions on the frontend itself. The worst such an attacker could do is attempt to exfiltrate the client's IP address or username from the mutual auth. For our threat models, this was acceptable. Depending on your jurisdiction or industry, your threat model may require you to go further, including moving the frontend authentication step itself out-of-process.

You'll note that there's no talk of SecureString or anything like that in this description. Because our threat model assumed a total frontend compromise, we had to ensure that our frontend never had an opportunity to observe the plaintext creds or plaintext request payload. That's the bar we had set for ourselves.

Clearly not every application needs such high-end security. The SensitiveData<T> type described in this document should be sufficient in preventing most mistakes, even though it makes no pretense of thwarting active attacks. But once we start getting into the realm of "but I need to ensure an adversary can't memory dump my process!" I really have to ask "who is your threat agent in this scenario?" Because now we're talking about somebody actively running code on the box, with permissions which are at least as privileged as the application's. And once you're in that scenario, no amount of generalized guidance that .NET can provide will be of any use whatsoever to you. You need a proper, customized security audit from a well-funded security organization. And by "well-funded" I mean prepare to shell out hundreds of thousands, perhaps even millions of dollars for such an audit.

GrabYourPitchforks avatar Apr 05 '21 18:04 GrabYourPitchforks

@GrabYourPitchforks Thanks for clarifications!

In particular, .NET cannot by itself provide support for secure handling of secret data. This is a facility that must be provided by the operating system.

This says that .Net Runtime communicates with externals (OS, processes, ...) crossing security boundaries. These communication points can contain secure or sensitive information which should be annotated and processed in "secure" way. In particular, I would expect that we would convert P/Invoke like method(string password) to method(SensitiveData<char> password). This would be self-annotated as you pointed. If this is not possible for some reason we need an attribute for method(string password) to indicate the sensitive point so that an analyzer suggest users to convert to/from SensitiveData<T> as quickly as possible. Another example could be reading a password from console. I'd expect something like SensitiveData<char> Console.ReadLine(). This is what PowerShell could use to address Read-Host -AsSecureString scenario. (I don't think PowerShell should stick with the SecureString type except for backward compatibility purposes. Thus we could obsolete AsSecureString switch and add new AsSensitiveData to follow .Net best practice.) If we get a similar API beforehand, then the recommendation "do not use SecureString" becomes clear.

iSazonov avatar Apr 06 '21 08:04 iSazonov

@iSazonov You're still trying to map .NET only concepts to Windows (and Linux APIs). As neither OS has a concept of .NET's SensitiveData it makes no sense to do so, as soon as you hit an OS API you have to convert it in order to use it.

blowdart avatar Apr 06 '21 20:04 blowdart

This says that .Net Runtime communicates with externals (OS, processes, ...) crossing security boundaries.

@iSazonov Not quite. A typical p/invoke just switches from the managed to the unmanaged world, but you're still within the current process. You've not crossed a process security boundary simply by virtue of performing a p/invoke. Now, that p/invoke might eventually lead to a security boundary being crossed: network i/o, RPC, etc. But that's a characteristic of the particular API being called and isn't directly tied to it being managed vs. unmanaged code.

I'd expect something like SensitiveData<char> Console.ReadLine().

Not needed. See also my response at https://github.com/dotnet/designs/pull/147#discussion_r607269722, where the question "do we need the ability to read an environment variable directly into a SensitiveData<char>?" was posted. The answer is no: call Environment.GetEnvironmentVariable or Console.ReadLine or MyConfigurationSystem.GetValue to read the data as a string (or byte[], or whatever its natural format is), then wrap the data inside a SensitiveData<T>. There's no need to avoid the initial materialization. It is an explicit non-goal of this design to avoid materializing the sensitive data in plaintext in memory. Rather, this design is intended to prevent inadvertent materialization of the sensitive data. This means that having the data in plaintext before it gets into a SensitiveData<T> - or in plaintext once you unshroud the SensitiveData<T> - is perfectly fine. The point of this type is to serve as an annotation to say "hey, this needs some extra care" - and to avoid people inadvertently materializing it as part of logging, serialization, or similar facilities.

GrabYourPitchforks avatar Apr 06 '21 22:04 GrabYourPitchforks

This means that having the data in plaintext before it gets into a SensitiveData<T> - or in plaintext once you unshroud the SensitiveData<T> - is perfectly fine. The point of this type is to serve as an annotation to say "hey, this needs some extra care" - and to avoid people inadvertently materializing it as part of logging, serialization, or similar facilities.

In this case, it is generally not clear why this new type is needed. (1) it is more flexible and more functional to use an attribute for annotations, this gives more options for analyzers. (2) If it is about "a defense-in-depth approach" why give up DAPI encipher. (3) If it is nothing about "security" why would someone use the new type and add complexity to their code and increase possible errors.

From this discussion, I conclude that if we get rid of SecureString in some application (for example, PowerShell) and use simple buffers, then this will not reduce its security. This job is not that hard to do. On the other hand, using this new type does not add seсurity and it is not worth the effort to use it.

iSazonov avatar Apr 07 '21 07:04 iSazonov

On the other hand, using this new type does not add saсurity

That's also clearly stated in the proposal. It is not intended as a standalone security feature. It is instead meant to act as a wrapper around the data to prevent inadvertent disclosure.

The DPAPI / separate heap / whatever functionality is solely an implementation detail to help guard against information disclosure in the event of heap corruption. It's not intended to prevent information disclosure in the case of a heap dump. This is also stated in the document.

GrabYourPitchforks avatar Apr 07 '21 16:04 GrabYourPitchforks

Status update

After looking through this feedback and conferring with some folks from PowerShell and other partner teams, I don't think it's appropriate to deprecate SecureString for the .NET 6 release. It's too widely used across the ecosystem, and it'd be best if we have a long-term replacement firmly in place before we start marking things obsolete.

However, I think in the .NET 6 release it would be entirely appropriate to have our "this represents sensitive data" type in-box and plumbed through an appropriate API surface. It can live side-by-side with types like SecureString, and for ease of migration we could even include translations between the two types.

The Secret<T> type

This represents a best stab at the type that this document previously called ShroudedBuffer<T> or SensitiveData<T>. I've renamed it Secret<T> both to reduce keystrokes and to simplify terminology.

namespace System.Security
{
    public sealed partial class Secret<T> : System.ICloneable, System.IDisposable where T : unmanaged
    {
        public Secret(System.ReadOnlySpan<T> buffer) { }
        public int Length { get { throw null; } }
        public System.Security.Secret<T> Clone() { throw null; }
        public void Dispose() { }
        public void UnshroudInto(System.Span<T> destination) { }
        object System.ICloneable.Clone() { throw null; }
    }
    public static partial class SecretExtensions
    {
        public static T[] ToUnshroudedArray<T>(this System.Security.Secret<T> secret) where T : unmanaged { throw null; }
        public static string ToUnshroudedString(this System.Security.Secret<char> secret) { throw null; }
        public static void Use<T, TArg>(this System.Security.Secret<T> secret, TArg arg, System.Buffers.ReadOnlySpanAction<T, TArg> spanAction) where T : unmanaged { }
    }
}

Discussion on Secret<T> API surface

The primary use case for this type is to mark a buffer of data as sensitive. Typical instantiations will be via Secret<byte> (for keys or narrow-char passwords) or Secret<char> (for wide-char passwords). I've put an unmanaged constraint on this type for now to avoid callers putting non-buffer data in here: we don't want people instantiating a Secret<string> or a Secret<byte[]> to protect the objects themselves rather than to protect their contents. Additionally, since T has an unmanaged constraint, any Secret<T> instance is completely immutable once created.

This type implements the ICloneable interface. The reason for this is that the type is also IDisposable, and there may be scenarios where you're handing the object off to a component who may destroy it. (Alternatively, you could be writing a component which consumes a Secret<T>, and you may wish to take ownership of the object.) The Clone method returns a duplicate whose lifetime is completely divorced from the original.

The UnshroudInto method copies the raw buffer contents into the provided destination. An exception is thrown if the destination buffer is too short. There is no TryUnshroudInto method, as it's expected callers will query the Length property if they need to be defensive against failure. The ToUnshrouded* accelerators create a T[] or a string, respectively. The lifetime of these returned objects is completely standalone from the lifetime of the Secret<T> itself.

The Use method is a convenience accelerator which encompasses unshrouding the Secret<T> into a temporary buffer, performing some action over that buffer, then destroying the buffer. It is spiritually similar to string.Create.

This type does not define a convenience Secret<T>.Empty static property accelerator. Such a property would lead to temptation to write if (theSecret == Secret<byte>.Empty) { /* ... */ }, which would perform a reference comparison and almost certainly isn't the desired behavior. If a consumer needs to check for empty inputs, the preferred mechanism is to query the Length property.

Changes to SecureString's API surface

The following changes are proposed to SecureString's API surface in order to help convert between SecureString and Secret<char> instances.

 namespace System.Security
 {
     public sealed partial class SecureString : System.IDisposable
     {
         public SecureString() { }
         [System.CLSCompliantAttribute(false)]
         public unsafe SecureString(char* value, int length) { }
+        public SecureString(System.Security.Secret<char> secret) { }
         public int Length { get { throw null; } }
         public void AppendChar(char c) { }
         public void Clear() { }
         public System.Security.SecureString Copy() { throw null; }
         public void Dispose() { }
         public void InsertAt(int index, char c) { }
         public bool IsReadOnly() { throw null; }
         public void MakeReadOnly() { }
         public void RemoveAt(int index) { }
         public void SetAt(int index, char c) { }
+        public System.Security.Secret<char> ToSecret() { throw null; }
     }
 }

These APIs represent a fairly straightforward conversion between the two types. Each conversion creates a new instance whose lifetime is independent of the instance from which it was created. In this API proposal, I opt to put a new ctor directly on SecureString rather than an extension method on SecureExtensions. This is because we should promote Secure<T> as the preferred alternative and keep SecureString as legacy, so any conversion APIs should remain on the legacy API surface and should not pollute the surface area of the preferred alternative.

Other ecosystem API changes

The commit https://github.com/GrabYourPitchforks/runtime/commit/7532410d14d7950241a87d5090af7bf1cb712e3b contains the full list of proposed libraries API changes, with the exception of Microsoft.Extensions.Configuration, called out under a separate section below.

The biggest API additions are likely to be the cryptographic APIs, which should accept a Secret<byte> or a Secret<char> for key or password inputs where feasible. For example, on SymmetricAlgorithm:

 namespace System.Security.Cryptography
 {
     public sealed partial class AesGcm
     {
         public AesGcm(byte[] key) { }
         public AesGcm(System.ReadOnlySpan<byte> key) { }
+        public AesGcm(System.Security.Secret<byte> key) { }
     }
 
     public abstract partial class SymmetricAlgorithm
     {
         public override System.Security.Cryptography.ICryptoTransform CreateDecryptor() { throw null; }
         public override System.Security.Cryptography.ICryptoTransform CreateDecryptor(byte[] rgbKey, byte[]? rgbIV) { throw  null; }
+        public override System.Security.Cryptography.ICryptoTransform CreateDecryptor(System.ReadOnlySpan<byte> rgbKey, System. ReadOnlySpan<byte> rgbIV) { throw null; }
+        public override System.Security.Cryptography.ICryptoTransform CreateDecryptor(System.Security.Secret<byte> rgbKey,  System.ReadOnlySpan<byte> rgbIV) { throw null; }
         public override System.Security.Cryptography.ICryptoTransform CreateEncryptor() { throw null; }
         public override System.Security.Cryptography.ICryptoTransform CreateEncryptor(byte[] rgbKey, byte[]? rgbIV) { throw  null; }
+        public override System.Security.Cryptography.ICryptoTransform CreateEncryptor(System.ReadOnlySpan<byte> rgbKey, System. ReadOnlySpan<byte> rgbIV) { throw null; }
+        public override System.Security.Cryptography.ICryptoTransform CreateEncryptor(System.Security.Secret<byte> rgbKey,  System.ReadOnlySpan<byte> rgbIV) { throw null; }
     }
 }

Other types under the System.Security.Cryptography.Pkcs and System.Security.Cryptography.X509Certificates namespaces are likewise candidates to be enlightened about Secret<T>.

This proposal does not update Process.Start, NetworkCredential, and certain other management types which are currently SecureString-enlightened. Those are specialized scenarios bound to specific target platforms, and we haven't been investing in those scenarios recently anyway. Builder types which take passwords (e.g., UriBuilder.Password) are similarly not updated for Secret<T>, as the point of those types is to perform string manipulation. Hiding the string contents would run counter to why people use such types in the first place.

Microsoft.Extensions.Configuration support

Microsoft.Extensions.Configuration contains two primary mechanisms for retrieving data from the configuration provider: (a) you can query the provider to get the string value directly; or (b) you can ask the provider to bind a configuration section to an object.

For retrieving a secret value (such as a database login credential) from a configuration section, we could define an extension method and encourage its use.

static class ConfigExtensions
{
    public static Secret<char> GetSecret(this IConfiguration config, string key);
}

public void Startup(IConfiguration config)
{
    string value = config["myKey"];
    Secret<char> secret = config.GetSecret("myKey");
    string unshroudedSecret = secret.ToUnshroudedString();
    Debug.Assert(value == unshroudedSecret);
}

Additionally, the configuration binding mechanism should be enlightened about Secret<T>, and it should support binding to properties of type Secret<T> as part of loading a config object. An example of this is shown below.

class MyOptions
{
    public string Key1;
    public Secret<char> Key2;
}

public void Startup(IConfigurationRoot configRoot)
{
    MyOptions options = new MyOptions();
    configRoot.GetSection("myConfig").Bind(options);

    // options.Key1 has the value corresponding to the key "myConfig:Key1"
    // options.Key2 has the value corresponding to the key "myConfig:Key2", but as a Secret<char>
}

Mechanisms which inspect an object graph and persist that graph to storage should treat Secret<T> objects as opaque. The entire point of Secret<T> is to prevent inadvertent disclosure. Components like Microsoft.Extensions.Logging must not attempt to unshroud these instances.

Conclusion

This summary attempts to cement Secret<T> in the ecosystem as a usable first-class exchange type for sensitive information.

If this is enough information to move forward with API review, I will split out the full list of API changes into its own proposal.

GrabYourPitchforks avatar Apr 23 '21 01:04 GrabYourPitchforks

/cc @maryamariyan @michaelgsharp @safern @tarekgh @ericstj @eerhardt as area owners for Microsoft.Extensions.Configuration. Check the message immediately above this one for potential impact to that library, and please feel free to share your thoughts.

GrabYourPitchforks avatar Apr 23 '21 01:04 GrabYourPitchforks

If this is enough information to move forward with API review, I will split out the full list of API changes into its own proposal

I'm still at a bit of a loss for why we really need this type and all this surface area and churn. It makes no guarantees about secrecy or security of the data. All it ends up being is a named type that we hope by name conveys "this is sensitive data so be careful what you do with it". There's a ton of such data in applications, of various forms well beyond strings, and we're not going to start changing every T that might contain sensitive data to be a Sensitive<T>; even if we could, the moment you need to pass it back as a T you lose the naming. I get the "defense in depth" argument, but I'm skeptical this provides enough depth or defense to be worth its weight.

stephentoub avatar Apr 23 '21 02:04 stephentoub

That skepticism is fair. But I think it's based on a misunderstanding of what this is intended to be. Generally, secrets and other sensitive data take the form of cryptographic keys, passwords, and connection strings. The typical representation of these types is byte[] and string. This API is intended to seed the ecosystem so that they can be taken as Secret<byte> and Secret<char>, respectively. (I'm not worried about a T explosion, as I think it's going to be very rare that any other T is interesting here.)

It's not expected that the typical app developer ever tries to crack open a Secret<T>. Most app developers should do nothing more than shuttle them around, treating them as opaque data. The real value for a typical app developer is that the wrapper forces opacity, preventing inadvertently including this data in logging and other diagnostics, and it provides a concrete type whose usage people can audit using static analysis tools. Such tools could audit usage of the Unshroud* APIs to ensure that developers are handling sensitive data in a manner consistent with company policy. (We've already seen interest in these tools for auditing SecureString usage.)

Consider one end-to-end scenario: use of Azure SDK APIs. To access storage, the application needs to read the SAS credentials from config, then get these credentials into the Azure SDK APIs. Generally the SAS credential is a connection string, but it could be a client id / client secret pair, or a client certificate, or a device identifier, etc. (See further examples.) This allows the credential to be flowed opaquely through your application, such that your own code never sees (or has opportunity to misappropriate) the credential, until it makes its way to the final library which consumes it.

string tenantName = "Bob";

IConfiguration config = OpenConfiguration();
string blobContainerUrl = config["tenant_" + tenantName + ":containerUrl"];
Secret<char> sasToken = config.GetSecret("tenant_" + tenantName + ":containerSasToken");

TenantInfo tenantInfo = new()
{
    Name = tenantName,
    ContainerUrl = blobContainerUrl,
    SasToken = sasToken,
    /* ... */
};

/* lots of program execution, time passes */

void ActOnBehalfOfTenant(TenantInfo tenant)
{
    BlobContainerClient client = new BlobContainerClient(
        new Uri(tenant.ContainerUrl),
        new AzureSasCredential(tenant.SasToken));
    client.DoSomething();
}

The AzureSasCredential class (part of the Azure SDK) would use the Secret<T>.Unshroud* APIs to extract the raw credential. And at this point, that instance is fully responsible for managing the credential data, including ensuring that it's only ever used in appropriate contexts. Hygiene within the application code itself is maintained. If the TenantInfo object is ever involved in logging or serialization, the SasToken property contents will remain confidential.

GrabYourPitchforks avatar Apr 23 '21 04:04 GrabYourPitchforks

I totally agree with @stephentoub

Why do we need Secret<T>/ShroudedBuffer<T>/SensitiveData<T> if we already have ProtectedData class? Can we make it non-static? We could enhance the class and port it to Unix in some way.

ProtectedData class is based on Windows DAPI as SecureString. So question is - do you think DAPI (and ProtectedData ) is also problematic and not recommended for use? If not, then I would consider this way as the main one. Especially since PowerShell users use it in an amazing way https://github.com/PowerShell/PowerShell/issues/11218#issuecomment-801068873 (It is possible because PowerShell uses CryptProtectData from DAPI to transformate SecureString).

As for sensitive data, the main thing here is to reduce the storage time. This data should be zeroed as soon as possible after use. But this does not require a new type at all. In fact, the compiler should insert the cleanup code as soon as the variable exited the current scope. To inform the compiler, we can use an attribute or a new C# keyword (like using or unsafe). It's more flexible. Then compiler can insert try-finally and cleanup code.

iSazonov avatar Apr 23 '21 06:04 iSazonov

But I think it's based on a misunderstanding of what this is intended to be

No, I understand it, I just don't believe in it.

stephentoub avatar Apr 23 '21 10:04 stephentoub

Not commenting on the utility, but FWIW Levi I'm really not a fan of the change to the proposed Secret naming. Terraform, among other tooling, already uses Sensitive to denote this functionality for similar use cases like preventing sensitive fields from getting printed to the command line and build logs. So it's a recognized term for the crowd I run with anyway, and I think it would be worth considering :)

I dislike bikeshedding, but just wanted to throw that one thing out there.

ericsampson avatar Apr 23 '21 12:04 ericsampson

ProtectedData class is based on Windows DAPI as SecureString. So question is - do you think DAPI (and ProtectedData ) is also problematic and not recommended for use?

SecureString is not based on DPAPI on any platform. DPAPI (ProtectedData) already has a very well-defined use case and behavioral guarantees. Repurposing that API to fit this scenario is not viable.

GrabYourPitchforks avatar Apr 23 '21 15:04 GrabYourPitchforks

@bartonjs When you get a sec, can you take a look at the System.Security.Cryptography.* ref diffs suggested in https://github.com/GrabYourPitchforks/runtime/commit/7532410d14d7950241a87d5090af7bf1cb712e3b? I think I identified all the places where we take a key or a password (and don't immediately turn around and expose it as a byte[], like SymmetricAlgorithm.Key does). If this seems like an explosion of changes and needs to be pared down please let me know!

GrabYourPitchforks avatar Apr 23 '21 17:04 GrabYourPitchforks

ProtectedData class is based on Windows DAPI as SecureString. So question is - do you think DAPI (and ProtectedData ) is also problematic and not recommended for use?

SecureString is not based on DPAPI on any platform. DPAPI (ProtectedData) already has a very well-defined use case and behavioral guarantees. Repurposing that API to fit this scenario is not viable.

Could you please clarify? SecureString uses CryptProtectMemory documented in DAPI .

Repurposing that API to fit this scenario is not viable.

That API is already used in PowerShell.

iSazonov avatar Apr 23 '21 18:04 iSazonov

SecureString uses CryptProtectMemory documented in DAPI .

There are two function families in play here: CryptProtectData and CryptProtectMemory. Even though both are defined in dpapi.h, the term "DPAPI" (when used by Windows security devs) usually refers to the CryptProtectData family of functions. The CryptProtectData family of functions is intended for the long-term persistence of data. This persistence survives reboots, and if your machine is AD-joined the key can be escrowed, allowing the persistence to survive hard drive reformats and to follow you across workstations. This also means that CryptProtectData / CryptUnprotectData can result in a network call or can fail. But importantly, for this scenario, SecureString / Secret<T> / etc. explicitly do not want the data to be persisted. Hence my comment that leveraging this function family is not viable.

On the other hand, CryptProtectMemory offers protection for transient data. The key lives only in kernel space. Once the process terminates, the kernel destroys the one and only copy of the key that ever existed. The data is irrecoverable at this point.

TBH it's a bit unfortunate that the Win32 functions are named so similarly but serve two wildly different purposes. But, eh, that ship has sailed. :)

GrabYourPitchforks avatar Apr 23 '21 18:04 GrabYourPitchforks

can you take a look at the System.Security.Cryptography.* ref diffs

It certainly looks like a lot. But it's mostly a lot of overrides, so probably OK.

bartonjs avatar Apr 23 '21 22:04 bartonjs