Uploaded image for project: 'phpBB3'
  1. phpBB3
  2. PHPBB3-10247

mediumint(8) too small for phpbb_login_attempts.attempt_id

    Details

    • Type: Bug
    • Status: Unverified Fix
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.9-RC3
    • Fix Version/s: 3.0.9-RC4
    • Component/s: None
    • Labels:
      None

      Description

      Mediumint is 3 bytes in MySQL. Assuming there are 25000 failed login attempts per day, we will run out of free attempt_id values after

      (2^(3×8)÷25000)÷365 = ~2

      years.

      Solution is to either drop attempt_id if it is not needed or to change it to 4 byte integer.

      Considering half a million failed login attempts per day and four bytes:

      (2^(4×8)÷500000)÷365 = ~24 years

        Issue Links

          Activity

          Hide
          naderman Nils Adermann added a comment -

          The current patch fixes this issue for new installs. The database updater works only on databases that allow you to drop the primary key column without further work (e.g. MySQL). Other databases such as Firebird might require an additional operation to remove contraints (such as primary key) from the column before dropping it. The key/column dropping code in db_tools needs to be reviewed & tested on all databases.

          Show
          naderman Nils Adermann added a comment - The current patch fixes this issue for new installs. The database updater works only on databases that allow you to drop the primary key column without further work (e.g. MySQL). Other databases such as Firebird might require an additional operation to remove contraints (such as primary key) from the column before dropping it. The key/column dropping code in db_tools needs to be reviewed & tested on all databases.
          Hide
          naderman Nils Adermann added a comment -

          The current version of db_tools cannot reliably drop a primary key column. So we removed the addition of the column from the update process altogether. Boards that were updated to a 3.0.9 RC will have the column removed with 3.0.10-RC1. The size should suffice to last until 3.0.10-RC1 becomes available.

          Show
          naderman Nils Adermann added a comment - The current version of db_tools cannot reliably drop a primary key column. So we removed the addition of the column from the update process altogether. Boards that were updated to a 3.0.9 RC will have the column removed with 3.0.10-RC1. The size should suffice to last until 3.0.10-RC1 becomes available.
          Hide
          nickvergessen Joas Schilling added a comment -

          Database type :: mysqli
          Previous version :: 3.0.8
          Updated version :: 3.0.9

          Updating database schema

          Progress :: . . . . . . . . Done
          Result :: Some queries failed, the statements and errors are listing below.

          Error :: Key column 'attempt_id' doesn't exist in table
          SQL :: CREATE TABLE phpbb_login_attempts ( attempt_ip varchar(40) DEFAULT '' NOT NULL, attempt_browser varchar(150) DEFAULT '' NOT NULL, attempt_forwarded_for varchar(255) DEFAULT '' NOT NULL, attempt_time int(11) UNSIGNED DEFAULT '0' NOT NULL, user_id mediumint(8) UNSIGNED DEFAULT '0' NOT NULL, username varchar(255) DEFAULT '0' NOT NULL, username_clean varchar(255) DEFAULT '0' NOT NULL, PRIMARY KEY (attempt_id) ) CHARACTER SET `utf8` COLLATE `utf8_bin`;

          Error :: Table 'phpbb-3.0.9.phpbb_login_attempts' doesn't exist
          SQL :: CREATE INDEX att_ip ON phpbb_login_attempts(attempt_ip, attempt_time)

          Error :: Table 'phpbb-3.0.9.phpbb_login_attempts' doesn't exist
          SQL :: CREATE INDEX att_for ON phpbb_login_attempts(attempt_forwarded_for, attempt_time)

          Error :: Table 'phpbb-3.0.9.phpbb_login_attempts' doesn't exist
          SQL :: CREATE INDEX att_time ON phpbb_login_attempts(attempt_time)

          Error :: Table 'phpbb-3.0.9.phpbb_login_attempts' doesn't exist
          SQL :: CREATE INDEX user_id ON phpbb_login_attempts(user_id)

          the problem is, the attempts_id is still listed as primary key in database_update.php

          					'PRIMARY_KEY'		=> 'attempt_id',

          therefor creation fails and produces the rest of errors.

          Show
          nickvergessen Joas Schilling added a comment - Database type :: mysqli Previous version :: 3.0.8 Updated version :: 3.0.9 Updating database schema Progress :: . . . . . . . . Done Result :: Some queries failed, the statements and errors are listing below. Error :: Key column 'attempt_id' doesn't exist in table SQL :: CREATE TABLE phpbb_login_attempts ( attempt_ip varchar(40) DEFAULT '' NOT NULL, attempt_browser varchar(150) DEFAULT '' NOT NULL, attempt_forwarded_for varchar(255) DEFAULT '' NOT NULL, attempt_time int(11) UNSIGNED DEFAULT '0' NOT NULL, user_id mediumint(8) UNSIGNED DEFAULT '0' NOT NULL, username varchar(255) DEFAULT '0' NOT NULL, username_clean varchar(255) DEFAULT '0' NOT NULL, PRIMARY KEY (attempt_id) ) CHARACTER SET `utf8` COLLATE `utf8_bin`; Error :: Table 'phpbb-3.0.9.phpbb_login_attempts' doesn't exist SQL :: CREATE INDEX att_ip ON phpbb_login_attempts(attempt_ip, attempt_time) Error :: Table 'phpbb-3.0.9.phpbb_login_attempts' doesn't exist SQL :: CREATE INDEX att_for ON phpbb_login_attempts(attempt_forwarded_for, attempt_time) Error :: Table 'phpbb-3.0.9.phpbb_login_attempts' doesn't exist SQL :: CREATE INDEX att_time ON phpbb_login_attempts(attempt_time) Error :: Table 'phpbb-3.0.9.phpbb_login_attempts' doesn't exist SQL :: CREATE INDEX user_id ON phpbb_login_attempts(user_id) the problem is, the attempts_id is still listed as primary key in database_update.php 'PRIMARY_KEY' => 'attempt_id', therefor creation fails and produces the rest of errors.
          Hide
          nickvergessen Joas Schilling added a comment -

          Second issue, when the files are already updated, but database isn't it produces the following error:

          General Error
          SQL ERROR [ mysqli ]
           
          Table 'phpbb-3.0.9.phpbb_login_attempts' doesn't exist [1146]
           
          SQL
           
          SELECT COUNT(*) AS attempts FROM phpbb_login_attempts WHERE attempt_time > 1309870131 AND attempt_ip = '127.0.0.1'

          I think we should not display this error, but use return_on_error, just like we do on the session.php with code for 3.0.2:

          							$db->sql_return_on_error(true);
           
          							$sql = 'UPDATE ' . SESSIONS_TABLE . ' SET ' . $db->sql_build_array('UPDATE', $sql_ary) . "
          								WHERE session_id = '" . $db->sql_escape($this->session_id) . "'";
          							$result = $db->sql_query($sql);
           
          							$db->sql_return_on_error(false);
           
          							// If the database is not yet updated, there will be an error due to the session_forum_id
          							// @todo REMOVE for 3.0.2
          							if ($result === false)
          							{
          								unset($sql_ary['session_forum_id']);
           
          								$sql = 'UPDATE ' . SESSIONS_TABLE . ' SET ' . $db->sql_build_array('UPDATE', $sql_ary) . "
          									WHERE session_id = '" . $db->sql_escape($this->session_id) . "'";
          								$db->sql_query($sql);
          							}

          Show
          nickvergessen Joas Schilling added a comment - Second issue, when the files are already updated, but database isn't it produces the following error: General Error SQL ERROR [ mysqli ]   Table 'phpbb-3.0.9.phpbb_login_attempts' doesn't exist [1146]   SQL   SELECT COUNT(*) AS attempts FROM phpbb_login_attempts WHERE attempt_time > 1309870131 AND attempt_ip = '127.0.0.1' I think we should not display this error, but use return_on_error, just like we do on the session.php with code for 3.0.2: $db->sql_return_on_error(true);   $sql = 'UPDATE ' . SESSIONS_TABLE . ' SET ' . $db->sql_build_array('UPDATE', $sql_ary) . " WHERE session_id = '" . $db->sql_escape($this->session_id) . "'"; $result = $db->sql_query($sql);   $db->sql_return_on_error(false);   // If the database is not yet updated, there will be an error due to the session_forum_id // @todo REMOVE for 3.0.2 if ($result === false) { unset($sql_ary['session_forum_id']);   $sql = 'UPDATE ' . SESSIONS_TABLE . ' SET ' . $db->sql_build_array('UPDATE', $sql_ary) . " WHERE session_id = '" . $db->sql_escape($this->session_id) . "'"; $db->sql_query($sql); }
          Hide
          nickvergessen Joas Schilling added a comment -

          Suggested fix for both issues: https://gist.github.com/1064821

          Show
          nickvergessen Joas Schilling added a comment - Suggested fix for both issues: https://gist.github.com/1064821
          Hide
          bantu Andreas Fischer added a comment -

          I think we should not display this error, but use return_on_error

          Disagree, let's just fail properly instead of silently.

          Show
          bantu Andreas Fischer added a comment - I think we should not display this error, but use return_on_error Disagree, let's just fail properly instead of silently.

            People

            • Assignee:
              nickvergessen Joas Schilling
              Reporter:
              bantu Andreas Fischer
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development