9
\$\begingroup\$

This is my first C# project. I still am a noob to programming, and while I'm happy I carried out this project, I know that my code needs A LOT of improving and that's why I posted it here.

This code represents a Login system for a bigger application in console, a password manager. This is why the dashboard() function in my code is commented, as I only posted the code for the login system.

Basically, the Main is composed from only one big recursive function, registerOrLogIn(). This function asks for 2 commands, 'Register' or 'Log in'. If the input is different, the function will of course call itself again (that's why it is a recursive one, right?) until the first input will be equal to 'Register' or 'Log in'. If either of them is inputted, the program will create an object from class Credentials. This object has 2 proprieties, username and password, both which are created by user input. Of course, when the user types the password, it will appear on console as asterisks like that ****, using maskPass() function.

Furthermore, after the object user is instantiated, the program will work with 2 cases. If first input was 'Log in', the program, through dbVerifyUsername(user.getUsername), will search in the database if the inputted user actually exists. Of course, if the input was 'Register', the program will make sure through dbVerifyUsername(user.getUsername) that the username if not already picked.

Using hashPassword(pass) function, program hashes and saltes the password that the user inputs in the database when they register. In verifyHashedPass(oldPass, newPass), the already saved password's hash and the introduced password are verified in order to match. If they do, great! the user can finally log in. In my code, when the user carries out the registration or the login, the function dashboard(username) is called. Otherwise, the console displays why they could not access their dashboard (e.g. passwords that do not match).

using System;
using System.Data.SqlClient;
using System.Security.Cryptography;

namespace PasswordManagerwithCRUDoperations
{
    class Program
    {
        static void Main(string[] args)
        {
            registerOrLogIn();
        }
        static void registerOrLogIn()
        {
            Console.WriteLine("Write 'Register' if you want to sign up or 'Log in' if you want to sign in");
            string input = Console.ReadLine();
            if (input != "Register" && input != "Log in")
            {
                Console.WriteLine("Command does not exist");
                registerOrLogIn();
            }
            else
            {
                Console.WriteLine("Your command was {0}", input);
                Credentials user = new Credentials();
                Console.WriteLine("Username: ");
                string usern = Console.ReadLine();
                Console.WriteLine("Password: ");
                string pass = maskPass();
                user.setUsername(usern);
                user.setPassword(pass);
                if (input == "Register")
                {
                    if (dbVerifyUsername(user.getUsername()))
                    {
                        hashPassword(user.getPassword());
                        dbInsertUser(user.getUsername(), hashPassword(user.getPassword()));
                        Console.WriteLine("You are now registered as {0}", user.getUsername());
                        //dashboard(user.getUsername());
                    }
                    else
                    {
                        Console.WriteLine("Username already exists");
                        registerOrLogIn();
                    }
                }
                if (input == "Log in")
                {
                    SqlConnection connection = new SqlConnection(@"Server=Server;Database=passmanagerdb;Trusted_Connection=true");
                    connection.Open();
                    SqlCommand command = new SqlCommand("Select password from userstbl where username=@usern", connection);
                    command.Parameters.AddWithValue("@usern", user.getUsername());

                    using (SqlDataReader reader = command.ExecuteReader())
                    {
                        if (reader.Read())
                        {
                            string newPass = Convert.ToString(user.getPassword());
                            string oldPass = Convert.ToString(reader["password"]);
                            verifyHashedPass(newPass, oldPass);
                            if (verifyHashedPass(newPass, oldPass))
                            {
                                Console.WriteLine("Grant acces");
                                //dashboard(user.getUsername());
                            }

                            else
                            {
                                Console.WriteLine("Acces denied. Passwords do not match");
                                registerOrLogIn();
                            }

                        }
                        else
                        {
                            Console.WriteLine("Acces denied. Username not found in the database");
                            registerOrLogIn();
                        }
                    }
                    connection.Close();
                }
            }
        }

        static string maskPass()
        {
            string pass = "";
            ConsoleKeyInfo key;
            do
            {
                key = Console.ReadKey(true);
                if (key.Key != ConsoleKey.Backspace && key.Key != ConsoleKey.Enter)
                {
                    pass += key.KeyChar;
                    Console.Write("*");
                }
                else
                {
                    if (key.Key == ConsoleKey.Backspace && pass.Length > 0)
                    {
                        pass = pass.Substring(0, (pass.Length - 1));
                        Console.Write("\b \b");
                    }
                    else if (key.Key == ConsoleKey.Enter)
                    {
                        break;
                    }
                }
            } while (key.Key != ConsoleKey.Enter);
            Console.WriteLine();
            return pass;
        }
        static bool dbVerifyUsername(string usern)
        {
            SqlConnection connection = new SqlConnection(@"Server=Server;Database=passmanagerdb;Trusted_Connection=true");
            connection.Open();
            SqlCommand command = new SqlCommand("Select id from userstbl where username=@usern", connection);
            command.Parameters.AddWithValue("@usern", usern);

            using (SqlDataReader reader = command.ExecuteReader())
            {
                if (reader.Read())
                {
                    connection.Close();
                    return false;
                }
                else
                {
                    connection.Close();
                    return true;
                }
            }
        }
        static string hashPassword(string pass)
        {
            RNGCryptoServiceProvider provider = new RNGCryptoServiceProvider();
            byte[] salt = new byte[16];
            provider.GetBytes(salt);
            Rfc2898DeriveBytes pbkdf2 = new Rfc2898DeriveBytes(pass, 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);

            string savedPass = Convert.ToBase64String(hashBytes);
            return savedPass;
        }
        static bool verifyHashedPass(string newPw, string oldPw)
        {   //reader["password"] - oldPw
            //user.getPassword() -newPw
            string theSavedPass;
            theSavedPass = Convert.ToString(oldPw);
            byte[] hashBytes = Convert.FromBase64String(theSavedPass);

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

            bool ok = true;
            for (int i = 0; i < 20; i++)
            {
                if (hashBytes[i + 16] != hash[i])
                {
                    ok = false;
                }
            }
            return ok;
        }
        static void dbInsertUser(string usern, string pass)
        {
            SqlConnection connection = new SqlConnection(@"Server=Server;Database=passmanagerdb;Trusted_Connection=true");
            connection.Open();
            SqlCommand command = new SqlCommand("Insert into userstbl (username, password) values (@usern, @pass)", connection);
            command.Parameters.AddWithValue("@usern", usern);
            command.Parameters.AddWithValue("@pass", pass);
            command.ExecuteNonQuery();
            connection.Close();
        }

        class Credentials
        {
            private string username;
            private string password;

            public void setUsername(string usern)
            {
                username = usern;
            }
            public string getUsername()
            {
                return username;
            }
            public void setPassword(string pass)
            {
                password = pass;
            }

            public string getPassword()
            {
                return password;
            }
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! Please tell us more about what your code is supposed to do and whether it does so satisfactory. Did you test it? Got any specific gripes with it? \$\endgroup\$ – Mast Oct 8 at 17:57
  • 1
    \$\begingroup\$ I'll update the description \$\endgroup\$ – Code Monkey Oct 8 at 18:02
4
\$\begingroup\$

A few things I noticed:

In maskPass after each ReadKey you're either concatenating a character with pass or you're assigning pass to a Substring of pass. This is very inefficient. Each concatenation and each call to Substring, creates a new string. It would be much better to use a StringBuilder to store the string until you're ready to return it.

In verifyHashedPass you convert the string oldPw to a string then assign it to a variable, then you only use that variable once. I would suggest using oldPw directly.

Also in the loop where you verify that the 2 hashes are the same, instead of using a bool there, it would make more sense to return false as soon as you find a mismatch and true if the loop finishes.

In the Credentials class, you can simplify it by using automatic get and set:

public string username{ get; set;}

This translates to the same thing as you are already doing but making it much more simple.

\$\endgroup\$
  • \$\begingroup\$ The bool in verifyHashedPass is probably there to make the timing as constant as possible. It's debatable whether this is necessary for comparing hashed passwords, but at least there should be a comment in the code, otherwise it might get "optimized" away. \$\endgroup\$ – Roland Illig Oct 13 at 7:32
4
\$\begingroup\$

Disclaimer: my first post here and I'm not C# programmer


If the input is different, the function will of course call itself again (that's why it is a recursive one, right?)

This is dangerous, as user can keep entering wrong inputs, which will increment scoping stack (eventually resulting in StackOverflowException being thrown).
Even doubly so, your function is the beginning of the program (it's not returning but calling dashboard() to pass on), so all stack layers made by wrong inputs will be kept all the way through

Most basic approach of mitigating this is making it a loop with exiting condition

do
{
    //Input from console
}while(/*input isn't the one we need*/)

You may even add some protection from spamming and limit amount of tries later simply by working around this loop (unlike function stack, where you need to carry iteration results into next layers)

\$\endgroup\$
  • \$\begingroup\$ Generally speaking, a recursive approach to this isn't that bad. Because the recursive call can be a tail call, the additional stack layers can be optimized away, making the recursion effectively act as a loop. Unfortunately the C# Compiler does not support this, which is a shame. \$\endgroup\$ – LukeG Oct 21 at 5:21
  • \$\begingroup\$ @LukeG It's not tail call because function does not return. It's head recursion because it calls dashboard in the end \$\endgroup\$ – Noone AtAll Oct 21 at 8:28
  • \$\begingroup\$ There are multiple instances where the recursive call is a tail call. Tail call does not necessarily need a return keyword, if it's the last statement in a function. This is definitely the case for the first recursive call, and the remaining 3 can be rewritten to be in tail call position. What the last called instance of "registerOrLogIn" does is irrelevant to the question, if a given recursive call is a tail call. \$\endgroup\$ – LukeG Oct 21 at 8:41
3
\$\begingroup\$

All in all, you did quite well. If you have the option, I'd recommend letting a system like Active Directory handle user credentials rather than storing and validating themselves. However, as there's a cost involved with that, password hashes are the next best thing.

PBKDF2 is a better choice than what I'm used to people using and you're salting it too, so kudos on that. (Argon2id and SCrypt are also good options. MD5 and SHA1 should not be used for passwords. SHA2 is on its way out. I don't know enough about SHA3 to comment.)

I noticed that you have your connection string hard coded. I'm not sure if that's just for the code example or not, but it would be a good idea to move that outside of your code base (even though it doesn't have a password in it).

\$\endgroup\$
3
\$\begingroup\$

Recusion Abuse

This does not seem to be a naturally recursive problem and using recursion here causes more issues than it is worth:

  • If the user enters too many invalid inputs, they can blow the stack.
  • If a user errors out by inputting an incorrect password, they have to re-select their intent to login before they are able to try again.

  • Control flow is not very flexible as the primary tool of control is recursively starting from the beginning.

  • What if you wanted to lock out the user after entering 5 incorrect passwords? Keeping track of this would be needlessly complex using recursion.
  • Decreased readability.

Single Responsibility Principle

Functions should do one thing. They should do it well. They should do it only. The method name "registerOrLogIn" immediately signals the method is doing more than one thing because of the "or". The method is responsible for registering the user and logging in the user. This should be separated into more modular methods. Also, if there are other ways the user can access your application in the future will you keep adding to this method? It makes sense to start to separate things now.

Resource Management

Explicitly Calling .Close() is not necessary if wrapped in a using statement in C#. Consider wrapping all of your SqlConnection statements in a using statement. Also, your connection string should be a global constant of some sort. Realistically this would probably come from the web.config, but a constant in the program class would work for now.

Conclusion

This is a great start! I like the way you are salting your hashes. Although things could be more modular, it is pretty easy to understand the flow of the code. I have recorded my live code review and will leave the link below. I hope this feedback helps!

Live code review: https://www.youtube.com/watch?v=c9ZheaGGRMI

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review, nice first answer! Really above and beyond with a live code review! \$\endgroup\$ – Simon Forsberg Oct 26 at 23:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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