Skip to main content
added 560 characters in body
Source Link
Adriano Repetti
  • 10.6k
  • 1
  • 24
  • 48

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

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?

Profiles.cs

Profiles.cs

Database.cs

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?

Profiles.cs

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

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?

Profiles.cs

Source Link
Adriano Repetti
  • 10.6k
  • 1
  • 24
  • 48

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.)

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.