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?
