Why does lock(this) thread.sleep not work with ASP.NET threading ?

Question

Hi

I have a password page and when someone enters an incorrect password I want to simply foil a brute force attack by having

bool isGoodPassword = (password == expected_password);

lock (this)
{
     if (!isGoodPassword)
           Thread.Sleep(2000);
}

I would expect that this would allow all correct passwords without stalling, but if one user enters a bad password another successful password from a different user would also be blocked. However, the lock doesn't seem to lock across ASP.NET threads. Doesn't make sense.

Answer 1

Well, you haven't shown what "this" is, but if you're in the context of a page... each request is going to get its own instance of the page, isn't it? Otherwise how would they have different passwords in the first place? You'll have several threads, each locking against a separate object.

In many ways this is a good thing: you don't want genuine users to be affeted by an attacker. On the other hand, it means that the attacker just needs to make several attempts in parallel in order to effectively ignore your attempt to foil him. As other answers have stated, you could get round this by using a single object - but please don't. Don't forget that new threads won't created ad infinitum by IIS: this approach would allow a single attacker to make your entire app unusable for all users, not just for authentication but throughout the app... and they wouldn't even need to have a valid password.

Instead, you may wish to consider recording IP addresses which have failed authentication, and throttle the number of requests you are willing to process that way. (Admittedly that may have issues for some users if they're behind the same proxy as an attacker, but it's less likely.) This won't stop a distributed attack, but it's a good start.

Answer 2

If you really, really want to block ALL users from accessing the page if one of the messes up his password, you could always do

bool isGoodPassword = (password == expected_password);

lock (this.GetType())
{
     if (!isGoodPassword)
          Thread.Sleep(2000);
}

as you have written it, this would just slow down the refresh of the current request, it will not foil a multiple connections attack.

Also, comparing the passwords implies that you know the users password, ant that is always a bad practice. A better approach is to keep a (salted) hash of the user's pass, and compare that to a hash of the input. Also, you might want to employ progressive delays (1st mistake - 1 sec wait time, 2nd mistake - 2s, 3rd - 4, and so on)

Answer 3

ASP.NET runs each request on a separate thread. If you want to lock across requests, you can use a static object:

public class LogOn : Page
{
    private static object _delaySync = new object();

    private void Authenticate()
    {
        lock(_delaySync)
        {
             if(password != expected_password)
             {
                 Thread.Sleep(2000);
             }
        }

    }
}

It might make more sense, though, to track requests by IP and block any which send a certain volume over a certain amount of time.

Answer 4

My two cents: I'd looked for a different approach. I don't believe a software solution is the correct place to prevent a denial attack. In the end this solution will fail. It takes time for IIS to process code to the point where it reaches the lock code. The lock code does not prevent the requests. It just allows only one to pass through at a time. Effectively it acts as a queue.

With that said, try using a static variable.

private static readonly  object _lock = new object();

...

lock (_lock)
{
     if (!isGoodPassword)
           Thread.Sleep(2000);
}