0
\$\begingroup\$

Summary

I have finished creating a secure login functionality. I have used 1 resource to help me with the security. The security section was copy and pasted but implemented into a class to suit it in my own way, but all the logic and properties, commands etc.. is all my work. https://medium.com/@mehanix/lets-talk-security-salted-password-hashing-in-c-5460be5c3aae

Would appreciate any helpful review to my current code I have, from the community in areas where I can improve.

Current Code

SecurePasswordHasher.cs

At the moment I do not use public static string Hash(string password) anywhere, the focus is at the verification.

public static class SecurePasswordHasher
{

    public static string Hash(string password)
    {
        // Create salt
        byte[] salt;
        new RNGCryptoServiceProvider().GetBytes(salt = new byte[16]);

        // Create hash
        var pbkdf2 = new Rfc2898DeriveBytes(password, salt, 10000);

        byte[] hash = pbkdf2.GetBytes(20);
        byte[] hashBytes = new byte[36];
        Array.Copy(salt, 0, hashBytes, 0, 16);
        Array.Copy(hash, 0, hashBytes, 16, 20);

        return Convert.ToBase64String(hashBytes);
    }

    public static bool Verify(string savedPassword, string givenPassword)
    {
        byte[] hashBytes = Convert.FromBase64String(savedPassword);
        byte[] salt = new byte[16];
        Array.Copy(hashBytes, 0, salt, 0, 16);
        var pbkdf2 = new Rfc2898DeriveBytes(givenPassword, salt, 10000);
        byte[] hash = pbkdf2.GetBytes(20);

        int ok = 1;
        for (int i = 0; i < 20; i++)
        {
            if (hashBytes[i + 16] != hash[i])
            {
                ok = 0;
            }
        }

        if (ok == 1)
        {
            return true;
        }
        else
        {   
            return false;
        }

    }
}

LoginViewModel

public class LoginViewModel : BaseViewModel
{
   
    // User Properties
    private UserModel _User;
    public UserModel User
    {
        get { return _User; }
        set
        {
            _User = value;
            OnPropertyChanged(nameof(User));
        }
    }

    private string _FirstName;
    public string FirstName
    {
        get { return _FirstName; }
        set
        {
            _FirstName = value;
            User = new UserModel
            {
                FirstName = _FirstName,
                LastName = this.LastName,
                Password = this.Password
            };
            OnPropertyChanged(nameof(FirstName));
        }
    }

    private string _LastName;
    public string LastName
    {
        get { return _LastName; }
        set
        {
            _LastName = value;
            User = new UserModel
            {
                LastName = _LastName,
                FirstName = this.FirstName,
                Password = this.Password
            };
            OnPropertyChanged(nameof(LastName));
        }
    }


    private string _Password;
    public string Password
    {
        get { return _Password; }
        set
        {
            _Password = value;
            User = new UserModel
            {
                FirstName = this.FirstName,
                LastName = this.LastName,
                Password = _Password
            };
            OnPropertyChanged(nameof(Password));
        }
    }

    // Login Command
    public ICommand LoginCommand { get; set; }
    
    // Function
    private bool LoginFunction(object param)
    {
        // Get user credentials
        UserModel user = param as UserModel;
    
    
        // Veryify credentials have data to work with
        // Also, reason this method return a bool, because it prevents NullReferenceException.
        if (user == null)
        {
            MessageBox.ShowMessageBox("Credentials not specified");
            return false;
        }
        else if (user.FirstName == null || string.IsNullOrEmpty(user.FirstName))
        {
            MessageBox.ShowMessageBox("Firstname cannot be empty");
            return false;
        }
        else if (user.LastName == null || string.IsNullOrEmpty(user.LastName))
        {
            MessageBox.ShowMessageBox("Surname field cannot be empty");
            return false;
        }
        else if (user.Password == null || string.IsNullOrEmpty(user.Password))
        {
            MessageBox.ShowMessageBox("Password field cannot be empty");
            return false;
        }
        // Find Firstname and Surename in the database
        else
        {
            using (var conn = new MySqlConnection(ConnectionString.ConnString))
            {
                conn.Open();
    
                string query = @"SELECT
                                    *
                                    FROM USERS u
                                JOIN DEPARTMENT d
                                on d.id = u.departmentid
                                JOIN usergroup ug
                                ON ug.id = u.UserGroupID
                                WHERE FirstName = @Firstname AND Lastname = @LastName";
    
                var userDetails = conn.Query<UserModel>(query, new { FirstName = user.FirstName, LastName = user.LastName}).ToList();
    
                // If user is found, proceed to verification
                if(userDetails.Count() == 1)
                {
                    // Get password from the user
                    string savedPasswordHash = userDetails.First().Password;
    
                    bool verification = SecurePasswordHasher.Verify(savedPasswordHash, Password);
    
                    if (verification == true)
                    {
                        // Store Username
                        UserData.FullName = $"{user.FirstName} {user.LastName}";
    
                        ShowDashboard.ShowDashboard();
                        return true;
                    }
                    // Password is incorrect
                    else
                    {
                        MessageBox.ShowMessageBox("User not found");
                        return false;
                    }
                }
                // User with the given Firstname and surename is not found
                else
                {
                    MessageBox.ShowMessageBox("User not found");
                    return false;
                }
            }
        }
    }

    // Messagebox Interface
    public IMessageBoxService MessageBox { get; set; }

    // Open Dashboard Interface
    public IShowDashboardService ShowDashboard { get; set; }



    public LoginViewModel(IMessageBoxService messageBox, IShowDashboardService showDashboard)
    {
        this.MessageBox = messageBox;
        this.ShowDashboard = showDashboard;
        LoginCommand = new RelayCommand(param => LoginFunction(param));
    }
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ If Most of it was just copy and paste then what should be reviewed? \$\endgroup\$ Commented Mar 11, 2021 at 12:24
  • \$\begingroup\$ @BCdotWEB Altered. \$\endgroup\$ Commented Mar 11, 2021 at 14:55

1 Answer 1

1
\$\begingroup\$

Storing password in string is totally insecure.

Because string is immutable object and can be kept in memory for undefined period of time. This makes easy to get clean password through not complicated reverse engineering operation.

By the way

The focus is at the verification.

Ok

Renamed savedPassword because it's a hash not clean password.

public static bool Verify(string savedPasswordHash, string givenPassword)
{
    byte[] hashBytes = Convert.FromBase64String(savedPasswordHash);
    byte[] salt = hashBytes.Take(16).ToArray();
    byte[] hash = new Rfc2898DeriveBytes(givenPassword, salt, 10000).GetBytes(20);
  
    return hashBytes.Skip(16).SequenceEqual(hash);
}

That's it. Easy Linq query.

\$\endgroup\$
4
  • \$\begingroup\$ @LV98 Here's my detailed Russian StackOverflow answer how to securely use PasswordBox in WPF. \$\endgroup\$
    – aepot
    Commented Mar 11, 2021 at 16:56
  • \$\begingroup\$ I'm just wondering.. is it worth to have the security if the application is only used internally. \$\endgroup\$ Commented Mar 11, 2021 at 17:03
  • \$\begingroup\$ @LV98 at least it's worth to learn security. \$\endgroup\$
    – aepot
    Commented Mar 11, 2021 at 17:04
  • \$\begingroup\$ Agreed.. Any suggestions where I can start? \$\endgroup\$ Commented Mar 11, 2021 at 18:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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