HiveBrain v1.2.0
Get Started
← Back to all entries
snippetcsharpMinor

Generate and store passwords securely

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
passwordsstoregenerateandsecurely

Problem

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();
}

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

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

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

public class Account
{
public Account(string servicename, string username, string encr

Solution

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. 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 = _dbProfil

Code Snippets

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 string GetAccountPassword(int accountIndex)
{
    var account = _dbProfile.Accounts[accountIndex];

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

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

Context

StackExchange Code Review Q#141412, answer score: 2

Revisions (0)

No revisions yet.