MemoryCache - Login Security

Question

I have a WCF service which should authenticate user against any bruteforce attack. Below is the code to avoid any bruteforce attack. I have used MemoryCache to keep track of invalid attempts by a user.

Could you please review the code and let me know if there are any issues?

Following defined in config file:

<InvalidLoginAttemptsThreshold value = "5"/>
<InvalidLoginLockoutInMinutes value = "20"/>

Code to add invalid login to cache and lock the account if invalid logins reaches threshold set in config.

private readonly CacheItemPolicy cacheInvalidLoginPolicy = new CacheItemPolicy();
private readonly ObjectCache cacheInvalidLogins = MemoryCache.Default; //Cache to hold Invalid Login Attempts: {UserId, obj UserLogin}

public class UserLogin
{
    public string UserName { get; set; }
    public List<DateTime> InvalidLoginTimestamp { get; set; }
    public bool IsLocked { get; set; }
}
private bool AuthenticateUser(string userName, string password)
{
    bool validCredentials = false;
    string userNameUpperCase = userName.ToUpper(); //MemoryCache treats 'a' and 'A' as different names
    if (cacheInvalidLogins.Contains(userNameUpperCase) && ((UserLogin)cacheInvalidLogins[userNameUpperCase]).IsLocked)
    {
        log.ErrorFormat("UserId: {0} tried to access locked account at {1}", userName, DateTime.Now);
        throw new FaultException("Your account is locked out. Please try after some time.", new FaultCode("AccountLocked")); 
    }

    try
    {
        validCredentials = securityService.Authenticate(userName, password);

        if (!validCredentials)
        {
            AddInvalidLoginToCache(userNameUpperCase);
        }
        else if (validCredentials && cacheInvalidLogins.Contains(userNameUpperCase))
        {
            //For valid credentials - remove from cache
            cacheInvalidLogins.Remove(userNameUpperCase);
        }
    }
    catch (Exception exception)
    {
        log.Error(exception);
    }

    return validCredentials;
}

private void AddInvalidLoginToCache(string userName)
{
    if (cacheInvalidLogins.Contains(userName))
    {
        string clientId = userName.Split('\\')[0];
        //Increment InvalidLoginCount for this user
        UserLogin invalidLogin = (UserLogin)cacheInvalidLogins[userName];
        invalidLogin.InvalidLoginTimestamp.Add(DateTime.Now);
        cacheInvalidLogins[userName] = invalidLogin; //todo: update cache policy here

        //Lock user account if InvalidLoginCount reach threshold set in config
        if (invalidLogin.InvalidLoginTimestamp.Count == invalidLoginThresholdFromConfig))
        {
            invalidLogin.IsLocked = true;
            cacheInvalidLogins.Set(userName.ToUpper(), invalidLogin, LockOutCachePolicyFromConfig());
            log.ErrorFormat("Invalid Login attempted by UserId: {0} at these timestamps: {1}", userName, String.Join(",", invalidLogin.InvalidLoginTimestamp));
            log.ErrorFormat("Account is locked out for UserId: {0}", userName);
            throw new FaultException("Your account is locked out. Please try after some time.", new FaultCode("AccountLocked")); 
        }
    }
    else
    {
        //todo: check if extra config setting is required for this cache policy
        cacheInvalidLoginPolicy.AbsoluteExpiration = DateTimeOffset.Now.AddMinutes(5);
        cacheInvalidLogins.Add(userName.ToUpper(),
                               new UserLogin {UserName = userName, InvalidLoginTimestamp = new List<DateTime> {DateTime.Now}},
                               cacheInvalidLoginPolicy);
    }
}

Answer 1

1. If I'm right it's not thread-safe. I suppose two browsers with the same username could login at the same time which means that the AuthenticateUser method runs parallel on two threads.

   Suppose the following:

   • the user already has an invalid login attempt,
   • two new threads enter AddInvalidLoginToCache the same time,
   • invalidLoginThresholdFromConfig = 2.

   A possible execution order is the following:

   // InvalidLoginTimestamp.Count = 1, we already have an invalid login attempt
   thread1: enters AddInvalidLoginToCache
   thread2: enters AddInvalidLoginToCache
   thread1: runs invalidLogin.InvalidLoginTimestamp.Add(DateTime.Now); // InvalidLoginTimestamp.Count = 2
   thread2: runs invalidLogin.InvalidLoginTimestamp.Add(DateTime.Now); // InvalidLoginTimestamp.Count = 3
   thread1: runs if (invalidLogin.InvalidLoginTimestamp.Count == 2)) // false 
   thread2: runs if (invalidLogin.InvalidLoginTimestamp.Count == 2)) // false
   

   The result is that a lucky attacker will never be locked out.

   The second problem is visibility. Since InvalidLoginTimestamp and invalidLogin are shared between threads you need some synchronization to ensure other threads will see modifications (not just stale data or invalid state).

   • C# multi-threading: Acquire read lock necessary?
   • How to synchronize the access to a List used in ASP.NET?

2. private readonly ObjectCache cacheInvalidLogins = MemoryCache.Default; //Cache to hold Invalid Login Attempts: {UserId, obj UserLogin}
   

   I'd put the comment before the line to avoid horizontal scrolling:

   //Cache to hold Invalid Login Attempts: {UserId, obj UserLogin}
   private readonly ObjectCache cacheInvalidLogins = MemoryCache.Default; 
   

   Furthermore, I'd rename it to invalidLoginAttemptsCache. The cacheInvalidLogins name looks like a method name since it starts with a verb (cache...).

3. bool validCredentials = false;
   ... // other statements
   try
   {
       validCredentials = securityService.Authenticate(userName, password);
   

   The boolean could be declared right before the first use with a smaller scope:

   ...  // other statements
   bool validCredentials = false;
   try
   {
       validCredentials = securityService.Authenticate(userName, password);
   

   Should I declare variables as close as possible to the scope where they will be used?

4. private void AddInvalidLoginToCache(string userName)
   {
       ...
       cacheInvalidLogins.Set(userName.ToUpper(), invalidLogin, LockOutCachePolicyFromConfig());
   

   The code calls this method with the uppercased username. I'd rename the parameter to upperCasedUserName and remove the .ToUpper() call.

5. This variable is never user, I guess you could remove it:

   string clientId = userName.Split('\\')[0];
   

6. Username seems to be one word, I'd not capitalize the n.