3
\$\begingroup\$

I want to be sure that my code below properly secures the website based on all of the information provided.

Login System Overview

When a user logs in, the following user info is saved to a persistent cookie:

  • User Id (Primary Key)
  • First Name
  • Last Name
  • User Role
  • Login Token (randomly generated guid value)
  • Valid (bool - indicates the login token is valid for this user)

A successful login creates the cookie:

[HttpPost]
public JsonResult Login(LoginUserModel user)
{
    try
    {
        // DoLogin throws an error if any of the details are incorrect otherwise login proceeds normally.
        if (DoLogin(user.EmailAddress, user.Password))
            return CreateResponseMessage(true);

        return ReportError(new Exception("Invalid login credentials"), "Log In");
    }
    catch (Exception ex)
    {
        return ReportError(ex, "Log In");
    }
}

private bool DoLogin(string emailAddress, string password)
{
    var User = db.Users.Include("UserRole").Where(x => x.EmailAddress == emailAddress && x.Deleted == false).FirstOrDefault();

    if (User != null)
    {
        if (String.Compare(User.UserRole.RoleName, "Admin", false) == 0)
        {
            if (Hashing.ValidatePassword(password, User.Password))
            {
                BaseLogin Login = new BaseLogin
                {
                    UserID = User.Id,
                    Token = Guid.NewGuid().ToString(),
                    LoginDate = DateTime.Now
                };

                db.Logins.Add(Login);
                db.SaveChanges();

                GenerateCookie(User, Login.Token);

                return true;
            }
        }
    }
    return false;
}

private void GenerateCookie(BaseUser User, string Token)
{
    HttpCookie UserCookie = new HttpCookie("ortund");
    UserCookie.Values["uid"] = Convert.ToString(User.Id);
    UserCookie.Values["fname"] = User.FirstName;
    UserCookie.Values["lname"] = User.LastName;
    UserCookie.Values["role"] = User.UserRole.RoleName;
    UserCookie.Values["token"] = Token;
    UserCookie.Values["valid"] = bool.TrueString;
    UserCookie.Expires = DateTime.Now.AddMonths(1);
    Response.Cookies.Add(UserCookie);
}

Logins are salted and hashed, then evaluated for comparison against saved information:

/// <summary>
/// Creates a salted PBKDF2 hash of the password.
/// This is done when the user record is created.
/// </summary>
/// <param name="password">The password to hash.</param>
/// <returns>The hash of the password.</returns>
public static string CreateHash(string password)
{
    // Generate a random salt
    RNGCryptoServiceProvider csprng = new RNGCryptoServiceProvider();
    byte[] salt = new byte[SALT_BYTE_SIZE];
    csprng.GetBytes(salt);

    // Hash the password and encode the parameters
    byte[] hash = PBKDF2(password, salt, PBKDF2_ITERATIONS, HASH_BYTE_SIZE);
    return PBKDF2_ITERATIONS + ":" +
        Convert.ToBase64String(salt) + ":" +
        Convert.ToBase64String(hash);
}

/// <summary>
/// Validates a password given a hash of the correct one.
/// </summary>
/// <param name="password">The password to check.</param>
/// <param name="correctHash">A hash of the correct password.</param>
/// <returns>True if the password is correct. False otherwise.</returns>
public static bool ValidatePassword(string password, string correctHash)
{
    // Extract the parameters from the hash
    char[] delimiter = { ':' };
    string[] split = correctHash.Split(delimiter);
    int iterations = Int32.Parse(split[ITERATION_INDEX]);
    byte[] salt = Convert.FromBase64String(split[SALT_INDEX]);
    byte[] hash = Convert.FromBase64String(split[PBKDF2_INDEX]);

    byte[] testHash = PBKDF2(password, salt, iterations, hash.Length);
    return SlowEquals(hash, testHash);
}

Every time the user performs an action, the cookie is checked as to ensure the cookie data matches the login data from the database:

public bool CheckUserCookie()
{
    try
    {
        string Token = Convert.ToString(Request.Cookies["ortund"]["token"]);
        var Login = db.Logins.Where(x => x.Deleted == false).FirstOrDefault(x => x.Token == Token);

        int UserId = Convert.ToInt32(Request.Cookies["ortund"]["uid"]);

        if (Login == null || Login.UserID != UserId)
        {
            ProcessLogout();
            return false;
        }

        return true;
    }
    catch
    {
        ProcessLogout();
        return false;
    }
}

public void ProcessLogout()
{
    try
    {
        if (Request.Cookies["ortund"] != null)
        {
            string Token = Convert.ToString(Request.Cookies["ortund"]["token"]);
            var Login = db.Logins.FirstOrDefault(x => x.Token == Token);

            if (Login != null)
            {
                Login.Deleted = true;
                db.SaveChanges();
            }
        }
    }
    catch(Exception ex)
    {
        Error Error = new Error
        {
            Action = "Log out",
            Date = DateTime.Now,
            Detail = ex.ToString(),
            Message = ex.Message,
            StackTrace = ex.StackTrace
        };

        SaveErrorDetails(Error);
    }
    finally
    {
        Session.Clear();
        HttpCookie UserCookie = new HttpCookie("ortund");
        UserCookie.Expires = DateTime.Now.AddMonths(-1);
        Response.Cookies.Add(UserCookie);
    }
}

This means that if a token is used with a user id which doesn't correspond with the id associated with the token, the cookie will be invalidated and deleted and the user will be redirected back to the login screen.

I shared a link to a site that employs this system along with a demo account in order to showcase the system. One person told me this:

ortund, open a new browser with no cookies stored whatsoever. Visit and you will be presented the login dialog. Don't enter anything there but open the browser's dev console. in the JS window, enter the following: document.cookie = "ortund=uid=4&fname=Demo&lname=User&role=Admin&token=7da95c2c-a127-4526-bf40-f9bccb19223b&valid=True"

As I understand this effectively hijacks a login which is, indeed a concern, however:

  • The person who showed me this had the URL for the site
  • The person who showed me this had login credentials for the site
  • Had already used those credentials to successfully log into the site

If I understand my own code and also the hack, it occurs to me that without an existing login, this hack would be impossible. Moreover, if it were employed and the uid and token values of the cookie didn't correspond with those on any single record in the database, the hack would also fail.

Is this "properly" secure? Can I improve security here and would such improvement require the a full restart on the development of the website in which this system is employed?

\$\endgroup\$
  • \$\begingroup\$ Perhaps this would be more relevant on security.stackexchange.com ? (But check their tour and the appropriate help pages first before posting.) \$\endgroup\$ – BCdotWEB May 19 '16 at 14:43
  • \$\begingroup\$ Okay so the guy who edited my question TOTALLY changed the purpose of it with his update to the question title. Now I don't think its likely that I'll get anything close to an answer having anything to do with my purpose for asking. \$\endgroup\$ – Ortund May 19 '16 at 15:09
  • \$\begingroup\$ @Ortund You will likely still get an answer adressing the security aspects. Titles of the form "Is my login sufficiently secure?" are very generic and applies to all questions with the security tag, which is why we on Code Review normally use question titles with a short summary of what the code does. It is our experience that questions gets more interesting and more attention that way. \$\endgroup\$ – Simon Forsberg May 19 '16 at 16:39
  • \$\begingroup\$ @SimonForsberg well it doesn't seem so in this case, now does it? Given that your comment was the most recent thing to happen on this question and it didn't even address the security aspects at all. \$\endgroup\$ – Ortund May 19 '16 at 17:46
  • \$\begingroup\$ @Ortund Code Review does not get as much traffic as Stack Overflow, this can be both good and bad. Be patient. I'm just here to clear things up about how things normally work on Code Review, I'm sorry that I don't say anything about your particular security concerns. \$\endgroup\$ – Simon Forsberg May 19 '16 at 18:09
1
\$\begingroup\$

In your DoLogin method you have a large nested if statement, you could reduce the nesting by reversing the conditions and returning false when these conditions are met. This doesn't change much except for the nesting of the statements

if (User != null)
{
    if (String.Compare(User.UserRole.RoleName, "Admin", false) == 0)
    {
        if (Hashing.ValidatePassword(password, User.Password))
        {
            BaseLogin Login = new BaseLogin
            {
                UserID = User.Id,
                Token = Guid.NewGuid().ToString(),
                LoginDate = DateTime.Now
            };

            db.Logins.Add(Login);
            db.SaveChanges();

            GenerateCookie(User, Login.Token);

            return true;
        }
    }
}
return false;

Here is what I came up with after I was done

if (User == null) return false;
if (String.Compare(User.UserRole.RoleName, "Admin", false) != 0) return false;
if (!Hashing.ValidatePassword(password, User.Password)) return false;
BaseLogin Login = new BaseLogin
{
    UserID = User.Id,
    Token = Guid.NewGuid().ToString(),
    LoginDate = DateTime.Now
};
db.Logins.Add(Login);
db.SaveChanges();
GenerateCookie(User, Login.Token);
return true;
\$\endgroup\$
  • \$\begingroup\$ This one really divides opinion in my office. Early returns or Single Return....I prefer the bottom example but it is a divisive one! \$\endgroup\$ – KingCronus May 20 '16 at 20:54
  • \$\begingroup\$ @KingCronus we could write the 3 if statements as one single if statement that returns of one condition is true, then there are only two single returns, true or false. Not really an early return at all. \$\endgroup\$ – Malachi May 21 '16 at 2:43

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.