Microsoft-authentication-library-for-dotnet: [Bug] Deprecate SecureString usage and replace it with string
As per .NET guidance, we should not use SecureString, as it misleads about the object being "secure". It's not. We should use a string instead.
Guidance and in depth discussions:
bgavrilMS
All 19 comments
sounds good. This will be way simpler for customers!
jmprieur
on 1 Mar 2021
Is that really the guidance? How about ReadOnlySpan<char>? One cannot control the lifetime of string in .NET and the garbage collector won't clear the memory. With ReadOnlySpan<char> one can wrap an array and clear it at convenient time with CryptographicOperations.ZeroMemory.
filipnavara
on 1 Mar 2021
@clairernovotny - could you clarify the guidance here?
I am not sure about ReadOnlySpan either, but afaik Spans are not available in the .net45 and other old stuff we maintain.
bgavrilMS
on 1 Mar 2021
@GrabYourPitchforks @blowdart ?
clairernovotny
on 1 Mar 2021
On downlevel platforms there's also ArraySegment<T>. Don't get me wrong, I want to get rid of SecureString as much as anyone else. I had to tackle the same decision in an OpenPGP and SASL library and it was a desirable property for the caller to erase all the passwords from memory as soon as possible.
filipnavara
on 1 Mar 2021
Keep in mind that the password is coming in via a string to begin with. It could be a secret in Key Vault, something from an environment variable, whatever. It's already in a string.
clairernovotny
on 1 Mar 2021
I defer to Levi on whether a span backed by an array would enable clearing.
But yea, if it comes from a string, I mean, you aren't controlling that, so until we get a zero-ing GC its all a bit blah. Even then, as a library, you don't get to set GC policy.
blowdart
on 1 Mar 2021
Keep in mind that the password is coming in via a string to begin with.
I am not familiar with the API shape. If that's the case then I agree that once it is in a string any attempt to get it out is doomed to fail.
For libraries where I had to deal with passwords I made extra effort to never pass them as strings (except for unit tests).
filipnavara
on 1 Mar 2021
The guidance in question - https://github.com/dotnet/platform-compat/blob/master/docs/DE0001.md
The fun heated issue around it - https://github.com/dotnet/runtime/issues/30612
But yea, if it comes from config, environment, files, key vault, basically anything outside the app, it's probably coming as a string rather than a byte array. No actual APIs take SecureString and use them like that, they convert it back to a string and then pass it to windows apis, because secure string was only a .net thing.
blowdart
on 1 Mar 2021
No actual APIs take SecureString and use them like that, they convert it back to a string and then pass it to windows apis
At the P/Invoke interop layer you don't really need to go trough the .NET string type. It is perfectly possible to marshal it through char array or span. Just saying, off topic in this context.
filipnavara
on 1 Mar 2021
The comment in the linked thread pretty much sums it up all. If it, at any point, is string then just use string.
If there's a way to avoid it, as a library author, it's nice to offer a way to use char[], ReadOnlySpan<char> or other similar concept. I know that most of the library consumers don't care but there are genuine use cases where the passwords are not handled as string in the application and they are cleared as soon as technically possible.
filipnavara
on 1 Mar 2021
I defer to Levi on whether a span backed by an array would enable clearing.
If the array is pinned during the entire lifetime where it might contain sensitive information, or if the array is allocated on the POH, then there will only ever be one copy of the data in memory.* The span itself isn't a copy of the data; it's a pointer to the object that contains the data. You'll see this "allocate an array, immediately pin it, use it within a _try_ block, and within the _finally_ block clear its contents" pattern used all over the ASP.NET crypto libraries.
The linked issue https://github.com/dotnet/runtime/issues/30612 had some ideas for runtime switches that would cause the GC to aggressively zero memory that was no longer needed, reducing the number of copies of _any_ potentially sensitive data that's coming across the wire. Consider a web server that brings a username + password or credit card number in as a form field input. It'll end up copied in several places: in the original request buffer, materialized as a string, in the array pool due to the transcoding process, etc. This switch tries to reduce active copies of the data where feasible.
The proposed _SecureString_ replacement https://github.com/dotnet/designs/pull/147 "solves" the problem by not attempting to provide any security guarantees at all. The proposed replacement is instead a marker type which says "the buffer contained herein contains sensitive information." The point of the marker interface is to make obvious in application code when sensitive data is being handled so that it's not inadvertently written to logs or exception messages. To accomplish this, the marker type exposes concrete APIs which allow raw access to the underlying data, but the more traditional APIs like _ToString_ continue to mask the data.
_* This is a dirty lie we tell ourselves so that we can sleep better at night. If you pass this data into pretty much any library API, we're going to end up making a copy of the data somewhere into a temporary buffer. Maybe it's on the stack. Maybe it's in the [unpinned] managed array pool. (And now the copy gets promoted to gen 2!) These copies are independent of external factors like the OS deciding to swap the data out to a page file._
GrabYourPitchforks
on 2 Mar 2021
Also, if I can pick on this a little bit more, let's track the flows of a single _SecureString_ instance through this library.
One copy created (_SecureString_ -> _char[]_):
Another copy created (_char[]_ -> _string_):
Multiple copies created (_StringWriter_'s internal buffer, _XmlWriter_'s temporary encoding buffer):
Another copy created (_StringWriter.ToString_):
Another copy created (_UTF-16_ -> _UTF-8_ transcoding):
That's __at least six__ distinct "insecure" copies of the original _SecureString_ instance being created, each with their own separate lifetimes, some possibly ending up in long-lived shared object pools.
And the kicker is that this isn't an uncommon pattern. It's very typical for applications to convert a _SecureString_ instance back to a _string_ in order to use it. This library isn't doing anything particularly out of the ordinary, and if I were reviewing this code I'd say "Yup, that all looks normal!" and sign off on it, caveated that using _SecureString_ at the beginning of this flow adds complexity with seemingly no real security benefit.
GrabYourPitchforks
on 2 Mar 2021
I have to agree with @GrabYourPitchforks assessment on the code. The slightly sad part is that most of these uses of string could be done in a way where it uses char[] arrays, pinned buffers and clearing. However, I don't think the HTTP stack gives the additional guarantees about manipulation of request content so it would all just reduce the area a little for almost no benefit in the end since the password would still end up in one buffer or another.
filipnavara
on 2 Mar 2021
Yes, we could fix some of those copies, but the string value needs to be part of an HTTP request, either in the XML body of a SOAP request or in the form body of a normal POST request. And here the circle of trust ends.
This is the reason that the code passed code review and security review. We do know about those copies, and the implication of using char[] instead of string, but they are not avoidable.
The thinking here is that we minimize the amount of time the secret stays in memory in clear text format / is vulnerable. It's not a perfect solution, but it does add a bit to defense in depth. Unless it doesn't, in which case we'd deprecate this?
bgavrilMS
on 2 Mar 2021
It's not a perfect solution, but it does add a bit to defense in depth. Unless it doesn't, in which case we'd deprecate this?
I'm arguing above that it doesn't add any real defense in depth. It's really adding complexity and maintenance costs - and forcing the library to call unsafe APIs in order to perform the conversion. And forcing us to have this conversation explaining why even though you're using the type you're still ending up with half a dozen (or more!) plaintext copies of the secret floating around in memory in various GC heaps. To me, the cons outweigh the pros. That's why we are looking to deprecate it in the core libraries.
GrabYourPitchforks
on 2 Mar 2021
Would someone link to the detailed discussion on this (maybe in the 1st post?).
I don't want to take the technical content offtopic, but if the main post could link the more detailed debates or resources that have been mentioned it would really help those of us wanting to understand/educate ourselves a bit more.
_EDIT_: could someone add these to the top, i missed them on mobile as not part of main post. Might help folks like me not clutter this up ;-)
sheldonhull
on 2 Mar 2021
Would someone link to the detailed discussion on this (maybe in the 1st post?).
The initial post lacks a bit of context. Generally there is a problem either side of secure string. I have my password as plain text. Something needs my password in plain text form. Converting from string to secure string and straight back to string doesn't make anything any more secure.
I spend most of my time in PowerShell where the script analyzer spots a parameter like
[string]$Password and says it should be a secure string
So someone makes it a secure string and inside the function they add
$password = [pscredential]::new("dummyName",$password).GetNetworkCredential().password`
And $password can now be used in whatever needs the plain text password.
Now , instead of the user writing
Invoke-Something -password solarwinds123
They write
Invoke-Something -password (ConvertTo-SecureString -Force -AsPlainText "Solarwinds123")
Anything scavenging memory will find at least 2 copies of Solarwinds123 - because whatever _used_ the plain text probably left a copy behind. Some things do a better job of not leaving the junk behind than others.
Secure strings can be exported for re-use by the same account only. That means I can leave my encrypted copy of a some text secret (password, API key) on my machine and if another account gets it, it will be of no use to them, but anything running as _me_ can _trivially_ decrypt it. Even then there is a case for saying make the input to _whatever_ a plain text string and leave the securing of credentials to the caller.
jhoneill
on 2 Mar 2021
As a reminder, I'm approaching this from the perspective of a .NET runtime team member. We generally see two facets to this problem:
_SecureString_ claims to minimize the occurrence of plaintext secrets in memory. We've basically given up on the type being a good mechanism for doing this. The linked runtime issue goes into it at a high level, and the investigation in this issue ("where does the data flow?") provides a concrete example of why the runtime cannot make real guarantees here.
_SecureString_ is a marker type saying "this is sensitive data" to prevent inadvertent disclosure in logs or exception messages. This has value in concept, but the existing _SecureString_ shape isn't well-suited for this purpose. Our replacement proposal is more suited to such a marker type.
Secure strings can be exported for re-use by the same account only.
This is PowerShell-specific functionality. It's great value add, but from .NET's perspective it's not intrinsically tied to the underlying _SecureString_ type. You could run the same logic over a regular string, primitives like _decimal_ or _DateTime_, or anything else that can ultimately be projected as byte[].
GrabYourPitchforks
on 2 Mar 2021
Related issues
chrisevans9629
路
5Comments
dansiegel
路
5Comments
trwalke
路
4Comments
23W
路
5Comments
hausec
路
3Comments
Most helpful comment
The guidance in question - https://github.com/dotnet/platform-compat/blob/master/docs/DE0001.md
The fun heated issue around it - https://github.com/dotnet/runtime/issues/30612
But yea, if it comes from config, environment, files, key vault, basically anything outside the app, it's probably coming as a string rather than a byte array. No actual APIs take SecureString and use them like that, they convert it back to a string and then pass it to windows apis, because secure string was only a .net thing.