[Auth] Login security issue

[michalsn]

isThrottled method (in localAuthentication::login) should be called earlier, just after selecting user from database. Current implementation creates vulnerability.

Vulnerability scenario, where attacker can send many requests and in consequence guess password to any account:

1. When attacker reaches allowed_login_attempts, we display time to the next valid login request (via isThrottled method).
2. Every invalid login attempt is logged before calling isThrottled method.
3. Potential attacker set his script to send 1 request per 2 seconds.
4. Let's say he already reached max_throttle_time, so we display message with the same time all over again (or higher).
5. When attacker sent request with valid credentials, time in message will be smaller than previous one.
6. In this case throttling works only in theory, we can send as many requests as we want.

[lonnieezell]

The purpose is never to completely block the requests from happening. But to make the pause between requests long enough that brute force is not longer worth it and they move on. I personally hate the ones that lock you out for 30 minutes or similar and think they are horrible for the user, and only necessary for sites that have very intense security needs, like banks, government sites, etc. This auth was never intended for that, but to create a more secure version for the majority of sites than I was seeing out there at the time.

From my understanding, there are a couple of types of brute force attacks. The most common is to slam the server and try to crack the password quickly before it's noticed. That's the type that will get caught very easily by this system and, if found, will lock the user out of the account for 15 minutes. The second type is the distributed check which is done slower, and uses a number of proxy servers to spread the attack across multiple IP address, or even looking like multiple countries. This system doesn't work quite as well for that, but tries by detecting higher than average failed logins compared to the average daily logins across the last 3 months. In that case, it just adds on time before more attempts can be made IIRC.

Neither method is perfect, for sure, and the only way I know to truly protect is to lock accounts. And that can be an used as an attack against a site in and of itself - by slamming many accounts and locking out large chunks of users.

So, yes, a dedicated hacker could get past the protections provided here by slowing down their requests in the manner you mentioned. But the time to crack things rises by an order of magnitude making them a less and less appealing target.

[michalsn]

Yes, I hate it too when the account is being locked (for some period of time or even worst), but it's the whole point of throttling. This is how Sprint is handling this right now, but the problem is that we must wait only when we try to log in with correct credentials. Every incorrect attempt can be done imediately after another. IMO is wrong.

The attack scenario I has provided isn't very likely when we speak about "typical" brute force attack. But this is changing when concrete user is used as a target. If we assume dictionary attack based only on informations gathered from web, about this user (pet name, year of birth, hometown etc.), then time needed to check our possible variations isn't that bad (1 request per 2 seconds - I would try my luck). And yes, I know we have password strenght checker and even dictionary with english words to check during our password creation. All this things are great, but we still can do better :)

To be honest, I don't think that attack scenario I came up with will be very common ;) but still... if we can (in easy way) avoid this threat, why shouldn't we?

One thing about locking the account. We both agree that we hate it. I was thinking about it before and I was going to provide solution to allow user to log in even if his account is under attack and is locked out. To do so, I was going to implement #109, and then introduce something I called a one-time-pass. Basically if our account reached more than max_throttle_time - it means we qualify to ban (+15 or +45 minutes). In that case we can send email to user with a link, whith can be used only one time, and bypass the throttling check during login process. This is how idea looks in short.

Ok, this is all. If you think that I over-complicate simple things, just close this issue :)

[lonnieezell]

the problem is that we must wait only when we try to log in with correct credentials. Every incorrect attempt can be done immediately after another. IMO is wrong.

The original intent was that if you login with correct credentials, there is no wait, you're unaffected by throttling. If you get it wrong, then you get 5 tries, by default, before getting throttled, at which point the time increases up until the max time. If an account is determined to be under brute force attack, it's blocked for 15 minutes.

I'll need to look through the code to ensure we're still doing that.

I'm a big fan of the idea of a one-time pass as an option in general. I think it's a very elegant solution for many different types of sites, honestly.

[michalsn]

The original intent was that if you login with correct credentials, there is no wait, you're unaffected by throttling.

I think it's a bad idea. We should check for throttling on every attempt, because there is no good way to see the difference between valid user and attacker who got lucky and guessed password.

Right now we never skip isThrottled check. If we would skip it, it would be even worse, because then, potential attacker could be sending many requests per second.

[lonnieezell]

I think it's a bad idea. We should check for throttling on every attempt, because there is no good way to see the difference between valid user and attacker who got lucky and guessed password.

I'm willing to be convinced otherwise, but here's my thinking when I created the library:

If the account, or multiple accounts, are currently under a brute force attack, and a valid user wants to login, they should be able to without being told to wait. There's no way to tell in this situation if that user is the real user, a hacker who has used social engineering to get a password from another site that matches this one (possibly the easiest way to hack it), or a lucky guess by the brute force attack.

And I just thought of something else. How would throttling a user with a successful login work? Unless we kick them out of the account they just successfully logged into, it doesn't seem to do anything. The throttling is to present a barrier to make the cost high enough to make brute force attacks move on to another site.

The only way that we can tell a valid user in any circumstance that I can think of is that they know the login credentials. Because there are other ways for bad guys to get the credentials, even that's not perfect, but it's a good as a password system alone can do. Two-factor is better, and passwordless is even better, though that assumes the bad guy doesn't have access to the user's email account.

I'm just not sure there's a better way to handle this and not make the user want to cuss and scream at the website and go somewhere else.

Right now we never skip isThrottled check. If we would skip it, it would be even worse, because then, potential attacker could be sending many requests per second.

And the only time I am saying it should be skipped is on a successful login. If a brute force managed to guess the credentials, they'd stop the attack anyway.

I may be missing something, in which case I'd be happy to change things, but am not currently convinced.

[IrcDirk]

@michalsn You can always extend @lonnieezell classes to make your own solution. I think there is no need to change this, the security is on right level, as this kind of brute force attacks u mentioned may be blocked with other software (like anti DDOS protections, firewalls).

p.s. widzę, że nas tu dużo (czyt. Polaków)

[michalsn]

All you says here makes perfect sense and from user experience perspective it's great.

The problem is that I can't see the solution to combine this two things (proper throttling and letting in users with valid credentials without throttling check).

This is why I think it's a bad idea - simplified scenario:

1. user logs in with invalid credentials
2. throttling check - you must wait x seconds
3. right after, user logs in with invalid credentials, but he don't have to wait x seconds, because of good user experience
4. throttling check - you must wait x seconds
5. right after, user logs in with valid credentials, but he don't have to wait x seconds, because of good user experience
6. no throttling check - it's valid user
7. welcome user123

Can you see now what I mean? These attempts may have been made by good guy as well as a hacker. I will say it one more time - from valid user perspective it's great, but from security perspective... i don't think so.

IMO proper throttling should take place just before checking that current attempt is valid. Only that way we can determine if given IP address/account isn't throttled. Otherwise we just pretend that there is any additional security. Yes, user may not be happy, but on the other hand it's his account under attack and we just trying to protect it. Security comes with a price - in this case a valid user is the one who pays it.

If we deal with brute force attack (or distributed brute force) there is always a idea of one-time pass that I have mentioned earlier (and as I understand you like the idea). Additionally users that have no failed login attempts at all, are not affected by throttling.

If you still don't consider this one as a real vulnerability I probably give up. I think I ran out of arguments ;)

[michalsn]

@IrcDirk Of course I can, but this is not the point ;) This is open source project and I see place for some improvements. Now we are discussing whether my concerns are valid or not. I just think that this auth library may be even better.

BTW I don't think we should rely solely on server software in that matter. But this is not the subject of this issue ;)

P.S. Prawdziwy tłum :)

[IrcDirk]

Much simpler solution to prevent brute force attacks, often used, is to show some kind of captcha after x login attempts. I use google recaptcha, it has good security. User doesnt have to wait x seconds becouse of something...

p.s. na githubie ogolnie jest naprawde duzo polakow ;)

[lonnieezell]

First off - I definitely appreciate the ideas and constructive criticism, thanks!

I think I'm still missing something. In your example, you pairs of the following 2 lines which don't seem to go together to me:

right after, user logs in with invalid credentials, but he don't have to wait x seconds, because of good user experience
throttling check - you must wait x seconds

Here's my original design (though I think one of your previous PR's moved the throttling location... but that's ok.)

• user has invalid login. He can try again, no delay, until the number of failed attempts matches the config setting (which could be set to 1 if you want the delay to start immediately)
• user has invalid login. Throttled, wait X seconds.
• user has invalid login. Throttle, wait X seconds.
• user has valid login. No throttling, he gets in the system.

Now, if the account is already under a brute force attack, from the user's perspective it looks like this:

• attack: throttled X seconds
• user has invalid login: throttled X seconds
• attack: throttled X seconds
• user has valid login: he's in
• attack, still throttled.

I think that last line may be what you've described in the past that I wasn't getting? Because once a valid user logs in, the attack might get more free access? I honestly don't recall if that is the situation, though I think the check was happening by email previously. I think you've switched that IP now, which is great, since it should keep the attacker being throttled while the user still logs in. I'm not in front of my primary computer right now and will have to check later.

Here's the other part I don't understand, though, and I think this is me being confused about what you're saying: let's say that the user is under throttling but puts a valid login in. You say the user should still be throttled on a valid login. What affect does that have? If the login credentials are valid, throttling doesn't have any affect because they get in the system. Whether we check if they should be throttle or not, the outcome is the same - the user gets in with no delay. We can't say, "You validated but you must wait a few seconds and try again" :) We hit a loop there.

[lonnieezell]

Oh - you're right about adding some form of security check instead of throttling. That might be a simpler solution, anyway :)

If we go that route, we should have an Interface in place for them and create a couple of different ones, like a reCaptcha, and a question-based one. phpBB 3 has something similar.

[michalsn]

Here is the real problem: does we really must wait X seconds between login attempts or it's just a message that we see? The answer is... this is just a message. We can make many requests and don't have to wait X seconds between them. It's because throttling is checked after validating user. Right now, this is smaller issue, because it cause only one real problem, that I described originally in this issue.

This will be a real threat when user with valid credentials will skip a throttling check. Why? If a certain user account is under attack, every login attempt must be considered as a request from attacker. I may poorly described what I mean before, but here is your example:

• attack: throttled X seconds
• user has invalid login: throttled X seconds
• attack: throttled X seconds
• user has valid login: he's in
• attack, still throttled.

Please, replace "user" with "attacker" in above example. Can you see the problem? When throttling is noticed we shouldn't even check if user credentials are ok. We should just send message that "you must wait X seconds to the next login attempt". Otherwise we're risking to let in attacker who guessed the password. I know that you hate this kind of behavior in web apps (so do I) but I think that there is no other choice from security perspective.

I just have checked if I really didn't messed up something with my PR's, but no... the logic behind login process stays the same as it was before. And I don't see there any skipping for throttling (in case of valid login). Best if you check it by yourself, when you will have some free time.

My two cents about captcha... You may disagree with me, but I don't consider google captcha as "good security" against any automated attack since there are SAAS apps that offers breaking it. But this is just my opinion. Of course I will not argue that for some sites, using google captcha may be beneficial. Nevertheless... if we would like to implement captcha properly, we still need to check for number of failed attempts for each request (including request with valid credentials).

[IrcDirk]

First of all, everithing can be cracked/breaked ... there is no 100% safe solution. First of all if we make recaptcha + another solution/solutions that proposed @lonnieezell it will be very problematic to make proper breaking bot for it. As for captcha there are many other posibilites. Thing is that u should consider if u need so much complex and secure solution on your site. Another is that users should make safe passwords, which u can force them if really need good security on site. Since if they have 1234 password throtling will not help ;)

[michalsn]

I'm not seeking for 100% safe solution. I just see here some simple mistakes (from my perspective of course) that degrade the level of security, and this can be easily avoided.

To be honest, I don't think that idea with captcha is totally wrong. What I had in mind is that not every captcha solution can be trusted at the same level, due to ready to use solutions that break it. From the other hand, OWASP (as I can remember) don't consider captchas as effective security solution.

I think that Sprint should provide as much security as it can. If you find that you don't need throttling (it's just an example) for a certain project, you can easily disable it right now. But default settings should follow the best practices to gain as much security as we can. This is my point of view.

About the safe passwords - Sprint won't let us to choose very simple passwords, so I think we're safe here :)

[lonnieezell]

Please, replace "user" with "attacker" in above example. Can you see the problem? When throttling is noticed we shouldn't even check if user credentials are ok. We should just send message that "you must wait X seconds to the next login attempt". Otherwise we're risking to let in attacker who guessed the password. I know that you hate this kind of behavior in web apps (so do I) but I think that there is no other choice from security perspective.

OK, I think I finally understand what you're saying. Sorry for being dense this time around. :) And I'm of mixed opinion. If the account is definitely under an attack (100+ attempts in a minute) they are locked out for 15 minutes already. With your proposal, as soon as the exceed their max free attempts, they are also effectively locked out because they must wait X seconds before making another attempt. When they make another attempt, they are told the same thing. Looking at the code in the system currently, it is impossible for anyone to login again on that account if we do it that way because it just looks at the total number of failed attempts for that account. So bigger things will have to be looked at before we can make any changes like this.

What does OWASP say about it? In one place they agree with account lockout - but we'd need a config setting for max allowed failed attempts to make it work. And I'm ok with having this implemented in this way. (I'll make a few issues with items we need to implement based on re-reading their recommendations tonight). This is from the Cheat Sheet, which appears to be the most recent information, though additional discussion can be found in their Guide to Authentication

Password lockout mechanisms should be employed that lock out an account if more than a preset number of unsuccessful login attempts are made. Password lockout mechanisms have a logical weakness. An attacker that undertakes a large number of authentication attempts on known account names can produce a result that locks out entire blocks of user accounts. Given that the intent of a password lockout system is to protect from brute-force attacks, a sensible strategy is to lockout accounts for a period of time (e.g., 20 minutes). This significantly slows down attackers, while allowing the accounts to reopen automatically for legitimate users.

https://www.owasp.org/index.php/Authentication_Cheat_Sheet#Prevent_Brute-Force_Attacks

As for Captcha - reading through OWASP stuff again, and they have this to say: "Applications MUST NOT implement CAPTCHA as there is case law against them with respect to universal access and ineffective" So probably won't go that route. :)

[michalsn]

If the account is definitely under an attack (100+ attempts in a minute) they are locked out for 15 minutes already. With your proposal, as soon as the exceed their max free attempts, they are also effectively locked out because they must wait X seconds before making another attempt. When they make another attempt, they are told the same thing. Looking at the code in the system currently, it is impossible for anyone to login again on that account if we do it that way because it just looks at the total number of failed attempts for that account. So bigger things will have to be looked at before we can make any changes like this.

I'm not quite sure if I understood you correctly. We have two conditions here, so it's all ok:

if ($attempts > 100 && $this->ci->login_model->isBruteForced($ip_address, $user_id))

isBruteForced method, checks if number of attempts for last 1 minute is greater than 100. So if account is no longer under active brute force attack, we can login. You can check my PR #155 - there're changes that move isThrottle check, to validate method. I think it's working quite well - I don't noticed any problems so far. So I think it's a good starter point for the next changes.

Lockout mechanism - i like this idea. I think that it could be (as well as brute force attack, and distributed brute force attact), a perfect place to implement one-time pass. When user want to login, and there is active attack on his account or entire system, which doesn't allow him to do so, we send special link that make it possible to skip throttling check. We send this link only when we notice correct attempt from user.

[lonnieezell]

Lockout mechanism - i like this idea. I think that it could be (as well as brute force attack, and distributed brute force attact), a perfect place to implement one-time pass.

I agree.

I'm not quite sure if I understood you correctly. We have two conditions here, so it's all ok:

if ($attempts > 100 && $this->ci->login_model->isBruteForced($ip_address, $user_id))

isBruteForced method, checks if number of attempts for last 1 minute is greater than 100. So if account is no longer under active brute force attack, we can login.

True, but then what happens is we check to see if there's any throttling time at all, just at the end of the isBruteForced method. If there's any reason for throttling at all (too many failed attempts), and we lock people solely based on throttling, before any authentication checks happen, then they'll hit a loop where it will constantly throttle them no matter if their credentials are good or not.

I think we've already come to a fix that satisfies both of us, though, in the max attempts check. I'll get that portion implemented today if I can. Then we can start to work on the other pieces.

[lonnieezell]

Well, could be longer. :) I got it implemented easily enough, but running tests on my system is throwing all kinds of weird errors now, so will have to tackle them later before I can upload with any semblance of certainty...

[michalsn]

I suppose you don't have a lot of free time lately, but... any progress on this one?

[lonnieezell]

Sorry. Kinda forgot to check back in on this one. I noticed you had a PR fixing some tests under PHP7 - do acceptance tests work for you? My acceptance are giving me parse failures currently, though unit tests all run fine.

[lonnieezell]

I got acceptance tests running - for some reason they decided that NOW they didn't like having a space in the directory name - and got all of them except for one running successfully again... I've pushed those changes and any previous change to the Auth stuff, into the repo now.

[michalsn]

Oh, I didn't notice that we have some acceptance tests in place :)

I had some errors... but I sent PRs that fixes them in my development environment. I hope it's gonna be working ok for you as well.

I've pushed those changes and any previous change to the Auth stuff, into the repo now.

I don't see any changes related to max attempts check. Is it ok?