search

emreakay / CodeIgniter-Aauth

Authorization, Authentication and User Management library for Codeigniter 2.x and 3.x to make easy user management and permission operations
http://emreakay.com
GNU Lesser General Public License v3.0
392 stars 235 forks source link

reCAPTCHA not working in 2.5 #150

Closed paulcanning closed 8 years ago

paulcanning commented 8 years ago

With the changes to the login process with DDOS etc, it looks like reCAPTCHA is broken.

The login attempts have now been moved to a seperate table, yet the code which sets the reCAPTCHA cookie or session is looking at the old login_attempts column in the user table.

Line 188 in the Aauth.php file.

REJack commented 8 years ago

i check this soon

paulcanning commented 8 years ago

Seems there is no way to link a user to the new last login attempt table?

Also, the same login attempt row is being updated despite me trying to log in with different users. Seems to be matching the IP only?

REJack commented 8 years ago

its not need to check DDoS based on User ID's or so, i forgot to switch $row->login_attempts with $this->update_login_attempts(). fixed in the new release.

paulcanning commented 8 years ago

Will check it now.

paulcanning commented 8 years ago

The problem now, is that there is no last login time in the user table, which is useful to check a users activity level.

paulcanning commented 8 years ago

OK, so it looks like the reCAPTCHA form is being shown after the first incorrect password entry, despite the config item, recaptcha_login_attempts being set to 3.

paulcanning commented 8 years ago

Think the problem is that you are doing $this->update_login_attempts() >= $this->config_vars['recaptcha_login_attempts'] when update_login_attempts() returns TRUE or FALSE, not an integer

REJack commented 8 years ago

oh yeah sh**, i fix it later, sry :tired_face:

REJack commented 8 years ago

i fixed this now with v2.5.4, tested it too and it works :smile: .

REJack commented 8 years ago

did you tested the new release? :smiley:

paulcanning commented 8 years ago

Just about to download and try now.

paulcanning commented 8 years ago

OK, so it does work. However, there are still some issues with this whole thing.

Firstly, the number of login attempts is still only incremented when someone puts in a (wrong) password that is between the min and max characters allowed. This means reCAPTCHA is not show if all login attempts use a password guess less then or more then the config values.

Secondly, lets say I tried to log in twice, and my config value for recaptcha_login_attempts is 3. When I go to actually enter the password on the 3rd attempt, there is no reCAPTCHA yet, as 2 < 3, obviously.

But when the 3rd attempt fails, I am shown the error message

Sorry, the reCAPTCHA text entered was incorrect.

and also shown the reCAPTCHA box. This just doesn't sit well with me. You cannot have a message saying "sorry, you didn't fill out the text" when it wasn't there to fill in. (also, just to note, reCAPTCHA is now usually just a tick box and may involved selecting images, it rarely uses a text input)

So yea, you fixed one thing, but other bits or still broken, sadly.

REJack commented 8 years ago
  1. yeah i see what you mean i need to move the if in L162 to/before L145
  2. I'm thinking about to remove the cookie/session-userdata for the recaptcha, then i can trigger it better without it the error message.

reCAPTCHA v2 are loaded over javascript and doesn't has/create a input field with required attribute, but you can create a form validation,

a way over javascript if reCAPTCHA is shown #recaptcha-anchor get a attribute on check/validation "aria-checked"="true" this can be checked over a javascript

i fix this stuff soon :smiley:

REJack commented 8 years ago

ok now i hope reCAPTCHA works flawless :sweat_smile: i found a massive bug in the *_login_attempts functions too.

v2.5.5

paulcanning commented 8 years ago

Hmm, still some issues.

Basically, the usage of IP is good, but now that the login attempts are disconnected from the user, it's locking out all user access for that IP (again, this is kinda good, kinda bad lol)

I think you're on the right track, just need to smooth it over :)

paulcanning commented 8 years ago

I think the issue regarding resetting the login attempts is caused by the reset_login_attempts method.

Seems the logic for the where part of the query is buggy. You are checking for a timestamp that is more then, or equal to, the current datetime, minus the login attempt period.

Lets say my last login attempt was at 15:00. And the login retry time period was 10 minutes.

If I came back and logged in successfully at 15:50, the login attempts would not be cleared, as you are looking for a timestamp of 15:40 in the where part of your query. and 15:00 is less then 15:40, not more then or equal to.

https://github.com/emreakay/CodeIgniter-Aauth/blob/master/application/libraries/Aauth.php#L494

paulcanning commented 8 years ago

Messing around, I now have two rows in my login attempts table, both with the same IP, but different timestamps and login attempts.

the whole IP login attempt thing is just looking really bad right now :(

REJack commented 8 years ago

Seeing as login attempts are currently tied to IP (which is good and bad, see point 1) when entering an email that does not exist or just random stuff for the login identifier, the login attempts are not increased.

That's fixed with v2.5.5, i moved the DDoS check before user check. I tested the whole DDoS protection without correct login data, its not needed and makes no sense to tie it to a user. (see next answer)

Still, login attempts are not tied to users. I can spam one known account email to try log in, and after it being locked, I am unable to log in to another account.

I wouldn't tie it to users and that's a right behavior, if a brute forcer attacks the page with 1 user login and would change the login data, the brute forcer wouldn't get locked and could continue brute forcing.

After waiting for the max_login_attempt_time_period , when I successfully log in to an account, the login attempts are not cleared, despite remove_successful_attempts being set to TRUE

If you wait after max_login_attempt_time_period you begin a new login_attempt so the old one stays in the database, right behavior too.

Messing around, I now have two rows in my login attempts table, both with the same IP, but different timestamps and login attempts.

That's a right behavior too, only thing what i missed to add get_login_attempts() to get a list of all login attempts with that a webmaster can block ip addresses that look like a brute forcer or check in users db over the ip_address-field if the brute forcer cracked a password/account

i need to add a function to remove old login_attempts (older than X) for usage over a cron job, like cleanup_pms()

If a user login successfully he removes his attempt, only attempts from unsuccessfully logins are in the database,

From the performance side there is no problem with leaving false attempts in the database. i can add a config option to clean old login_attempts on successful login (older than max_login_attempt_time_period), i personally would never use this, because i could not look for brute forcer and ban them over my firewall/etc.

please take this not as an attack, i have experience with bypassing DDoS Protections so i doesn't want to create a bypass-able DDoS protection.

paulcanning commented 8 years ago

Thanks for explaining :)

Like I said, it seems to be much better now!

Oh, one improvement - maybe add the max_login_attempt_time_period into the error message shown to the user? So it actually says eg "You are locked out for X minutes"?

REJack commented 8 years ago

Oh, one improvement - maybe add the max_login_attempt_time_period into the error message shown to the user? So it actually says eg "You are locked out for X minutes"?

This improvement is a part of v3.0.0 :smile:

My plans are:

  • write from scratch in MVC (move DB actions into models)
    • ...
    • enhance language lines with sprintf

this would break other translations i could change only german & english but the other translations i cant change, that's why i wouldn't improve it in v2.5.

reCAPTCHA works now flawless? if yes can you close this issue :smiley:

paulcanning commented 8 years ago

Yes, it looks like it is activating at the right time and working. I'll close this issue.