patterncsharpMinor
Password building and verification
Viewed 0 times
andverificationbuildingpassword
Problem
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:
The classes I've made are:
```
using System;
using System.Security;
using System.Security.Cryptography;
using System.Runtime.InteropServices;
namespace PasswordTest
{
///
/// A container object which stores a hash, it's salt and the amounth of iterations.
/// The underlying hashfunction is pbkdf2.
///
public sealed class Password : IDisposable
{
///
/// 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.
///
public static readonly int SALT_LENGHT = 64;
///
/// the number of iterations the hashfunction does.
/// rule of thumb, make it as big as possible whitout annoying your end-users.
///
public static readonly int ITERATIONS = 64000;
///
/// 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.
///
public static readonly int HASH_BYTE_COUNT = 20;
///
/// A computed hash
///
pub
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
{
///
/// A container object which stores a hash, it's salt and the amounth of iterations.
/// The underlying hashfunction is pbkdf2.
///
public sealed class Password : IDisposable
{
///
/// 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.
///
public static readonly int SALT_LENGHT = 64;
///
/// the number of iterations the hashfunction does.
/// rule of thumb, make it as big as possible whitout annoying your end-users.
///
public static readonly int ITERATIONS = 64000;
///
/// 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.
///
public static readonly int HASH_BYTE_COUNT = 20;
///
/// A computed hash
///
pub
Solution
///
/// 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.
///
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.
Code Snippets
/// <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;//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);private void match()
{
using (Rfc2898DeriveBytes pbkdf2 = new Rfc2898DeriveBytes(_matchPattern, _salt, _iterations))
{
_isMatch = pbkdf2.GetBytes(_hash.Length).SequenceEqual(_hash);
}
}Context
StackExchange Code Review Q#103987, answer score: 6
Revisions (0)
No revisions yet.