0

I have made a login system for my application, however, it is working extremely inconsistently.

Sometimes the password will work, but other times it says it's incorrect. I'm 100% sure I'm typing it correctly.

To store the passwords, I generate a random salt, hash the password with the salt and store the hashed password and salt along with the username in the database.

To authenticate the user, I select the hashed password and salt based on the given username. I then hash their password attempt with the salt and see if it matches their original hashed password, allowing them to log in if so.

My code is as follows:

private const int NumberOfRounds = 5000, SaltLength = 32;
public static byte[] GenerateSalt()
{
    using (var rng = new RNGCryptoServiceProvider())
    {
        var randomNumber = new byte[SaltLength];

        rng.GetBytes(randomNumber);

        return randomNumber;
    }
}

public static byte[] HashPassword(byte[] passwordToHash, byte[] salt)
{
    using (var deriveBytes = new Rfc2898DeriveBytes(passwordToHash, salt, numberOfRounds))
    {
        return deriveBytes.GetBytes(32);
    }
}



public static bool IsPasswordValid(string inputPassword, string hashedPassword, string salt)
{
    byte[] potentialValidPassword = HashPassword(Encoding.Unicode.GetBytes(inputPassword),
                Encoding.Unicode.GetBytes(salt));

    string potentialAsString = Encoding.Unicode.GetString(potentialValidPassword);

    return  Encoding.Unicode.GetBytes(hashedPassword).SequenceEqual(potentialValidPassword) ||
                hashedPassword.Equals(potentialAsString);
}

The reason my check compares both the byte array value and the string value is that sometimes the byte value comparison fails but the string value works.

My code to insert a user into the database is as follows

public SecurityReturnMessage AddUser(string username, string password)
{
    byte[] salt = PasswordManagement.GenerateSalt();

    byte[] hashedPassword = PasswordManagement.HashPassword(Encoding.Unicode.GetBytes(password), salt);

    if (conn.State != ConnectionState.Open)
        conn.Open();

    int result;
    using (IDbCommand comm = conn.CreateCommand())
    {
        comm.CommandText = "usp_IFRS_SEC_USER_INSERT";
        comm.CommandType = CommandType.StoredProcedure;

        SqlParameter hashPwd =new SqlParameter("@hashpwd", SqlDbType.NVarChar)
        {
            Value = Encoding.Unicode.GetString(hashedPassword)
        };

         SqlParameter saltParameter = new SqlParameter("@salt", SqlDbType.NVarChar)
         {
             Value = Encoding.Unicode.GetString(salt)
         };

         comm.Parameters.Add(Extensions.CreateParameter(comm, "@user", username));
         comm.Parameters.Add(hashPwd);
         comm.Parameters.Add(saltParameter);

         var returnVal = comm.CreateParameter();
         returnVal.Direction = ParameterDirection.ReturnValue;
         comm.Parameters.Add(returnVal);

         comm.ExecuteNonQuery();

         result = (int)returnVal.Value;
     }

     if (conn.State != ConnectionState.Closed)
         conn.Close();

     return (SecurityReturnMessage)result;
}

If anyone could help me out with this I'd be extremely grateful.

Robert Woods
  • 350
  • 1
  • 6
  • 17
  • The main error is `Unicode.Get____()`. To prevent this, do _not_ write your own security code. ASP.NET provides the Identymanager, alas in many variations and versions. – H H Jan 19 '18 at 13:29
  • @HenkHolterman sure, I just assumed that meant to not implement the hashing algorithm myself, which I haven't. Do you have any good reading on IdentityManager? – Robert Woods Jan 19 '18 at 13:53

1 Answers1

2

I Suggest you to use the Base64 conversions as below when you convert values from Bytes to String and vice versa.

    string base64 = Convert.ToBase64String(bytes);
    byte[] bytes = Convert.FromBase64String(base64);

This way you can make sure that the unicode data is not lost or corrupted during either of the conversion process, which might be a valid reason for your code's inconsistency.

Ranjani
  • 73
  • 1
  • 10
  • Thanks for your reply, however I get format exceptions now. The messages are either "The input is not a valid Base-64 string as it contains a non-base 64 character, more than two padding characters, or an illegal character among the padding characters. " or "Invalid length for a Base-64 char array or string." – Robert Woods Jan 19 '18 at 13:15
  • Yes, this is the better approach but it will not automagically repair your passwords that are already stored. – H H Jan 19 '18 at 13:27
  • @HenkHolterman Sure, this is still in development so I truncated the user table and created new ones, however I get the exceptions described above – Robert Woods Jan 19 '18 at 13:54
  • Then you're not doing it right. Look for some sample code, plenty arounf also on SO. – H H Jan 19 '18 at 16:53