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

Add SecureString support for passwords. #48

Closed
wants to merge 1 commit into from
Closed

Add SecureString support for passwords. #48

wants to merge 1 commit into from

Conversation

JohnWFlaherty
Copy link

I added SecureString support in my fork and thought you might like it as well.

@ChrisMcKee
Copy link
Collaborator

ChrisMcKee commented Oct 24, 2019

Thanks for the contribution; it's not really recommended that you use securestring (its inclusion in the framework at the moment exists solely to avoid "breaking" parts of powershell far as I remember); the security gains are negligible to non-existent.

I've included references below. Thanks again. 👍

image
https://docs.microsoft.com/en-us/dotnet/api/system.security.securestring?view=netframework-4.8

DE0001: SecureString shouldn't be used

Motivation

  • The purpose of SecureString is to avoid having secrets stored in the process
    memory as plain text.
  • However, even on Windows, SecureString doesn't exist as an OS concept.
    • It just makes the window getting the plain text shorter; it doesn't fully
      prevent it as .NET still has to convert the string to a plain text
      representation.
    • The benefit is that the plain text representation doesn't hang around
      as an instance of System.String -- the lifetime of the native buffer is
      shorter.
  • The contents of the array is unencrypted except on .NET Framework.
    • In .NET Framework, the contents of the internal char array is encrypted.
      .NET doesn't support encryption in all environments, either
      due to missing APIs or key management issues.

Recommendation

Don't use SecureString for new code. When porting code to .NET Core, consider
that the contents of the array are not encrypted in memory.

The general approach of dealing with credentials is to avoid them and instead
rely on other means to authenticate, such as certificates or Windows
authentication.

ref: https://github.com/dotnet/platform-compat/blob/master/docs/DE0001.md

@ChrisMcKee ChrisMcKee closed this Oct 24, 2019
@JohnWFlaherty
Copy link
Author

Hi @ChrisMcKee, thanks for the consideration. I appreciate the references and your opinion.

Just as an aside:
I am aware. The reason SecureString was invented is still relevant and valuable even without encryption in memory. It is my opinion that the above recommendation from the core team is just their way of crying uncle. The idea is good but uptake and understanding never really materialized. The PasswordBox captures a password properly [or it used to anyways]. So far so good but when I want to make a hash there is no safe place to go. I could sandbox the string before calling your hash function but that won't work because internally you put a copy of my password in managed memory and all of my effort is wasted.

So yes, the idea was good, the implementation weak, and the uptake very poor. I could argue that your class should not have a managed string interface. 😃 (Since your purpose is to hash passwords.)

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

Successfully merging this pull request may close these issues.

2 participants