Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic prevention from brute-force attacks #4913

Closed
rcubetrac opened this issue Sep 25, 2015 · 14 comments

Comments

Projects
None yet
2 participants
@rcubetrac
Copy link

commented Sep 25, 2015

Reported by @alecpl on 25 Sep 2015 06:55 UTC as Trac ticket #1490549

We have several plugins to aid in preventing such attacks from being successful, as well as OTP plugins, and typical deployments allow the user to be locked out in centralized infrastructure not just Roundcube itself (i.e. LDAP).

To protect roundcube core better against such attacks we could/should:

  1. make sure the session/token is regenerated after failed login.
  2. perhaps temporarily block the user account.

Migrated-From: http://trac.roundcube.net/ticket/1490549

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Sep 28, 2015

Comment by @alecpl on 28 Sep 2015 06:44 UTC

Looks like the first issue is already resolved in git-master. The token is different on each request after failed login. We should backport this to 1.1.
As for the second point, real brute-force prevention is not a simple task, as we already have plugins for this, I'd keep this functionality in plugins.

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Sep 28, 2015

Comment by @alecpl on 28 Sep 2015 06:59 UTC

Fixed in 3d9798d.

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Sep 28, 2015

Status changed by @alecpl on 28 Sep 2015 06:59 UTC

new => closed

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Sep 30, 2015

Comment by @alecpl on 30 Sep 2015 06:59 UTC

Is this really a fix? I gave it more thought and in my opinion re-generating the token or even session ID does nothing. We send the new values in the response to the login request, so attacker can just use the new values and continue password guessing. The only fix for such attacks is to delay responses/lock account after some failed attempts. Question is if that should be done in core or can be a plugin. I'm reopening to give it more consideration.

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Sep 30, 2015

Status changed by @alecpl on 30 Sep 2015 06:59 UTC

closed => reopened

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Sep 30, 2015

Comment by @alecpl on 30 Sep 2015 07:08 UTC

BTW, the simplest solution would be to store last-failed-login timestamp in users table. On failed login check that timestamp, if last failed login happened not more than 1 minute ago do sleep(5) before responding.

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Oct 12, 2015

Comment by lv01jsi on 12 Oct 2015 08:30 UTC

How about displaying captcha after 2-3 (configurable) failed logins?

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Oct 16, 2015

Comment by @thomascube on 16 Oct 2015 15:00 UTC

Maybe adding sleep(1) or usleep(500000) on failed login attempts would be a sane addition. Using a last-failed-login timestamp seems like a good proposition and with a major release, database schema changes are acceptable.

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Oct 17, 2015

Comment by @alecpl on 17 Oct 2015 11:39 UTC

Done in c1bbf0d, ticket for 1.2 created as #1490566.

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Oct 17, 2015

Status changed by @alecpl on 17 Oct 2015 11:39 UTC

reopened => closed

@rcubetrac rcubetrac closed this Oct 17, 2015

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Dec 27, 2015

Comment by martijn brinkers on 27 Dec 2015 08:38 UTC

I think a server side sleep of 1 second might make the server an easier target for a DOS. If an attacker tries to login with multiple accounts at the same time, might this not take up all resources from the server because every connection sleeps for 1 second?

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Dec 28, 2015

Comment by @thomascube on 28 Dec 2015 16:56 UTC

Replying to martijn.brinkers:

I think a server side sleep of 1 second might make the server an easier target for a DOS. If an attacker tries to login with multiple accounts at the same time, might this not take up all resources from the server because every connection sleeps for 1 second?

That depends on the webserver you're using but generally speaking: yes. It however cuts the DOS attack off on a webserver level and doesn't propagate it to the backend IMAP server. So it might bring down your frontend node(s) running Roundcube but keeps the rest of your email stack alive.

A more sophisticated brute-force prevention comes with Roundcube 1.2. See the forked ticket #1490566.

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Dec 29, 2015

Comment by martijn brinkers on 29 Dec 2015 08:31 UTC

Replying to thomasb:

That depends on the webserver you're using but generally speaking: yes. It however cuts the DOS attack off on a webserver level and doesn't propagate it to the backend IMAP server. So it might bring down your frontend node(s) running Roundcube but keeps the rest of your email stack alive.

A user might not be really interested in why he/she cannot login. If webmail is not accessible for whatever reason, the user will be very unhappy. With sleep(1) it is pretty easy to DOS the service with only one client.

A more sophisticated brute-force prevention comes with Roundcube 1.2. See the forked ticket #1490566.

I'm not sure the fix in #1490566 really helps. The check for multiple logins is done per user (which is a must imho). The DOS attack however is still possible if I'm not mistaken because the attacker can just try to login with ever changing fake names.

Personally I think that the sleep(1) should be removed (in the 1.2 and 1.1 release).

@rcubetrac rcubetrac added this to the 1.1.4 milestone Mar 20, 2016

@alecpl

This comment has been minimized.

Copy link
Member

commented Sep 13, 2016

The vulnerability was discovered by Fortinet’sFortiGuard Labs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.