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

Obsolete the SecureString type #30612

Closed
GrabYourPitchforks opened this issue Aug 17, 2019 · 25 comments
Closed

Obsolete the SecureString type #30612

GrabYourPitchforks opened this issue Aug 17, 2019 · 25 comments
Assignees
Labels
api-ready-for-review API is ready for review, it is NOT ready for implementation area-System.Security
Milestone

Comments

@GrabYourPitchforks
Copy link
Member

The SecureString type does not actually fulfill the promises it makes to developers. MSDN already disrecommends its use in new code, and there's other documentation listing some technical reasons why it can't fulfill its promises.

We've already seen customers who use this type in server applications in an effort to fulfill auditing / compliance requirements, then those customers get bitten because they fail their audits. I detailed this a little more on Twitter, including suggesting mechanisms that customers could use to meet compliance requirements.

This legacy type is a different scenario than legacy types like ArrayList (the non-generic collection type). Types like ArrayList aren't recommended for new code because there are better alternatives available, but there's no real harm in keeping those older types around for compatibility reasons. Contrast this against SecureString, where the existence of the type is causing active harm to customers who use it and subsequently fail audits.

Since the type purports to make security promises to developers, and since it cannot fulfill those promises, it should be obsoleted in the next version of .NET Core.

/cc @blowdart @bartonjs

@GrabYourPitchforks GrabYourPitchforks self-assigned this Aug 17, 2019
@blowdart
Copy link
Contributor

blowdart commented Aug 18, 2019

@jozefizso You thumbed down this idea, so I'm assuming you use securestring right now. Is there any chance you could expand on your objection here?

@jozefizso
Copy link

Yes, for example the PasswordBox control returns password as SecureString. This prevents the password from being written out to log files and it’s required to work with some native Win32 authentication API.

The same way you can remove MD5 hash algorithm from .NET because people are using it to insecurely store passwords.

@blowdart
Copy link
Contributor

You've got about the only good example of using it right, powershell passwords being the other. MD5 at least as more legitimate uses (and is baked into the HTTP standard. Heck we can't remove 3DES because it's part of EMV).

If it was moved out of corefx and into a separate, unsuipported package, where eventually a lot of bad decisions will go and which would have a horrific name like Microsoft.Unsupported.UnsafeThings (this is why no-one lets me name stuff), where you could opt into using it, and would have to put an extra flag in your solution to maybe make people think some more, would that be helpful?

@GrabYourPitchforks
Copy link
Member Author

GrabYourPitchforks commented Aug 18, 2019

FYI I'm not suggesting deleting the type, only obsoleting it.

Jozef, who is the adversary in your scenarios threat model? How does the use of SecureString in this particular scenario thwart the adversary?

Edit: I ask not to be argumentative, but because there might exist an opportunity to create a better type for your scenario. We might be able to craft something more narrow in scope than what SecureString was trying to accomplish, and perhaps that replacement would be better suited for your application.

@SteveL-MSFT
Copy link
Contributor

In addition to logging, PowerShell also relies on this type to determine when to prompt without echo. Would it make sense to have a SensitiveString type that would also zero out memory on dispose? At least this would set some expectation, and maybe allow migration over time.

@jozefizso
Copy link

SecureString has it's obvious usages in the WinForms, WPF and PowerShell API.

Without providing any replacement nor information how to do it right and what will you do to make existing workflows like asking for passwords in console apps, password boxes, etc. and preventing passwords from leaking in places like loggin it does make sense to delete a class from .NET Framwork.

Having such a needed class in unsupported library? Ehm, no, just no.

@blowdart
Copy link
Contributor

Part of the point of the issue is to gather feedback on what you use it for, however "obvious" uses doesn't help us very much. We want to know

  • What your use case is? (If it's calling Win32 APIs then what Win32 APIs)
  • What are you trying to protect against in that use case? What's the threat its use is countering?

We don't want to let our own biases make assumptions, we want you to be explicit please.

@yuhong
Copy link

yuhong commented Aug 18, 2019

@GrabYourPitchforks
Copy link
Member Author

Related issue: GC support for zeroing the managed heap on compaction (https://github.com/dotnet/coreclr/issues/18386). The premise of that work item is to limit exposure of sensitive data in memory, regardless of whether it's in a string, array, or some other type.

@blowdart
Copy link
Contributor

@jozefizso I admit I am a little confused when you mention Win32. Windows has no idea about securestrings, they're purely a .NET concept. Which APIs are you thinking of here?

@bartonjs
Copy link
Member

@jozefizso That's not a Win32 method, that's the marshaller method for how to pass a SecureString to a C function that takes wchar*. (Decrypt it, pass the pointer to where it's decrypted) Whatever Win32 functions are being called could work just as well with System.String, or char[].

@stevenwdv
Copy link

I'm not a professional developer yet, so I don't know if my opinion counts, but it seems weird to me that this would be obsoleted. Aren't there still benefits in using this? If you're careful you can make sure that the string has a short, predetermined, lifetime.
But maybe I'm missing something: 'obsoleted' means that it is no longer needed because there is a better alternative, right? Can you tell me what this alternative would be? (For cases where we request a password from the user.) It seems to me that using a normal string would not be better, so I guess this is not what you mean.

@bartonjs
Copy link
Member

Aren't there still benefits in using this?

SecureString, on Windows, is useful if:

  • It is properly created
    • You populate it via a one-character-at-a-time source with no read-ahead, or
    • You populate it from a native wchar* (aka C# char*) in the SecureString(char*, int) constructor.
  • It is properly used
    • It is only ever used in a P/Invoke signature (or C# code that operates on the char* from the Marshaller output).

Effectively, if you've ever had the value in a System.String instance then you've lost all the "value" SecureString offers.

'obsoleted' means that it is no longer needed because there is a better alternative, right? Can you tell me what this alternative would be?

It depends on your threat model / risk concern:

  • If you believe someone is actively inspecting your memory (live or hiberfil.sys):
    • The password/sensitive-material needs to never be in your memory.
      • Windows: You need a secure desktop prompt that is feeding directly into LSASS, doing work in the LSA process, and sending only the "safe" result back to your process. (No, I don't think we have code to make that happen).
      • Linux/macOS: I don't know of a solution.
  • If you're concerned you'll accidentally print it in log statements:
    • Wrap a System.String in a struct with no ToString defined (or public override string ToString() => "** REDACTED **";
  • If you're just mildly concerned about the password being in WER uploads or pagefiles:
  • If you just like that it says "Secure" in it:
    • Just use System.String.

It seems to me that using a normal string would not be better, so I guess this is not what you mean.

It is. Just use System.String. Statistically speaking, you already had the value in a System.String (via a web request, or File.ReadAllText, or whatever), or you're at some point going to write a method like SecureStringToString, and now you've just warmed up your CPU for no real benefit.

If you're careful you can make sure that the string has a short, predetermined, lifetime.

So long as you never copied the data into byte[], char[], or System.String, sure. Most people aren't careful.

but it seems weird to me that this would be obsoleted

We actually docs-deprecated it more than 2 years ago: https://github.com/dotnet/platform-compat/blob/master/docs/DE0001.md, with a big warning on MSDN/docs.microsoft.com (https://docs.microsoft.com/en-us/dotnet/api/system.security.securestring?view=netframework-4.8)

image

Now we're just moving it to a compile warning so more people notice.

@stevenwdv
Copy link

@bartonjs Thanks for the info! It's unfortunate that people are not careful in their use of SecureString, of course in the cases you mentioned there is no difference except for the word Secure. I still have a question: is the point of this warning that people move to System.String, which is less secure or as secure at best (if you weren't careful with SecureString), or is the point that people avoid storing passwords in their memory altogether?

@jozefizso
Copy link

From the code sample on MSDN it was obvious, that the password is secured in a general scenario - PasswordBox up to the LogonUser.

As a developer, SecureString provides a promise of storing password securely and I don't have to deal with Secure Kernel stuff.

I don't know which one is worse - that you failed to document the SecureString class does not provide any security (it appears it behaves exactly contrary to documented behavior) or that you are removing a useful class.

Can it be misused? Of course it can. With such premise we can delete the whole .NET Framework and not just this single class.

@jozefizso
Copy link

PS: if you are concerned by misusing the SecureString class, what do you think people will do without it? They will create their own crypto so applications will be even more insecure.

@jhoneill
Copy link

It depends on your threat model / risk concern:

  • If you believe someone is actively inspecting your memory (live or hiberfil.sys):
    This threat applies equally to any secret in memory in a non-encrypted form. If the answer is "don't use passwords but use certificate based logon instead" the private key associated with the cert is in memory - is it removed quickly enough to be safe ? If a user has a password manager which pastes passwords into a browser, those are in memory (encrypted via the clipboard) for an unknowable period of time. Authenticators implemented as apps have their secret in memory....
    The only way to avoid this is for the secret to be stored and used outside the machines memory (e.g. by a smart card , TPM or co-processor), and can be mitigated by not keeping the secret in memory for any longer than needed.

The "DE0001: SecureString shouldn't be used" also starts with a misconception, I think,
its purpose - is not to avoid having secrets stored in the process memory as plain text. The primary use - and I am talking from the purpose of someone engaged in automation, rather than end user apps - is to persist secrets securely. This is a job it does reasonably well... If the concern is the user's typed password, anyone able to scan memory can probably log keystrokes.

Now we're just moving it to a compile warning so more people notice.
And that just teaches people to ignore compile warnings; unless the warning is to dispose of the secure string object safely as early as possible it is of a very little value

PS: if you are concerned by misusing the SecureString class, what do you think people will do without it? They will create their own crypto so applications will be even more insecure.

Indeed. Not only will that mean homebrew applications which store things less securely - "Just XOR everything with 42, it'll be fine", but the problem of the secret being in memory in the clear is not solved.

@daviburg
Copy link

daviburg commented Nov 1, 2019

SecureString is not perfect. SecureString can be misused. SecureString can be misunderstood.
Deprecating SecureString is pretty much giving up on providing a solution for providing some degree of protection of secrets.

No, SecureString's value is not limited to Win32 APIs. We are working on Azure integration service and we deal with passwords, credentials and secrets all the time (we handle over 300 services, which have a great range of authentication methods). We don't want those in the clear in memory. We do provide support for non-password based credentials when possible, which is not always the case. There are many, many APIs -local OS and remote services- which still rely on username password credentials. Some 3rd party clients have learn to use SecureString to protect the passwords they pass to their own server service (with on-the-wire encryption, so that the secrets goes from in-memory encrypted to on-the-wire encrypted).
We further would like to see SecureString implemented in more client for security sensitive content such as HTTPS, KeyVault, ... so that we can do as third party have learnt to do (keeping secrets encrypted at rest, on the wire, in-memory at all time while never decrypting full buffer at once).
We also use SecureString as a way to flag all sensitive fields, it makes it much easier to spot them for stripping for logs, UX, and even have different user access level controls (e.g. an ops engineer can see the service's health but can't see secure payload, or JIT access approval is required).

Removing SecureString and telling developers to go implement their own encrypted approach is baffling. Improve it instead. Provide better pattern. Implement code analysis detector for unsafe use of SecureString. Don't give up on security. Ever.

@GrabYourPitchforks
Copy link
Member Author

Removing SecureString and telling developers to go implement their own encrypted approach is baffling. Improve it instead. Provide better pattern. Implement code analysis detector for unsafe use of SecureString. Don't give up on security. Ever.

Nobody is telling developers to implement their own approach to encryption, and nobody is giving up on security. In fact, the proposed replacement (see https://github.com/dotnet/coreclr/issues/18386) clears out memory more aggressively and prevents secrets leakage better than SecureString ever did. It's a way to move the entire platform forward while leaving behind types like SecureString which were never able to live up to their promise.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 5.0 milestone Feb 1, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Mar 3, 2020
@sterenas
Copy link

Very interesting discussion around SecureString. We've got some services that connect to Azure SQL DB from a WinForms app (No ASP.NET). Need to encrypt the ConnectionString's password (this password is not typed, by an user, it's hardcoded in the ConnectionString itself). I'm not able to use Azure's AKS as I'd still need to hardcode at least one secret. Would you recommend SecureString or any other approach to get this done securely? Thanks!

@GrabYourPitchforks
Copy link
Member Author

@sterenas Your scenario involves encryption of data at rest, such as connection strings being read from a config file? SecureString never helped with those scenarios; it only ever worked on ephemeral data.

If you've got persisted data you need to encrypt on the local box, using X.509 certificates (for server farms) or DPAPI (for consumer machines) is a typical solution. The data is encrypted at rest. Your application decrypts the payload locally and then can operate on the plaintext as it sees fit.

@jhoneill
Copy link

Your scenario involves encryption of data at rest, such as connection strings being read from a config file? SecureString never helped with those scenarios; it only ever worked on ephemeral data.

This may be technically true - in that when creating (for example) a PowerShell credential object it uses a secure string, but when we export that credential (e.g. get-credential | export-clixml cred.xml) the data is a long series of digits printed to a text file (not technically a secure string). That gets turned back into a secure string, and back into a credential. In that sense SecureString is pivotal to a process of securing keys and passwords at rest.

With a certificate the problem is merely moved. Whatever has the private key can authenticate. So now instead of having to keep a password or API key secure, the need is to keep the Private Key both secure secure at rest, AND available transparently.

@GrabYourPitchforks
Copy link
Member Author

So now instead of having to keep a password or API key secure, the need is to keep the Private Key both secure secure at rest, AND available transparently.

Certificate private keys can be marked non-exportable. As long as the cert is in the personal or machine store, the calling process never has access to the raw private key material. That's why using certs is so popular especially in headless environments like production servers. For consumer environments, DPAPI offers a similar capability.

@GrabYourPitchforks
Copy link
Member Author

GrabYourPitchforks commented Jul 23, 2020

Video

Updated proposal at dotnet/designs#147.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-ready-for-review API is ready for review, it is NOT ready for implementation area-System.Security
Projects
None yet
Development

No branches or pull requests