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

SecureString obsoletions and shrouded buffer proposal #147

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

GrabYourPitchforks
Copy link
Member

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: dotnet/runtime#30612

/cc @terrajobst @blowdart @bartonjs @jeffhandley

@GrabYourPitchforks
Copy link
Member Author

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

@terrajobst
Copy link
Member

terrajobst commented Jul 28, 2020

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.

@GrabYourPitchforks
Copy link
Member Author

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
Copy link
Member Author

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.

@iSazonov
Copy link

iSazonov commented Apr 2, 2021

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 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.


This does not change the user-observable runtime behavior of the obsoleted APIs. Any existing code compiled against these call sites will continue to work.

The above only proposes to obsolete the SecureString constructor and some specific receivers. The intent is that if an application does wish to continue using SecureString as an exchange type or for interop or manipulation purposes, we don't want to force them to sprinkle a bunch of unnecessary suppressions throughout the code base. Suppressing just the small handful of instances where these types are created or consumed should be enough for those developers to signal that they're comfortable using the type.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppressing just the small handful of instances where these types are created or consumed should be enough for those developers to signal that they're comfortable using the type.

Assuming we use a single diagnostic ID for all secure string related obsoletions they wouldn't have to sprinkle anything -- they could just globally turn this particular obsoletion off.

Comment on lines +9 to +18
- All `System.Security.SecureString` [ctor overloads](https://docs.microsoft.com/dotnet/api/system.security.securestring.-ctor)
- All `System.Net.NetworkCredential` [ctor overloads](https://docs.microsoft.com/dotnet/api/system.net.networkcredential.-ctor) which accept SecureString as a parameter
- [`System.Net.NetworkCredential.SecurePassword`](https://docs.microsoft.com/dotnet/api/system.net.networkcredential.securepassword)
- All [`System.Diagnostics.Process.Start`](https://docs.microsoft.com/dotnet/api/system.diagnostics.process.start) overloads which accept SecureString as a parameter
- [`System.Diagnostics.ProcessStartInfo.Password`](https://docs.microsoft.com/dotnet/api/system.diagnostics.processstartinfo.password)
- All overloads of the following `System.Security.Cryptography.X509Certificates.X509Certificate` APIs which accept SecureString as a parameter:
- [The ctor](https://docs.microsoft.com/dotnet/api/system.security.cryptography.x509certificates.x509certificate.-ctor)
- [`Export`](https://docs.microsoft.com/dotnet/api/system.security.cryptography.x509certificates.x509certificate.export)
- [`Import`](https://docs.microsoft.com/dotnet/api/system.security.cryptography.x509certificates.x509certificate.import)
- Any equivalent ctor or method on [`X509Certificate2`](https://docs.microsoft.com/dotnet/api/system.security.cryptography.x509certificates.x509certificate2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't call this out explicitly, but it looks like all these APIs have string equivalents (Process.Start doesn't, but one can construct ProcessStartupInfo and set PasswordClearText). I assume the (implicit) goal is that we'll ensure that the scenario (passing a credential) can still be achieved without using SecureString, correct?


> This list is largely drawn from the feedback at https://github.com/dotnet/runtime/issues/30612 plus our analysis of internal consumers.

### Storing credentials
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hardly an expert in this domain but I've built enough web sites and console apps for cloud services now that I see emerging patterns: tokens and OAuth. Both rely on secrets being passed to APIs and both tend to be strings coming from configuration or environment variables, both of which tend to be strings. NetworkCredential could be used (because both userName and password are nullable) but that seems like a clutch to me. Also, it would suffer the same problem as SecureString in that the origin is already a string (set of strings). The concern here isn't so much security/clear text data in memory, it's more the exchange type problem. If the origin is a string, people are super likely to use that as the exchange type and everything else becomes annoying. See Uri. We end up in a world where a ton of APIs are either only taking string, only taking Uri, and some both. This forces users to frequently convert back and forth which is annoying. And that was even an area where we had tooling early on to push people towards Uri (FxCop).

So without covering entry points and having good analyzers/fixers, promoting a type as the exchange mechanism (whether or not the type already exists) will be hard. Also, because string is so pervasively used today, we'll have to make it easy to convert back and forth between whatever exchange type we'll want and string. But this also means that the other benefits, such as avoiding the data in crash dumps, will be harder to achieve. I still believe it's doable but we should think of a strategy to get there and pair it with analyzers and code fixers to help folks to get better over time, by, for example suggesting to not convert a <the exchange type> to a string if the receiver also accepts <the exchange type>.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I expect SensitiveData<byte> to be used more frequently than SensitiveData<char>. This matches aspnet's existing Secret class today, whose purpose is to hold cryptographic key material. There is sample code for converting a SensitiveData<T> back into a T[] or a string (see line 94 of this doc). Is that code sufficient, or do you expect there will need to be first-class APIs for this?


That secrets originate as strings deserves a bit more attention. The secret almost certainly entered the application via an HTTP request, or it was read from config, or it was entered into a WinForms textbox. These copies of the secrets are still floating around in memory (potentially even in a shared pool!), and SecureString does nothing to mitigate their being leaked.

The only reliable mechanism for preventing secrets from getting into crash dumps is to disable crash dumps for applications which process sensitive data. For web servers, this might mean separating out the "sensitive" parts of the application (login, payment processing, etc.) into a separate service running in a separate secured environment. The remaining aspects of the web service can then run with normal crash dump diagnostics provided they never have access to the plaintext form of the sensitive data.
Copy link
Member

@terrajobst terrajobst Apr 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should expand the list of secrets a bit more and also list how much we can/should protect them. For example, what about tokens? OAuth client IDs/secrets? Private keys for signing? Certs? Passwords are just one class of secrets.

Practically, I'm not sure I know how I can extract the secrets processing in my apps -- ultimately I need to send a token in every request. I guess that would mean that we don't consider a token a secret because it's short lived? Again, I'm not an expert so maybe that's obvious to experts, but we might want to spell this out explicitly to make it clear what the scope is we're trying to solve here.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. What is secret is context specific and is up to the reader to define as it's domain knowledge. If we list things people will go "It's not on the list so it's fine"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point isn't to be exhaustive. The point is to enumerate common secrets to ensure we have sensible end-to-end across the APIs/technologies that need we need to (1) get them into the process, (2) passing them around and (3) handle them to the correct receiver.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Experience shows that's not how it will be read. It will become a check list, in an audit, just like SecureString use is now. I don't want to see that happen.

To take your token example, it's still context sensitive, based on what the token is, what it's sent over, how it's created, and if it's backed by token binding. Simply saying token is not enough, and to be enough you have to be exhaustive, and suddenly the paragraph becomes bloated and unwieldy, and will take constant maintenance as new secrets come along.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can provide an example showing something Immo mentioned in another fork: how do I read from the environment variables or config and populate an instance of this type? It's also a good opportunity for us to show very clearly "it's ok to materialize a temporary array or string; we're not trying to prevent this." It doesn't really answer the question of "what is a secret?", but TBH that question isn't really important. What is important is how to use this as an exchange type.


__We should take care not to position this "sensitive buffer" type as a security feature for the purpose of achieving compliance.__ It's intended solely for code hygiene purposes and to avoid accidental disclosure. Scenarios like achieving PCI compliance require more fundamental runtime work, such as that under consideration at https://github.com/dotnet/runtime/issues/10480.

This proposal intentionally excludes `ToString`, `ToArray`, or other methods which might unshroud the buffer. `ToString` is called frequently by library code as part of logging, and we always want unshrouding to be a _deliberate_ action. Additionally, we want the caller to maintain full control over the unshrouded buffer where the contents are written, which precludes "opinionated" APIs like `ToArray` which create such buffers themselves.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I buy that ToString() is called by logging and other code paths that don't need to unshroud the buffer; but to me this doesn't mean we can (or shouldn't) provide a method like UnshroudToString() or UnshroudToArray(). Otherwise it will be hard to insert the type into existing code bases where virtually everyone uses strings today. The benefit of using a well-known method like UnshroudToString() is that it can be audited. If everyone just does the string.Create trick it's harder to audit (and harder to tool with an analyzer).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm I can see both sides of that argument :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Providing a well-known method would be fine in spirit IMO. The point of this type is to prevent accidental materialization of the string / array. If somebody is intentionally doing it, who are we to stop them?

Though to be honest the reason I didn't add such methods in the first place is that I assumed people would be nitpicky about how they wanted the information to be materialized. "I want it in a pinned array!" or whatever. CopyTo is intended to be a compromise which allows the caller to allocate the buffer themselves, then the secret is copied into that buffer. So CopyTo becomes the auditable method call. But if we need a simple accelerator, we could add one.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd much rather not. A single CopyTo is auditable, adding more methods means more noise, and more chance for a bad audit to miss things. Keeping it to a single method is better in terms of auditing, there's less things to miss.


### Best practices for implementation

If the backing buffer for the sensitive data is stored in-proc, we should attempt to use a separate unmanaged heap for storing these values, and we should not hand out direct references to this heap. This helps mitigate the possibility of sensitive data being leaked due to corruption of the normal heap. This concept is [already in use by OpenSSL](https://www.openssl.org/docs/man1.1.0/man3/CRYPTO_secure_malloc_initialized.html) and a few high-sensitivity Windows services. If using a separate heap is not feasible, we should attempt to use .NET's pinned object heap.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the backing buffer for the sensitive data is stored in-proc, we should attempt to use a separate unmanaged heap for storing these values, and we should not hand out direct references to this heap.

It's not clear to me how we'd deal with how this data gets into the process in the first place, such as:

  • Microsoft.Extensions.Configuration
  • Environment variables

For example, we should consider a method like SensitiveData<char> Environment.GetSensitveEnvironmentVariable(string name).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's wrong with the following? It's short and simple and doesn't require API changes beyond what's proposed here.

string envVar = Enviroment.GetEnvironmentVariable(name);
SensitiveData<char> sensitiveData = new SensitiveData<char>(envVar);

Remember, the point of this type is not to prevent the initial materialization of strings / arrays, nor is it to hide the contents of this buffer from an active memory dump. The point is to mark a piece of data as "sensitive" and to prevent accidental re-materialization of this string / array. The code sample above fits with this spirit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember, the point of this type is not to prevent the initial materialization of strings / arrays, nor is it to hide the contents of this buffer from an active memory dump.

Which is good, since the entire environment block is already in dumpable memory 😄

Copy link
Member

@terrajobst terrajobst Apr 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember, the point of this type is not to prevent the initial materialization of strings / arrays, nor is it to hide the contents of this buffer from an active memory dump. The point is to mark a piece of data as "sensitive" and to prevent accidental re-materialization of this string / array. The code sample above fits with this spirit.

If we believe that to be the case, then this certainly simplifies the design.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which is good, since the entire environment block is already in dumpable memory

there is prior art for redacting dumps, eg., the triage dumps that blank out much of the data leaving only the stack. in theory, if they don't already, they could clean the environment block. but they couldn't clean an arbitrary part of the GC heap. so I'm not sure the analogy with the env block holds entirely?

@bartonjs
Copy link
Member

bartonjs commented Apr 2, 2021

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.

@danmoseley
Copy link
Member

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?

@KevinCathcart
Copy link

KevinCathcart commented May 20, 2021

One thing I would like to see is that the documentation not only not make any claims of special protections, but also to explicitly disclaim that any such measure implemented will be available on all platforms or remain the same going forward.

In order to do that one would need to mention that additional measures may be taken while being extremely vague about what is being done. I would propose wording like "The implementation might take additional steps to reduce the probability that the secret could be accidentally exposed, but the details are not necessarily the same on all platforms, and are subject to change".

I think that is sufficiently vague to not be promising anything, and is intended as basically a warning that you cannot look inside implementation and take the current behavior as a guarantee of what will always be done. New platforms may not be capable of everything, or a more efficient or smaller code size implementations could potentially be used at a later point that don't provide identical protections.

It also properly frames the intended purpose of any such protections. Even if measures like encrypting the secret or storing secrets out of process were to be used, they would be better understood as a imperfect mitigation against potential bugs like heartbleed then as protection against malicious use of memory dumps.

If one has stringent requirements for handling of a particular secret, one would be best off writing custom code to ensure that those requirements are being met.

@Joe4evr
Copy link

Joe4evr commented May 28, 2021

public void RevealAndUse<TArg>(TArg arg, System.Buffers.ReadOnlySpanAction<T, TArg> spanAction) { }

Isn't the convention for methods like this to name the type parameter TState? That's how it is on String.Create.

@filipnavara
Copy link
Member

filipnavara commented Jun 8, 2021

The reviewed API still has public System.Security.Secret<T> Clone() { throw null; } which should use the new namespace instead.

Same for public static string RevealToString(this System.Security.Secret<char> secret) { throw null; }.

@filipnavara
Copy link
Member

filipnavara commented Jun 8, 2021

What is the int return value of RevealInto? I assume it's the number of elements written but I could not find a definite answer.

@ericsampson
Copy link

The type being "System.Secret" seems like we're going to set up for too much conflict

I still think that Sensitive might be a better choice than Secret to help avoid the whole issue of interpretation that Secure caused. You'd just hate for all this work to amount to little because the same thing ends up happening again.
But I won't harp on this any more.

@danmoseley
Copy link
Member

@GrabYourPitchforks now this is approved, do you expect it will be in 6.0 or miss that? cc @shirhatti

@GrabYourPitchforks
Copy link
Member Author

@danmoseley I'm not optimistic about meeting 6.0. Last I heard there was a push to get this in for 6.0 but not at the expense of other features. If there aren't any objections from your end I'd feel more comfortable punting this to 7.0.

@danmoseley
Copy link
Member

That's fine with me -- I asked because @shirhatti was interested in adding API in Microsoft.Extensions that consume it. @shirhatti is 7.0 OK?

@shirhatti
Copy link
Contributor

If it were happening in 6.0, I wanted to make sure we had enough time to react in Extensions.
7.0 is fine by me.

@danmoseley danmoseley modified the milestones: 6.0.0, 7.0 Jun 16, 2021
@GSPP
Copy link

GSPP commented Jul 27, 2021

Is there precedence for putting extension methods on non *Extensions named types?

@stephentoub
Copy link
Member

Is there precedence for putting extension methods on non *Extensions named types?

Enumerable

@eerhardt
Copy link
Member

Is there precedence for putting extension methods on non *Extensions named types?

Also, it is preferred in a lot of cases to not name the type “*Extensions”. Once you name it that, the type is now locked to only contain extension methods.

@AraHaan
Copy link
Member

AraHaan commented Aug 13, 2021

I would prefer SecureString to face the same fate as BinaryFormater and the replacement type known as Secret<> to be the only way forward.

@Neme12
Copy link

Neme12 commented May 2, 2022

I watched the API review (https://www.youtube.com/watch?v=IHRPmHuDQvU) and have a few thoughts.

  1. Somebody (I think @GrabYourPitchforks) said that the type explicitly won't make any guarantees, it should just serve as a marker and encourage hygienic use and consumers shouldn't rely on any guarantees. I think the idea about hygienic usage makes a lot of sense - you are very explicit about revealing the underyling data with each method and as it primarily deals with spans, you can copy the data into your own buffer that you instantiate and whose lifetime you control, which means that you can (if needed) create a pinned buffer and then clear it appropriately when no longer used. It makes it very explicit that you're getting the underlying data and does encourage hygienic use. But I don't think is useful without any guarantees at all, because if the type doesn't at least guarantee that the data is cleared from memory after calling Dispose(), you may always copy the data into a buffer that you use hygienically and pin and clear properly, but that's useless if the type itself doesn't clear the data after you're done with it. I think there should be at least 1 guarantee of this type (as opposed to it being an implemention detail), and it is that calling Dispose will clear the data (including all copies possibly made by compaction) from memory, and there won't be any copy left.

    This could be implemented by clearing the buffer on Dispose and either pinning the underyling buffer on creation, or if there's ever the runtime feature about clearing data when compacting (GC Heap Compaction should clear the source data runtime#10480) and the implementation detects that this feature is turned on, it wouldn't even have to pin the data, just make sure to clear it on Dispose, since it will clear the only remaining copy of it. If you're not calling Dispose, I think it's fine if it doesn't get cleared when garbage collected, because the consumer didn't explicitly ask for it to be cleared. But I really feel like this is the 1 guarantee that the type should make. I don't think this is in any way problematic unlike the guarantees that SecureString made - encryption turned out to be hard to do an all platforms. But I think clearing the data is something completely different - that's something that can be done on every conceivable future platform and is easy to do. It doesn't have to guarantee that it won't appear in a crash dump, it doesn't have to guarantee anything about encryption or do any encryption at all. But without at least guaranteeing that the data will be cleared after Dispose and no copies will remain in memory, any hygienic use would be practically useless because the user can be hygienic all they want if the type itself isn't and defeats every purpose of it. Also, then it couldn't even be used as a building block with the PCI requirement that data has to be cleared from memory when no longer needed. And if the implementation does clear it, people might rely on that behavior anyway.

  2. Couldn't Secret<T> be a struct? It seems like it's really just a small and immutable wrapper type around a buffer so it might make sense to make it a struct to avoid allocations and make it more efficient. On the one hand, you could say that this is more of a security-oriented type, the implementation is not efficient anyway and security is a much more important concern than performance. But if making it a struct doesn't actually harm security at all, why not make it slightly more efficient that way. The idea that because a type is associated with slow and inefficient usage, an allocation isn't a concern, has already backfired for types like Task - in some cases, it does matter. In the future, this type might become really common and it could add up. Also, some libraries might avoid it because of the allocation.

    EDIT: Later I realized that this is actually problematic because the type is disposable and therefore not actually immutable. I still think it might be worth thinking about this a bit and if there's a way to make it a struct while making sure that people don't use it incorrectly, maybe even with a built-in analyzer.

  3. @terrajobst said that the problem with SecureString is that the name explicitly claims that it is secure and consumers might then have the wrong assumptions (which I agree with), and that the name Secret is much better because it doesn't imply anything being secure, it just identifies something as a secret. I don't really agree with this. To me, Secret also has some implications - it kind of implies that the data is stored secretly, suggesting that nobody else could have access to it. I think this name is almost as misleading as SecureString. That's why I think a name like SensitiveData or SensitiveBuffer would be much better as that's the only name that actually doesn't imply anything being secure or stored in a secret way and just identifies something as a sensitive piece of data. Also, Secret<T> doesn't say that it's actually a buffer of data - with that name, I would naively expect to be able to pass any T in there and that it would wrap a single value, not make a buffer.

  4. I think there should also be a constructor that takes T[] in addition to the Create method. Yes, the Create method was added because it's more convenient as the type argument can be inferred. But in some cases, you might prefer to create this using a constructor rather than the factory method, especially where you can use target type new - private Secret<T> _secret = new(...) is better than private Secret<T> _secret = Secret.Create(...). That's why I think there should be parity between the factory methods and constructors where there can be. It would of course be nice if the constructor could take string as well but that can't be done because it's a specialization of T.

  5. I feel like there should be an overload of RevealAndUse without TArg. Of course the overload with TArg is important as it lets you pass state into the lambda, but in some cases, you might not need any state at all - like when you're just calling static methods. With the arg being required, users will have to supply something like null, and then further realize that now they have to specify the type argument explicitly. I think it would be really nice if there was an overload that didn't take the argument.

  6. Should there be a RevealAndUseAsync method as well? That one could have a delegate that takes ReadOnlyMemory<T> instead of ReadOnlySpan<T> as a parameter. I'm sure there will be a request for this variant at some point if it's not there from the beginning.

  7. It seems that with the current shape of the type, the data will always have to be copied into the Secret<T>. This makes sense of course - the type wants to make sure that the data is a) really immutable, and b) allocated and stored in the way that the type itself controls. But I think it can use the same approach that this string.Create overload uses to make it possible to avoid the copy:

    public static String Create<TState>(int length, TState state, SpanAction<char, TState> action);

    the equivalent method on Secret would be:

    public static Secret<T> Create<T, TState>(int length, TState state, SpanAction<T, TState> action) where T : unmanaged;

    (I would probably also add a constructor equivalent of this, although that might not make sense if an async version of this is added as well. BTW, seeing that string.Create uses TState as the name of the type argument makes me think that RevealAndUse should also use TState as opposed to TArg.)

    This would allow Secret<T> to allocate the memory in whatever way it does that - pinned or in a custom heap etc, and then have the user fill the data in-place. This could allow users to use this type without making a copy whenever creating it.

  8. Regarding the 2 APIs added to SecureString and how they can only be added for .NET vNext and not made portable, because that would mean adding these method to Secret<T> and it would be unfortunate to have the new thing refer to the old thing, I think this alternative to make it portable might make a lot of sense:

    namespace System.Security
    {
        public static class SecureStringSecretExtensions
        {
            public static SecureString ToSecureString(this Secret<char> secret);
            public static Secret<char> ToSecret(this SecureString secureString);
        }
    }

    This would make it so that users of Secret<T> won't see the ToSecureString method unless they already have a using for the namespace in which SecureString (and not a lot of other things) resides, and users of SecureString will always see ToSecret. I think this is a nice solution and IMHO it would be a bit unfortunate if it's not made portable even though it could actually be done in a way that's nice and usable like this.

Those are my 2 (or it seems more like 200) cents. I hope some of these ideas might be useful. 😅


The backing buffer may be (but is not required to be) enciphered or marked do-not-dump. In practice, we will probably encipher data when we can to avoid accidental leakage of the sensitive data. We will probably not mark the pages as do-not-dump due to the difficulty of bookkeeping and the fact that both Linux and Windows primarily intend do-not-dump regions to be for large data sections (such as texture data for games), and they institute an upper bound on the number of pages which may be annotated as do-not-dump.

## New overloads on existing APIs
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about the IDataProtector api's?
maybe I'm storing some "sensitive" data like

  • passwords for unlocking PFX cert containers
  • api token's that are transported encrypted from a (more) secured backend

having the 1 byte at a time would also be very nice to limit what can be leaked from memory dumping attempts

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i always try to defense in depth whenever possible
we never know when we end up with the next version of Heartbleed
so the least amount of time data is spent (partially) unencrypted the better

@Neme12
Copy link

Neme12 commented Sep 1, 2023

Was this idea abandoned? :(

@danmoseley
Copy link
Member

This would make it so that users of Secret won't see the ToSecureString method unless they already have a using for the namespace in which SecureString (and not a lot of other things) resides, and users of SecureString will always see ToSecret.

I may be missing something but the proposal seems to be to put the new type in System.Security, so the using will inevitably be there?

@Neme12
Copy link

Neme12 commented Sep 2, 2023

@danmoseley The latest approved proposal in #147 (comment) places it in System.Buffers.

@HHobeck
Copy link

HHobeck commented Dec 15, 2023

Hi there.

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 - or in plaintext once you unshroud the SensitiveData - 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.

The design goal of @GrabYourPitchforks is exactly what I'm missing in the framework. As an application developer I can think of dozen of use cases. One has been described in the following proposal:

Anyway what I'm wondering because it was also mentioned multiply times from other community members but without answering the question: Why do we not introducing an attribute type to get a better integration in tooling and IDE or other use cases when it is not possible to use this type? I'm not saying to replace the Secret/ShroudedBuffer/SensitiveData (the last term gets a thumb up from me ;)) but to have it side by side. The intention remains independent of the fact if you use a data type or an annotation type. We not know what the use case might be and should not limit the user to use just a data type. The design goal can be extended to an annotation attribute as well. We not tarnish somebody's glory. Are we?

Edit: It could be also a best practice if you return a secret also to annotate the method or property with a deserves protection or deserve security attribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.