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

Improve handling of DDoS/brute force attacks on login form #6383

Open
wants to merge 1 commit into
base: 3.3.x
Choose a base branch
from

Conversation

rob006
Copy link

@rob006 rob006 commented Apr 12, 2022

Checklist:

  • Correct branch: master for new features; 3.3.x for fixes
  • Tests pass
  • Code follows coding guidelines: master and 3.3.x
  • Commit follows commit message format

Tracker ticket (set the ticket ID to your ticket ID):

https://tracker.phpbb.com/browse/PHPBB3-


Background:

I had a DDoS attack on login form. It quickly filled the sessions table with tens of thousands of sessions for the anonymous user. A lot of sessions for this user dramatically decreased the performance of SELECT COUNT(session_id) AS sessions FROM SESSIONS_TABLE ... query, since the efficiency of session_user_id index was almost non-existent (99% of sessions was for anonymous user, so the query needed to count almost all rows in table). I excluded this query for the anonymous user, since it was not needed anyway. This significantly improved performance of forum, but...

...sessions were still created and the sessions table was growing. I ignored this, but after a few days I realized that cron does not work correctly. It turns out that task for clearing old sessions tried to load hundreds of thousands of sessions in one query, which exceeded the memory limit, and the whole process failed (and this was repeated over and over since old sessions were never removed). I added a limit to this query, so sessions could be cleaned up in smaller batches (I'm still not sure if this limit is optimal, but it worked for me).

@3D-I
Copy link
Contributor

3D-I commented Apr 14, 2022

Plese open a ticket in the tracker, first. Then link it to your PR which should follow the guidelines, thanks..

@@ -157,7 +157,8 @@ function garbage_collect($type)
FROM ' . CONFIRM_TABLE . ' c
LEFT JOIN ' . SESSIONS_TABLE . ' s ON (c.session_id = s.session_id)
WHERE s.session_id IS NULL' .
((empty($type)) ? '' : ' AND c.confirm_type = ' . (int) $type);
((empty($type)) ? '' : ' AND c.confirm_type = ' . (int) $type)
. ' LIMIT 100000';
$result = $db->sql_query($sql);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be using sql_query_limit() instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants