A hash-signature-type which tests on comparing against normal strings

I decided to throw this together today figuring it would be cool to add to my repertoire. Before we always had performed this "task" externally. Find the value we want to compare, hash it and compare with a valid hash. But my idea was to bundle this into a object which behaves nicely with string types. It then could internally compare the two and determine if they match.

I'm just curious on what you all think here and where I can improve. And if I've even done something any of you find could save time and find more usable than not.

Benefits

• No generic classes I have to drag around, it is now a type with a specific purpose.

• The data for the type is never stored in plain text, therefore should be safe, right?

Usage

ProtectedString ThisIsMyProtectedString1 = "";

ThisIsMyProtectedString1 = "Test";

if (ThisIsMyProtectedString1 == "Test")
    MessageBox.Show("Equal");

Le Code

```
public struct ProtectedString
{

private string _Data;

public String Data
{
get { return _Data; }
private set { _Data = value; }
}

private const int SALT_BYTE_SIZE = 24;

private const int HASH_BYTE_SIZE = 24;

private const int PBKDF2_ITERATIONS = 1000;

public ProtectedString(String value)
{
_Data = "";
_Data = CreateHash(value);
}

public static implicit operator ProtectedString(String value)
{
return new ProtectedString(value);
}

public static bool operator ==(ProtectedString c1, String c2)
{
return c1.Equals(c2);
}

public static bool operator !=(ProtectedString c1, String c2)
{
return !c1.Equals(c2);
}

public static bool operator ==(String c1, ProtectedString c2)
{
return c2.Equals(c1);
}

public static bool operator !=(String c1, ProtectedString c2)
{
return !c2.Equals(c1);
}

public bool Equals(String value)
{
Boolean result = false;
result = ValidateHash(value, _Data);

private string _Data;

public String Data
{
    get { return _Data; }
    private set { _Data = value; }
}

You should make this immutable by either declaring _Data as readonly and remove the setter or by having only public readonly string Data;.

In addition you should be consitent with the style you are using. Here you use one time the alias string and one time the String object. Most C# developers use only the alias, but thats a decision for you to make. Once you have made a decision you should stick to your style.

public ProtectedString(String value)
{
    _Data = "";
    _Data = CreateHash(value);
}

the assignment of "" to _Data is only adding noise to the code. Remove it.

because it is a struct the default parameterless constructor needs to be called like so

public ProtectedString(String value)
    :this()
{
    _Data = CreateHash(value);
}

public override bool Equals(object obj)
{
    return base.Equals(obj);
}

This should be changed in way that it is really doing an Equal of the class rather then only calling the base.Equals() like so

public override bool Equals(object obj)
{
    var protectedString = obj as ProtectedString;

    if (protectedString == null) { return false; }

    return _Data.Equals(protectedString.Data);
}

It could be possible that this isn't 100% correct, because I can't test it.

private string CreateHash(String value)
{
    RNGCryptoServiceProvider csprng = new RNGCryptoServiceProvider();

    byte[] salt = new byte[SALT_BYTE_SIZE];

    csprng.GetBytes(salt);

    byte[] hash = PBKDF2(value, salt, PBKDF2_ITERATIONS, HASH_BYTE_SIZE);

    return 
        Convert.ToBase64String(salt) +
        Convert.ToBase64String(hash);
}

RNGCryptoServiceProvider implements through its base class the IDisposable interface hence you should use a using block to proper dispose it. I would extract the generation of the salt to a separate method.

private byte[] GetSalt()
{
    using (RNGCryptoServiceProvider csprng = new RNGCryptoServiceProvider())
    {
         byte[] salt = new byte[SALT_BYTE_SIZE];

         csprng.GetBytes(salt)

         return salt;
    }
}

private string CreateHash(String value)
{

    byte[] salt = GetSalt();

    byte[] hash = PBKDF2(value, salt, PBKDF2_ITERATIONS, HASH_BYTE_SIZE);

    return 
        Convert.ToBase64String(salt) +
        Convert.ToBase64String(hash);
}

private bool ValidateHash(String value, String validHash)
{
    byte[] full = Convert.FromBase64String(validHash);

    byte[] salt = full.Take(SALT_BYTE_SIZE).ToArray();

    byte[] hash = full.Skip(SALT_BYTE_SIZE).ToArray();

    byte[] testHash = PBKDF2(value, salt, PBKDF2_ITERATIONS, hash.Length);

    return SlowEquals(hash, testHash);
}

here you should replace hash.Length with HASH_BYTE_SIZE as the last parameter of the call to PBKDF2().

private bool SlowEquals(byte[] a, byte[] b)
{
    uint diff = (uint)a.Length ^ (uint)b.Length;

    for (int i = 0; i < a.Length && i < b.Length; i++)
        diff |= (uint)(a[i] ^ b[i]);

    return (diff == 0);
}

You should always use braces {} although they might be optional. Using braces makes your code less error prone which should have top priority for security related stuff.

private byte[] PBKDF2(string value, byte[] salt, int iterations, int outputBytes)
{
    Rfc2898DeriveBytes pbkdf2 = new Rfc2898DeriveBytes(value, salt);

    pbkdf2.IterationCount = iterations;

    return pbkdf2.GetBytes(outputBytes);
}

Rfc2898DeriveBytes is implementing IDispoable through its base class too, hence you should use a using block.

private byte[] PBKDF2(string value, byte[] salt, int iterations, int outputBytes)
{
    using (Rfc2898DeriveBytes pbkdf2 = new Rfc2898DeriveBytes(value, salt))
    {
        pbkdf2.IterationCount = iterations;

        return pbkdf2.GetBytes(outputBytes);
    }
}

StackExchange Code Review Q#109134, answer score: 3