Generate and store passwords securely

Question

I wrote a password manager with the goal of being able to recover your passwords without connecting to an external resource. I used Electrum's recoverable bitcoin wallet for inspiration. When you create a profile in the password manager, a random 12 word phrase is generated, hashed, encrypted with the profile password and saved in the database. When you log in to a profile, the phrase hash is decrypted and saved in memory. When you add an account, you give the name of the website the account is for (service) and your username. The password is a hash of service+username with phrase hash as seed. No password is actually saved unless you want to use your own, in which case it is encrypted with the phrase hash. When you go to request the password for an account the program checks if there is a password saved (indicating you used your own password). If there is a password saved it is decrypted and given to the user, otherwise it is generated again with the phrase hash. If you lose the password manager or accidentally delete a profile you only need to know the phrase and the service+username of each account to recover your passwords. Passwords that were input by the user of course can not be recovered. I want to know if my method of generating passwords and retrieving them is secure.

Database.cs

    public class Database
    {
        public class DBProfile
        {
            public DBProfile(string name, string phrasehash)
            {
                Name = name;
                EncryptedPhraseHash = phrasehash;
                Accounts = new List<Account>();
            }

            [JsonProperty("Name")]
            public string Name { get; set; }

            [JsonProperty("EncryptedPhraseHash")] 
            public string EncryptedPhraseHash { get; set; }

            [JsonProperty("Accounts")]
            public List<Account> Accounts { get; set; }
        }

        public class Account
        {
            public Account(string servicename, string username, string encryptedpassword)
            {
                ServiceName = servicename;
                Username = username;
                EncryptedPassword = encryptedpassword;
            }

            [JsonProperty("ServiceName")]
            public string ServiceName { get; set; }

            [JsonProperty("Username")]
            public string Username { get; set; }

            [JsonProperty("EncryptedPassword")]
            public string EncryptedPassword { get; set; }
        }

        public class PhraseHashJson
        {
            public PhraseHashJson(string phrasehash)
            {
                PhraseHash = phrasehash;
            }

            [JsonProperty("PhraseHash")]
            public string PhraseHash { get; set; }
        }

        public static bool IsProfile(string name)
        {
            return File.Exists(name + ".mpr");
        }

        public static DBProfile CreateProfile(string profilename, string profilepassword, string phrase)
        {
            var passhash = Crypto.GenerateHash(profilepassword);
            var phrasehash = Crypto.GenerateHash(phrase);

            var json = new PhraseHashJson(phrasehash);
            var encryptedphrasehash = Crypto.EncryptStringAES(JsonConvert.SerializeObject(json), passhash);

            var newProfile = new DBProfile(profilename, encryptedphrasehash); 
            var text = Crypto.EncryptStringAES(JsonConvert.SerializeObject(newProfile), "b_@_51C-$33d");

            File.WriteAllText(profilename + ".mpr", text);
            return newProfile;
        }

        public static void SaveProfile(DBProfile dbProfile)
        {
            var text = Crypto.EncryptStringAES(JsonConvert.SerializeObject(dbProfile), "b_@_51C-$33d");

            File.WriteAllText(dbProfile.Name + ".mpr", text);
        }

        public static void DeleteProfile(string profilename)
        {
            File.Delete(profilename + ".mpr");
        }

        public static DBProfile GetProfile(string profilename)
        {
            var encryptedProfile = File.ReadAllText(profilename + ".mpr");
            var json = Crypto.DecryptStringAES(encryptedProfile, "b_@_51C-$33d");

            return JsonConvert.DeserializeObject<DBProfile>(json);
        }

        public static DBProfile GetProfileByPath(string path)
        {
            var encryptedProfile = File.ReadAllText(path);
            var json = Crypto.DecryptStringAES(encryptedProfile, "b_@_51C-$33d");

            return JsonConvert.DeserializeObject<DBProfile>(json);
        }

        public static List<DBProfile> GetProfiles()
        {
            var paths = Directory.GetFiles(Directory.GetCurrentDirectory(), "*.mpr");
            var profiles = paths.Select(GetProfileByPath).ToList();

            return profiles;
        }

        public static string GeneratePhrase()
        {
            byte[] data = new byte[4];
            var lphrase = new List<string>();
            var wordlist = Properties.Resources.wordlist.Split(new[] { Environment.NewLine }, StringSplitOptions.RemoveEmptyEntries).ToList();
            RNGCryptoServiceProvider rngCsp = new RNGCryptoServiceProvider();
            for (var i = 0; i < 12; i++)
            {
                rngCsp.GetBytes(data);
                int randomNum = BitConverter.ToInt32(data, 0);
                int place = Mod(randomNum, wordlist.Count); 
                lphrase.Add(wordlist[place]);
            }

            return string.Join(" ", lphrase);
        }

        public static int Mod(int x, int m)
        {
            int r = x % m;
            return r < 0 ? r + m : r;
        }
    }

Crypto.cs-- Encryption class I found on StackOverflow and modified a bit

public class Crypto
    {
        private static readonly byte[] Salt = Encoding.ASCII.GetBytes("P&s$w0<rd__>_^*_M6n2g#r"); //todo: do something about this

        public static string GenerateHash(string plaintext)
        {
            var plainText = Encoding.UTF8.GetBytes(plaintext);
            HashAlgorithm algorithm = new SHA256Managed();

            var plainTextWithSaltBytes = new byte[plainText.Length + Salt.Length];

            for (var i = 0; i < plainText.Length; i++)
                plainTextWithSaltBytes[i] = plainText[i];

            for (var i = 0; i < Salt.Length; i++)
                plainTextWithSaltBytes[plainText.Length + i] = Salt[i];

            return Convert.ToBase64String(algorithm.ComputeHash(plainTextWithSaltBytes));
        }
        public static string GenerateHashWithSeed(string plaintext, string salt)
        {
            HashAlgorithm algorithm = new SHA256Managed();

            var plainText = Encoding.UTF8.GetBytes(plaintext);
            var saltBytes = Encoding.UTF8.GetBytes(salt);
            var plainTextWithSaltBytes = new byte[plainText.Length + saltBytes.Length];

            for (var i = 0; i < plainText.Length; i++)
                plainTextWithSaltBytes[i] = plainText[i];

            for (var i = 0; i < saltBytes.Length; i++)
                plainTextWithSaltBytes[plainText.Length + i] = saltBytes[i];

            return Convert.ToBase64String(algorithm.ComputeHash(plainTextWithSaltBytes));
        }
        /// <summary>
        /// Encrypt the given string using AES.  The string can be decrypted using 
        /// DecryptStringAES().  The sharedSecret parameters must match.
        /// </summary>
        /// <param name="plainText">The text to encrypt.</param>
        /// <param name="sharedSecret">A password used to generate a key for encryption.</param>
        public static string EncryptStringAES(string plainText, string sharedSecret)
        {
            if (string.IsNullOrEmpty(plainText))
                throw new ArgumentNullException("plainText");
            if (string.IsNullOrEmpty(sharedSecret))
                throw new ArgumentNullException("sharedSecret");

            string outStr; // encrypted string to return

            try
            {
                // generate the key from the shared secret and the salt
                var key = new Rfc2898DeriveBytes(sharedSecret, Salt);

                // Create a RijndaelManaged object
                var aesAlg = new RijndaelManaged();
                aesAlg.Key = key.GetBytes(aesAlg.KeySize/8);

                // Create a decryptor to perform the stream transform.
                ICryptoTransform encryptor = aesAlg.CreateEncryptor(aesAlg.Key, aesAlg.IV);

                // Create the streams used for encryption.
                using (var msEncrypt = new MemoryStream())
                {
                    // prepend the IV
                    msEncrypt.Write(BitConverter.GetBytes(aesAlg.IV.Length), 0, sizeof (int));
                    msEncrypt.Write(aesAlg.IV, 0, aesAlg.IV.Length);

                    using (var csEncrypt = new CryptoStream(msEncrypt, encryptor, CryptoStreamMode.Write))
                        using (var swEncrypt = new StreamWriter(csEncrypt))
                            swEncrypt.Write(plainText); //Write all data to the stream.

                    outStr = Convert.ToBase64String(msEncrypt.ToArray());
                }

                aesAlg.Clear();
            }
            catch (Exception ex)
            {
                Console.WriteLine("Error: {0}", ex);
                return "";
            }

            // Return the encrypted bytes from the memory stream.
            return outStr;
        }

        /// <summary>
        /// Decrypt the given string.  Assumes the string was encrypted using 
        /// EncryptStringAES(), using an identical sharedSecret.
        /// </summary>
        /// <param name="cipherText">The text to decrypt.</param>
        /// <param name="sharedSecret">A password used to generate a key for decryption.</param>
        public static string DecryptStringAES(string cipherText, string sharedSecret)
        {
            if (string.IsNullOrEmpty(cipherText))
                throw new ArgumentNullException("cipherText");
            if (string.IsNullOrEmpty(sharedSecret))
                throw new ArgumentNullException("sharedSecret");

            // Declare the string used to hold
            // the decrypted text.
            string plaintext;

            try
            {
                // generate the key from the shared secret and the salt
                var key = new Rfc2898DeriveBytes(sharedSecret, Salt);

                // Create the streams used for decryption.                
                var bytes = Convert.FromBase64String(cipherText);

                using (var msDecrypt = new MemoryStream(bytes))
                {
                    var aesAlg = new RijndaelManaged(); // Create a RijndaelManaged object with the specified key and IV.
                    aesAlg.Key = key.GetBytes(aesAlg.KeySize / 8);

                    // Get the initialization vector from the encrypted stream
                    aesAlg.IV = ReadByteArray(msDecrypt);

                    // Create a decrytor to perform the stream transform.
                    var decryptor = aesAlg.CreateDecryptor(aesAlg.Key, aesAlg.IV);

                    using (var csDecrypt = new CryptoStream(msDecrypt, decryptor, CryptoStreamMode.Read))
                        using (var srDecrypt = new StreamReader(csDecrypt))
                            plaintext = srDecrypt.ReadToEnd(); // Read the decrypted bytes from the decrypting stream and place them in a string.

                    aesAlg.Clear();
                }
            }
            catch (Exception ex)
            {
                Console.WriteLine("Error: {0}", ex);
                return "";
            }

            return plaintext;
        }

        private static byte[] ReadByteArray(Stream s)
        {
            var rawLength = new byte[sizeof(int)];

            if (s.Read(rawLength, 0, rawLength.Length) != rawLength.Length)
                throw new SystemException("Stream did not contain properly formatted byte array");

            var buffer = new byte[BitConverter.ToInt32(rawLength, 0)];

            if (s.Read(buffer, 0, buffer.Length) != buffer.Length)
                throw new SystemException("Did not read byte array properly");

            return buffer;
        }
    }

Profile.cs

public class Profile
    {
        public string ProfileName;
        public bool LoggedIn = false;
        private Database.DBProfile _dbProfile;
        private string _phraseHash;

        public Profile(Database.DBProfile dbProfile)
        {
            _dbProfile = dbProfile;
            ProfileName = _dbProfile.Name;
        }

        public Database.Account AddAccount(string servicename, string username, string password)
        {
            // If password was generated by the program we don't need to save it, just the variables we can use to regenerate it
            var encryptedpassword = (password == null) ? null : Crypto.EncryptStringAES(password, _phraseHash); 

            var newAccount = new Database.Account(servicename, username, encryptedpassword); // Create account object

            // Make sure profile does not already have an account with that name
            if (_dbProfile.Accounts.Any(a => (a.Username == username) && (a.ServiceName == servicename))) return null;
            // Add account to profile
            _dbProfile.Accounts.Add(newAccount); // Add account to profile objects list
            Database.SaveProfile(_dbProfile); // Save account in database
            return newAccount;
        }

        public bool Login(string password)
        {
            var passwordHash = Crypto.GenerateHash(password); // Get password hash
            try
            {
                var encryptedphrasehash = Crypto.DecryptStringAES(_dbProfile.EncryptedPhraseHash, passwordHash); // Try to decrypt phrase with given password, throws if invalid
                _phraseHash = JsonConvert.DeserializeObject<Database.PhraseHashJson>(encryptedphrasehash).PhraseHash; // Get phrase hash from decrypted profile
                LoggedIn = true;
                return true;
            }
            catch (Exception) // Wrong password
            {
                return false;
            }
        }

        public void Logout()
        {
            RNGCryptoServiceProvider rngCsp = new RNGCryptoServiceProvider();
            for (var i = 0; i < 3; i++)
            {
                byte[] data = new byte[(int) Math.Round((double) (_phraseHash.Count()))];
                rngCsp.GetBytes(data);
                int randomNum = BitConverter.ToInt32(data, 0);
                _phraseHash = randomNum.ToString();
            }
            LoggedIn = false;
            _phraseHash = null;
        }

        public string GetAccountPassword(int accountIndex)
        {
            Database.Account account = _dbProfile.Accounts[accountIndex];
            // If encrypted password is null the password was generated with the program so we just re-generate it
            // If it isn't null the password was given by the user so we need to decrypt it
            return (account.EncryptedPassword == null) 
                ? Crypto.GenerateHashWithSeed(account.ServiceName + account.Username, _phraseHash)
                : Crypto.DecryptStringAES(account.EncryptedPassword, _phraseHash); 
        }

        public void Delete()
        {
            if (LoggedIn) Logout(); // Erase phrase hash 
            Database.DeleteProfile(ProfileName);
        }

        public List<Database.Account> GetAccounts()
        {
            return _dbProfile.Accounts;
        }

        public void DeleteAccount(int index)
        {
            var accountRemove = _dbProfile.Accounts[index];
            _dbProfile.Accounts.Remove(accountRemove);
            Database.SaveProfile(_dbProfile);
        }
    }

Adriano Repetti · Accepted Answer · 2016-09-15 13:33:39Z

Security Issue

If I have these these two accounts (names are obviously weird just to illustrate the problem):

1) service: hotmail, username: user.

2) service: hot, username: mailuser.

Your code will generate the same hash because you just concatenate strings (service name + user name). It's not a terrible problem but if an attacker knows an user is using your tool then with little social engineering (to encourage him to register to his free great service) he may get the user's Hotmail password when user registers to his fake site (if he won't let the user choose a free username then they will match.)

Customization

You store your dictionary inside resources. Resources are localized and it's a good thing however it may be hard for translators to handle such format (newline to separate items.)

Note that most tools export resources to CSV files usually edited with Microsoft Excel. It's a point to discuss further with them but I'd at least consider the opportunity to simply include a text file (deployed in the same directory of your localized assemblies.) This will also give the opportunity to your end-users to add/change their dictionary (or to use a custom one.)

Database.cs

You declared Database class as a public instantiable class however it contains only static methods and nested classes. Mark it as static to disallow creation of object of that type.

Class DBProfile is not extended anywhere (and I don't see any extension point unless you just want to add properties in derived classes). Mark it as sealed (same for every class in your code, pick the habit to have sealed classes by default.) Also DBProfile should be DbProfile.

You do not need to use JsonPropertyAttribute if you re-declare the same name of the property, just drop it.

Are DBProfile properties read-only? In that case make the setter private (or, if you're using C# 6 just drop it.)

DBProfile and Account (and the others) classes are public, same for their constructor. If you have a public constructor you may/should validate arguments, if it's used only internally then make it internal instead of public.

IsProfile() method is little bit too vague (IMO). If you expect a full file name then make it clear (both in argument and method names) otherwise you're assuming a specific location for current folder.

You're not handling any exception. Things may go wrong (especially for I/O) but often to wait little bit and retry may solve file is in use conflicts (think if - by case - an user runs your application twice.)

You have some hardcoded strings, drop them and use static readonly string fields. If you will ever change profile file extension or password for encryption...

In GetProfiles() you're using Directory.GetCurrentDirectory(). Isn't, usually, better to search data from a known location? User's data folder, user's documents or application's startup path? Do you really have an usage scenario where you set profiles path using working directory?

In GeneratePhrase() you split a string from resources (IMO no need to specify StringSplitOptions.RemoveEmptyEntries) and you convert a string[] to List<string>. No need to do it, you use it with index and an array is the perfect structure for that (also you may want to do not perform splitting for every call to GeneratePhrase().)

You do not need to store randomly picked words in lphrase (to later build a string with String.Join), you can directly use a StringBuilder and directly append there.

All your methods are public. Do you really need them all from outside? If that's the case maybe you should change little bit your design to provide more encapsulation...

Crypto.cs

Your class should be static.

In CreateEncryptor you create many disposable objects (for example encryptor) but you do not dispose them. Always dispose objects which implement IDisposable.

You do not need to temporary store result in outStr, with using where appropriate you can directly return the result (and you code will also be less indented).

You're catching Exception. Do not do it (many reasons, just search about it). Catch what you know you can handle and ignore the rest. Also you're logging to console...hmmmm I doubt this will be a console application...

ReadByteArray() is throwing SystemException, you shouldn't throw that exception (original intended usage has been abandoned because useless and full of inconsistencies but still it must be considered a base exception class like Exception.) You may consider to throw InvalidDataException, for example.

Profiles.cs

Class should be (?) sealed and you should validate constructor's parameters.

_dbProfile should be readonly. ProfileName isn't needed, it may be a shortcut private property for _dbProfile.Name or just dropped.

Each time you added a comment I'd remove it to refactor out a method, from:

public string GetAccountPassword(int accountIndex)
{
        Database.Account account = _dbProfile.Accounts[accountIndex];
        // If encrypted password is null the password was generated with the program so we just re-generate it
        // If it isn't null the password was given by the user so we need to decrypt it
        return (account.EncryptedPassword == null) 
            ? Crypto.GenerateHashWithSeed(account.ServiceName + account.Username, _phraseHash)
            : Crypto.DecryptStringAES(account.EncryptedPassword, _phraseHash); 
}

To:

public string GetAccountPassword(int accountIndex)
{
    var account = _dbProfile.Accounts[accountIndex];

    if (account.HasUserDefinedPassword)
        return Crypto.DecryptStringAES(account.EncryptedPassword, _phraseHash);

    return Crypto.GenerateHashWithSeed(account.FullProfileName, _phraseHash);
}

Also I'd consider to make some methods private, I do not see calling code but accountIndex is smelly. DbProfile.FullProfileName simply concatenates ServiceName and Username as in your original code), if you will change this logic you will then have just one point to modify.

Stack Exchange Network

Generate and store passwords securely

1 Answer 1

Security Issue

Customization

Database.cs

Crypto.cs

Profiles.cs

You must log in to answer this question.

Hot Network Questions

Generate and store passwords securely

1 Answer 1

Security Issue

Customization

Database.cs

Crypto.cs

Profiles.cs

You must log in to answer this question.

Related

Hot Network Questions