3
\$\begingroup\$

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

}
\$\endgroup\$
3
\$\begingroup\$

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().

\$\endgroup\$
  • \$\begingroup\$ Can you elaborate a bit more please? \$\endgroup\$ – David Carrigan Nov 2 '15 at 17:17
  • \$\begingroup\$ I guess I just don't get it.. (the middle one at least) The first is because I don't have proper and/or full equal implements, the last seems to be a bug? A String "Test" will match any ProtectedString set to equal "Test" \$\endgroup\$ – David Carrigan Nov 2 '15 at 17:49
  • \$\begingroup\$ Only a perfect hash doesn't have collisions. So there are an unlimited number of different strings equal to the same hash-object. \$\endgroup\$ – Deduplicator Nov 2 '15 at 17:52
  • 1
    \$\begingroup\$ I get it now...crap. \$\endgroup\$ – David Carrigan Nov 2 '15 at 17:54
  • \$\begingroup\$ How can I get around this issue? I mean you do see what I'm trying to get at with this right? \$\endgroup\$ – David Carrigan Nov 2 '15 at 18:28
3
\$\begingroup\$
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);
    }
}
\$\endgroup\$
  • \$\begingroup\$ Excellent points. I have a question about two of them though... If I remove _Data = ""; I get the error that I cannot call this before all fields are assigned to. Secondly I include public override bool Equals(object obj) because the IDE warns me I define both == and != operators but do not override Object.Equals...etc. Also I apologize for my lack of alias use...my heavy VB background has me on that syntax highlight and capitalized word look and feel. \$\endgroup\$ – David Carrigan Nov 2 '15 at 16:44
  • \$\begingroup\$ It's actually _Data = this.CreateHash(value); Sorry I wrote that sort of wrong to. It's warning I cannot use 'this' object until all fields have been assigned. I suppose I could fix this by making my CreateHash methods etc static. But I was not wanting to make either externally accessible. \$\endgroup\$ – David Carrigan Nov 2 '15 at 16:50
  • \$\begingroup\$ OK so since I provided a way to compare against value (== and !=), the IDE is instructing me to provide a way to compare under the Equal method as to not fall back on the Object.Equals() because of my custom implements may not always provide the desired results? Also I cannot have instance field initializers statements in a structs. \$\endgroup\$ – David Carrigan Nov 2 '15 at 17:11
2
\$\begingroup\$

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");
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.