3
\$\begingroup\$

I want to start by just mentioning I'm a semi-beginner at C# and cyber security so any feedback is appreciated :)

What I think I need reviewing is the SQLite queries and maybe the hashing functions

I'm making a password manager project (sorry in advance for wall of code):

The supporting functions: (I probably don't need to worry about these)

using System.Data.SQLite;
using System.Security.Cryptography;
using Microsoft.AspNetCore.Cryptography.KeyDerivation;

//Just a function to check if the username/password inputted was valid format (you can probably ignore this)
private bool ValidLogin()
{
    string Message = "Enter a ";
    if (LoginTBUsername.Text == "")
    {
        Message += "username";
    }
    if (LoginTBPassword.Text == "")
    {
        if (LoginTBUsername.Text == "")
        {
            Message += " and a ";
        }
        Message += "password";
    }
    if (Message != "Enter a ")
    {
        LoginLabelError.Text = Message;
        LoginLabelError.Left = (LoginPanel.Width - LoginLabelError.Width) / 2;
        LoginLabelError.Visible = true;
        return false;
    }
    if (LoginTBPassword.Text.Length < 8)
    {
        LoginLabelError.Text = "Password must be 8 or more characters";
        LoginLabelError.Left = (LoginPanel.Width - LoginLabelError.Width) / 2;
        LoginLabelError.Visible = true;
        return false;
    }
    return true;
}

//Simple hash system
private string Hash(string Password, byte[] Salt, int Iteration = 12288)
{
    return Convert.ToBase64String(KeyDerivation.Pbkdf2(
        password: Password,
        salt: Salt,
        prf: KeyDerivationPrf.HMACSHA512,
        iterationCount: Iteration,
        numBytesRequested: 256 / 8));
}
//The login code
private void LoginButtonLog_Click(object sender, EventArgs e)
{
    if (!ValidLogin())
    {
        return;
    }
    
    //Gets the path to the folders im using
    string AppData = Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData);
    string MDBPath = AppData + "\\Password Manager\\Accounts.mdb";

    //Since database doesnt exist, no point continuing to login
    if (!File.Exists(MDBPath))
    {
        LoginLabelError.Text = "Account not found";
        LoginLabelError.Left = (LoginPanel.Width - LoginLabelError.Width) / 2;
        LoginLabelError.Visible = true;
        return;
    }

    string Username = LoginTBUsername.Text;
    string Password = LoginTBPassword.Text;

    //Heres where i need feedback
    string Command;
    using (SQLiteConnection SQLite = new SQLiteConnection($"Data Source={MDBPath};Version=3"))
    {
        SQLite.Open();
     
        //Checks if username exists (returns if not)
        Command = $"SELECT COUNT(1) FROM Accounts WHERE Username = '{Username}'";
        if ((Int64) new SQLiteCommand(Command, SQLite).ExecuteScalar() == 0)
        {
            LoginLabelError.Text = "Account doesn't exist";
            LoginLabelError.Left = (LoginPanel.Width - LoginLabelError.Width) / 2;
            LoginLabelError.Visible = true;
            return;
        }

        //Could probably turn these two into single statement but dont know how
        Command = $"SELECT Password FROM Accounts WHERE Username = '{Username}'";
        string PasswordHash = (string) new SQLiteCommand(Command, SQLite).ExecuteScalar();

        Command = $"SELECT Salt FROM Accounts WHERE Username = '{Username}'";
        byte[] Salt = Convert.FromBase64String((string) new SQLiteCommand(Command, SQLite).ExecuteScalar());

        if (PasswordHash == Hash(Password, Salt))
        {
            LoginPanel.Visible = false;
            LoginTBPassword.Text = "";
        }
        else
        {
            LoginLabelError.Text = "Incorrect Password";
            LoginLabelError.Left = (LoginPanel.Width - LoginLabelError.Width) / 2;
            LoginLabelError.Visible = true;
        }
    }
}

The register code:

//Register code
private void LoginButtonReg_Click(object sender, EventArgs e)
{
    if (!ValidLogin())
    {
        return;
    }

    string AppData = Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData);
    string MDBPath = AppData + "\\Password Manager\\Accounts.mdb";

    //Creates nessacary folders
    if (!File.Exists(MDBPath))
    {
        if (!Directory.Exists(AppData + "\\Password Manager"))
        {
            Directory.CreateDirectory(AppData + "\\Password Manager");
        }
    }

    string Username = LoginTBUsername.Text;
    string Password = LoginTBPassword.Text;
    byte[] SaltByte = new byte[128 / 8];
    using (RandomNumberGenerator RNG = RandomNumberGenerator.Create())
    {
        RNG.GetBytes(SaltByte);
    }
    Password = Hash(Password, SaltByte);
    string Salt = Convert.ToBase64String(SaltByte);

    //Heres where i need feedback
    string Command;
    using (SQLiteConnection SQLite = new SQLiteConnection($"Data Source={MDBPath};Version=3"))
    {
        SQLite.Open();

        Command = "CREATE TABLE IF NOT EXISTS Accounts (Username varchar(32), Password varchar(32), Salt varchar(32))";
        new SQLiteCommand(Command, SQLite).ExecuteNonQuery();

        //Checks if account doesnt already exist
        Command = $"SELECT COUNT(1) FROM Accounts WHERE Username = '{Username}'";
        if ((Int64) new SQLiteCommand(Command, SQLite).ExecuteScalar() > 0)
        {
            LoginLabelError.Text = "Account already exists";
            LoginLabelError.Left = (LoginPanel.Width - LoginLabelError.Width) / 2;
            LoginLabelError.Visible = true;
            return;
        }

        //Hides the login panel and clears the password box
        LoginPanel.Visible = false;
        LoginTBPassword.Text = "";

        Command = $"INSERT INTO Accounts VALUES ('{Username}', '{Password}', '{Salt}')";
        new SQLiteCommand(Command, SQLite).ExecuteNonQuery();

        //Creates a folder with id where i store the user specific passwords
        Command = "SELECT COUNT(*) FROM Accounts";
        Int64 RowCount = (Int64) new SQLiteCommand(Command, SQLite).ExecuteScalar();
        Directory.CreateDirectory(AppData + $"\\Password Manager\\Account{RowCount}");
    }
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ A tip: look at NuGet package "Bouncy Castle". It provides a lot of modern encryption and hashing algorithms. \$\endgroup\$
    – aepot
    Commented Jan 24, 2021 at 0:02
  • \$\begingroup\$ I have never been a fan of keeping the salt stored near the application or the database and neither decrypting a hashed string!. because if the app can decrypt what is encrypted, then there is no reason to encrypt it in the first place! \$\endgroup\$
    – iSR5
    Commented Jan 26, 2021 at 4:08

1 Answer 1

3
\$\begingroup\$

As is very common for starter code of this nature, your view, model and controller are all mashed together. LoginPanel.Visible = false is a view obligation; file operations are a controller obligation and username/password representation are a model obligation. These all need to be teased apart - do some reading on MVC or MVVM, or more traditional ideas of "business layer" / "data layer".

Don't do this:

    Command = $"SELECT COUNT(1) FROM Accounts WHERE Username = '{Username}'";

Use EXISTS instead of COUNT(1), and never EVER leave your commands open to injection attack by doing naive string interpolation of user data. Use prepared statements instead.

These two statements here:

    Command = $"SELECT COUNT(1) FROM Accounts WHERE Username = '{Username}'";
    Command = $"SELECT Salt FROM Accounts WHERE Username = '{Username}'";

can be combined into one statement that issues a SELECT Salt with a predicate for the username. If no rows are returned, the username was wrong.

Rather than constructs like this:

Directory.Exists(AppData + "\\Password Manager")

use System.IO.Path for concatenation. Even if you did need a backslash, use an @ string to avoid the need for an escape.

This:

Command = "CREATE TABLE IF NOT EXISTS Accounts (Username varchar(32), Password varchar(32), Salt varchar(32))";
    

is something that should be done on application startup, not in the middle of a LoginButtonReg_Click UI call.

\$\endgroup\$
4
  • \$\begingroup\$ You say to create the table if not exists on application start-up. I'm really pedantic so i put it there if the table is deleted while the application is running. \$\endgroup\$
    – Evorage
    Commented Jan 23, 2021 at 17:57
  • \$\begingroup\$ Why would that ever happen? This should be a purpose-assigned database that should never have tables deleted, never mind have concurrent users. \$\endgroup\$
    – Reinderien
    Commented Jan 23, 2021 at 18:04
  • \$\begingroup\$ The more important thing here (though I don't know whether SQLite supports it) is that your application user should not have DDL permission at all. \$\endgroup\$
    – Reinderien
    Commented Jan 23, 2021 at 18:04
  • \$\begingroup\$ what if the file gets deleted while the application is running. With my code, it creates the DB file on connection (version 3 SQLite) and creates the table. Eh its be being pedantic \$\endgroup\$
    – Evorage
    Commented Jan 24, 2021 at 13:35

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.