Password building and verification

Question

I'm building a library for myself to use to store user passwords. The library itself shouldn't do the retrieval of data, but it should provide container classes for the data.

I would like to know what I've done wrong in it, what could be improved and what's concidered a bad practise with passwords on the way I've done it.

I'm using PBKDF2 as the hashing algorithm. The salt is randomly generated for every Password, but not tied to the PC itself in any way.

Reasons:

• I want the passwords to be portable. (Maybe not in a future extension.) The iterations I use is 64000 iterations, doubled every year from 2012 and on + a random number.
• I've read that most password brute force tools don't work well with multiple passwords with different iterations. (It won't stop people, but it will slow them down.)

The classes I've made are:

using System;
using System.Security;
using System.Security.Cryptography;
using System.Runtime.InteropServices;

namespace PasswordTest
{
    /// <summary>
    /// A container object which stores a hash, it's salt and the amounth of iterations.
    /// The underlying hashfunction is pbkdf2.
    /// </summary>
    public sealed class Password : IDisposable
    {
        /// <summary>
        /// the number of bytes the salt is in lenght. make sure that this is bigger than what a graphics card can handle easily to be safe.
        /// </summary>
        public static readonly int SALT_LENGHT = 64;

        /// <summary>
        /// the number of iterations the hashfunction does.
        /// rule of thumb, make it as big as possible whitout annoying your end-users.
        /// </summary>
        public static readonly int ITERATIONS = 64000;

        /// <summary>
        /// The amounth of bytes the hash exists of, again. as big as possible.
        /// but pbkdf2 has a problem that after 20 bytes it doesn't cost as much cpu power for the hacker as it does for the user.
        /// </summary>
        public static readonly int HASH_BYTE_COUNT = 20;

        /// <summary>
        /// A computed hash
        /// </summary>
        public byte[] Hash { get; private set; }  //should be concidered secret, although it's encrypted. thus fairly safe if it leaks.

        /// <summary>
        /// The accompanying salt of the hash.
        /// </summary>
        //public byte[] Salt { get; private set; }  //doesn't have to be concidered secret but it can't hurt

        public byte[] Salt { get; private set; }

        /// <summary>
        /// The amounth of iterations the internal hashfunction has to do.
        /// </summary>
        public int Iterations { get; private set; }

        /// <summary>
        /// Makes a Password from old password data.
        /// </summary>
        /// <param name="hash">The already hashed password.</param>
        /// <param name="salt">The salt.</param>
        /// <param name="iterations">How much iterations this password got.</param>
        public Password(byte[] hash, byte[] salt, int iterations)
        {
            Hash = hash;
            Salt = salt;
            Iterations = iterations;
        }

        /// <summary>
        /// Makes a Password
        /// </summary>
        /// <param name="password">The byte[] representing the password.</param>
        /// <param name="leaveOpen">if no, the byte[] will be disposed after use.</param>
        public Password(byte[] password, bool leaveOpen)
        {
            setIterations();
            gen(password, leaveOpen);
        }

        /// <summary>
        /// Makes a Password from a SecureString
        /// </summary>
        /// <param name="passwordString">The SecureString representing a password.</param>
        /// <param name="leaveOpen">If no, the SecureString will be disposed after use.</param>
        public Password(SecureString passwordString, bool leaveOpen)
        {
            int length = passwordString.Length;
            char[] chars = new char[length];
            IntPtr pointer = IntPtr.Zero;

            try
            {
                pointer = Marshal.SecureStringToBSTR(passwordString);
                Marshal.Copy(pointer, chars, 0, length);
            }
            finally
            {
                if (pointer != IntPtr.Zero)
                {
                    Marshal.ZeroFreeBSTR(pointer);
                }
                if (!leaveOpen)
                {
                    passwordString.Dispose();
                }
            }

            byte[] bytes = new byte[chars.Length];
            Buffer.BlockCopy(chars, 0, bytes, 0, bytes.Length);
            for(int i = 0; i < chars.Length; i++)
            {
                chars[i] = '0';
            }

            setIterations();

            this.gen(bytes, false);
        }

        /// <summary>
        /// Unsafe. Use a SecureString.
        /// </summary>
        /// <param name="passwordString"></param>
        public Password(string passwordString)
        {
            setIterations();
            byte[] bytes = new byte[passwordString.Length * sizeof(char)];
            Buffer.BlockCopy(passwordString.ToCharArray(), 0, bytes, 0, bytes.Length);
            gen(bytes, true);
        }

        /// <summary>
        /// Generates the password, including the hash.
        /// </summary>
        /// <param name="password">The data to generate the password from.</param>
        /// <param name="leaveOpen">If no, the SecureString will be disposed after use.</param>
        private void gen(byte[] password, bool leaveOpen)
        {
            Salt = new byte[Password.SALT_LENGHT];

            //generating salt
            using (RNGCryptoServiceProvider rngCsp = new RNGCryptoServiceProvider())
            {
                rngCsp.GetBytes(Salt);
            }

            //generating hash
            using (Rfc2898DeriveBytes pbkdf2 = new Rfc2898DeriveBytes(password, Salt, Iterations))
            {
                Hash = pbkdf2.GetBytes(HASH_BYTE_COUNT);
            }

            if (!leaveOpen)
            {
                for (int i = 0; i < password.Length; i++)
                {
                    password[i] = 0;
                }
            }
        }

        public override string ToString()
        {
            return BitConverter.ToString(Hash) + ' ' + BitConverter.ToString(Salt) + ' ' + Iterations;
        }

        public void Dispose()
        {
            Hash = null;
            Salt = null;
            Iterations = 0;
        }

        public void setIterations()
        {
            //double the amounth of iterations every year, starting at 2012.
            Iterations = ITERATIONS * (int)Math.Ceiling(Math.Pow(2, ((DateTime.UtcNow - new DateTime(2012, 1, 1)).TotalDays / 365.25 / 2.0)));
            //adding a little more randomness to it, makes it hard for graphics cards to optimize some stuff. it looks not mutch, but it does a thing.
            Iterations += new Random().Next(2000);

        }
    }
}

and

using System;
using System.Linq;
using System.Runtime.InteropServices;
using System.Security;
using System.Security.Cryptography;

namespace PasswordTest
{
    class PasswordMatcher : IDisposable
    {
        private byte[] _hash;
        public byte[] _matchPattern;
        private byte[] _salt;
        private int _iterations;

        private bool? _isMatch;
        public bool LeaveOpen { get; private set; }

        /// <summary>
        /// true if the password and the pattern are a match
        /// </summary>
        public bool IsMatch
        {
            get
            {
                if (_isMatch == null)
                {
                    match();
                }
                return (bool)_isMatch;
            }
        }

        public PasswordMatcher(Password password, SecureString matchPattern, bool leaveOpen)
        {
            _hash = password.Hash;
            _salt = password.Salt;
            _iterations = password.Iterations;

            int length = matchPattern.Length;
            char[] chars = new char[length];
            IntPtr pointer = IntPtr.Zero;

            try
            {
                pointer = Marshal.SecureStringToBSTR(matchPattern);
                Marshal.Copy(pointer, chars, 0, length);
            }
            finally
            {
                if (pointer != IntPtr.Zero)
                {
                    Marshal.ZeroFreeBSTR(pointer);
                }
                if (!leaveOpen)
                {
                    matchPattern.Dispose();
                }
            }

            Buffer.BlockCopy(chars, 0, _matchPattern, 0, chars.Length);
            for (int i = 0; i < chars.Length; i++)
            {
                chars[i] = '0';
            }

            LeaveOpen = leaveOpen;
        }

        public PasswordMatcher(byte[] hash, SecureString matchPattern, byte[] salt, int iterations, bool leaveOpen)
        {
            _hash = hash;

            int length = matchPattern.Length;
            char[] chars = new char[length];
            IntPtr pointer = IntPtr.Zero;

            try
            {
                pointer = Marshal.SecureStringToBSTR(matchPattern);
                Marshal.Copy(pointer, chars, 0, length);
            }
            finally
            {
                if (pointer != IntPtr.Zero)
                {
                    Marshal.ZeroFreeBSTR(pointer);
                }
                if (!leaveOpen)
                {
                    matchPattern.Dispose();
                }
            }

            Buffer.BlockCopy(chars, 0, _matchPattern, 0, chars.Length);
            for (int i = 0; i < chars.Length; i++)
            {
                chars[i] = '0';
            }

            _salt = salt;
            _iterations = iterations;
            LeaveOpen = leaveOpen;
        }


        public PasswordMatcher(Password password, string matchPattern, bool leaveOpen)
        {
            _hash = password.Hash;
            _salt = password.Salt;
            _iterations = password.Iterations;

            byte[] bytes = new byte[matchPattern.Length * sizeof(char)];
            Buffer.BlockCopy(matchPattern.ToCharArray(), 0, bytes, 0, bytes.Length);
            _matchPattern = bytes;
            LeaveOpen = leaveOpen;
        }

        public PasswordMatcher(Password password, byte[] matchPattern, bool leaveOpen)
        {
            _hash = password.Hash;
            _salt = password.Salt;
            _iterations = password.Iterations;
            _matchPattern = matchPattern;
            LeaveOpen = leaveOpen;
        }

        public PasswordMatcher(byte[] hash, byte[] matchPattern, byte[] salt, int iterations, bool leaveOpen)
        {
            _hash = hash;
            _matchPattern = matchPattern;
            _salt = salt;
            _iterations = iterations;
            LeaveOpen = leaveOpen;
        }

        public PasswordMatcher(byte[] hash, string matchPattern, byte[] salt, int iterations, bool leaveOpen)
        {
            _hash = hash;
            _salt = salt;
            _iterations = iterations;

            byte[] bytes = new byte[matchPattern.Length * sizeof(char)];
            Buffer.BlockCopy(matchPattern.ToCharArray(), 0, bytes, 0, bytes.Length);
            _matchPattern = bytes;
        }

        private void match()
        {
            using (Rfc2898DeriveBytes pbkdf2 = new Rfc2898DeriveBytes(_matchPattern, _salt, _iterations))
            {
                _isMatch = pbkdf2.GetBytes(_hash.Length).SequenceEqual(_hash);
            }
        }

        public void Dispose()
        {
            _hash = null;
            _matchPattern = null;
            _salt = null;
        }

        public void Dispose(bool dispose)
        {
            if (dispose)
            {
                Dispose();
            }
        }
    }
}

Answer 1

    /// <summary>
    /// the number of bytes the salt is in lenght. make sure that this is bigger than what a graphics card can handle easily to be safe.
    /// </summary>
    public static readonly int SALT_LENGHT = 64;

Any GPU limitation on salt lengths is small, unlikely to last, and will not necessarily apply to ASICs. I do not think it is a good idea to rely on it. The salt is only used on the first iteration of PBKDF2 so it might even be possible to precompute the first step with a CPU and leave the rest for a GPU.

It would be fine to halve the salt length, but since it only costs memory and a constant amount of work, it is not a problem to leave it as is.

Nit: You misspelled "length".

        //double the amounth of iterations every year, starting at 2012.
        Iterations = ITERATIONS * (int)Math.Ceiling(Math.Pow(2, ((DateTime.UtcNow - new DateTime(2012, 1, 1)).TotalDays / 365.25 / 2.0)));
        //adding a little more randomness to it, makes it hard for graphics cards to optimize some stuff. it looks not mutch, but it does a thing.
        Iterations += new Random().Next(2000);

Again, I do not think relying on this is a good idea, but it does not seem insecure either. Personally, I would rather make the number of iterations a smoother function of time, i.e. round after the multiply. You still get different iteration counts for different passwords, but you also avoid the abrupt stepping up every two years.

Also, you lack any code to upgrade the iteration count of password hashes as time goes by. That should probably happen somewhere in the PasswordTest class, e.g. if the iteration count is less than some fraction of the current.

    private void match()
    {
        using (Rfc2898DeriveBytes pbkdf2 = new Rfc2898DeriveBytes(_matchPattern, _salt, _iterations))
        {
            _isMatch = pbkdf2.GetBytes(_hash.Length).SequenceEqual(_hash);
        }
    }

My .NET knowledge is shaky, but as far as I know SequenceEqual is allowed to terminate early, at the first unequal element. If that is the case, it allows a timing attack. An online attacker who knows the salt and iteration count could test 256 passwords that give different first bytes in the hash, time which of them returns failure quickest, then move on to the next character. This turns a normally online attack into an effectively offline one.

You should use a constant time comparison here.