3
\$\begingroup\$

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);
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ What type/class is securityService? \$\endgroup\$ – ChrisW Mar 16 '14 at 17:41
4
\$\begingroup\$
  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).

  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.

\$\endgroup\$

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.