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

Question

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);
        return result;
    }

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

    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);
    }

    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);
    }

    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);
    }

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

        pbkdf2.IterationCount = iterations;

        return pbkdf2.GetBytes(outputBytes);
    }

}

Answer 1

Your notion of "equality" is very surprising:

Two of your ProtectedStrings can compare unequal, but equal to the same String.
Two Strings can compare unequal, but equal to the same ProtectedString.
Two Strings can compare equal to one ProtectedString, but only one of them compares equal to a second ProtectedString.

So, you thoroughly broke transitivity, which is a bad idea.

Aside from that, whenever you override .equals(object), you should also override GetHashCode().

Answer 2

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);
    }
}

Answer 3

One thing bothers me in this implementation. Using an implicit operator between two types of strings (plain vs protected) hides the fact the plain string gets converted to a cryptographic digest. As consumer of the API, I might not know such conversion takes place.

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

By using an explicit operator, the consumer gets a hint that a conversion might take place.

As Guidelines state:

The explicit conversions that are not implicit conversions are conversions that cannot be proven to always succeed, conversions that are known to possibly lose information, and conversions across domains of types sufficiently different to merit explicit notation.

I would argue plain and protected strings are sufficiently different to merit explicit notation.

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

And then call it like:

var ThisIsMyProtectedString1 = (ProtectedString)"Test";

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