User SecureString for Basic and Proxy passwords

[russcam]

This commit changes the BasicAuthenticationCredentials and ProxyPassword types
to use SecureString for holding the passwords in memory, as opposed to as a plain text string.

A plain text string is retrieved from SecureString only at the point at which it is required.
This prevents plain text passwords from being automatically emitted in cases where properties
may be enumerated and captured e.g. logging frameworks.

Closes #2505

[Mpdreamz]

Return of SecureString 🍰 we left it out for a reason for a long time no less because the Secure bit is debatable:

https://github.com/dotnet/platform-compat/blob/master/docs/DE0001.md was always a reason for us not to implement it.

A plain text string is retrieved from SecureString only at the point at which it is required.
This prevents plain text passwords from being automatically emitted in cases where properties
may be enumerated and captured e.g. logging frameworks.

Is reason enough to add it in though 👍

[RolfDeVries]

In some cases the createstring method returns a value that looks like chinese instead of the original string. I still have to investigate why. Is this a known issue?

I think it is a threading issue. Secure string does not seem thread safe, it encrypts and decrypts the buffer in place on Read actions without a lock.
https://referencesource.microsoft.com/#mscorlib/system/security/securestring.cs,77d68ea938f47705,references

Elasticclient is now no longer threadsafe, which does not comply to the docs
https://www.elastic.co/guide/en/elasticsearch/client/net-api/current/lifetimes.html

[russcam]

@RolfDeVries could you provide more detail about

1. how you're instantiating ConnectionSettings and ElasticClient
2. what platform you're running on and framework version
3. the environment it's running e.g. full trust

There should be just a single instance of SecureString created, with decryption handled by

elasticsearch-net/src/Elasticsearch.Net/Connection/SecureStrings.cs

Lines 15 to 33 in 0584f8d

	public static string CreateString(this SecureString secureString)
	{
	var num = IntPtr.Zero;
	if (secureString != null && secureString.Length != 0)
	{
	try
	{
	num = Marshal.SecureStringToBSTR(secureString);
	return Marshal.PtrToStringBSTR(num);
	}
	finally
	{
	if (num != IntPtr.Zero)
	Marshal.ZeroFreeBSTR(num);
	}
	}
	return string.Empty;
	}

which allocates an unmanaged binary string each time and copies the contents of SecureString.

[russcam]

A quick test with

var secureString = "password".CreateSecureString();
var count = 10000;
var tasks = new List<Task<string>>(count);

for (int i = 0; i < count; i++)
	tasks.Add(new Task<string>(() =>
	{
		var s = secureString.CreateString();
		Console.WriteLine(s);
		return s;
	}));

tasks.ForEach(t => t.Start());	
Task.WaitAll(tasks.ToArray());

returns the expected results, but I may be missing something.

[RolfDeVries]

When i use 4 parallel thread to read the password 1000 times it will fail:

using System;
using System.Security;
using System.Threading;
using Elasticsearch.Net;

namespace SecureStringTest
{
public class Program
{
private const string Password = "password";
private static readonly SecureString SecureString = Password.CreateSecureString();

    public static void Main(string[] args)
    {
        Console.WriteLine(SecureString.CreateString());

        for (var i = 0; i < 4; i++)
        {
            new Thread(ThreadStart).Start();
        }

        Console.ReadLine();
    }

    private static void ThreadStart()
    {
        for (var i = 0; i < 100; i++)
        {
            var actual = SecureString.CreateString();
            Console.WriteLine(actual);
        }
    }
}

}

Running on .net core 2.2 windows server 2016 with 6 virtual processors.

[russcam]

Thanks @RolfDeVries, can replicate on .NET Core 2.2.

[russcam]

Looks to be related to Marshal.SecureStringToBSTR() et. al. Marshal.SecureStringToGlobalAllocUnicode() works

public static string CreateString(this System.Security.SecureString secureString)
{
    var num = IntPtr.Zero;
    if (secureString != null && secureString.Length != 0)
    {
        try
        {
            num = Marshal.SecureStringToGlobalAllocUnicode(secureString);
            return Marshal.PtrToStringUni(num);
        }
        finally
        {
            if (num != IntPtr.Zero)
                Marshal.ZeroFreeGlobalAllocUnicode(num);
        }
    }

    return string.Empty;
}

[RolfDeVries]

Looks like its working

[russcam]

@RolfDeVries I've opened #3825 to address

[RolfDeVries]

Thanks very much for the quick response!