Skip to content

Commit

Permalink
Fixed issue #06202 : SQL error with failed_login_attempts with IPv6
Browse files Browse the repository at this point in the history
Dev: need some test for security : due to privacy extensions, solution can be save only ipv4 or remove the privacy extension
Dev: TODO for 1.92
  • Loading branch information
Shnoulle committed Jun 13, 2012
1 parent 54fe2ee commit ef300ee
Show file tree
Hide file tree
Showing 8 changed files with 23 additions and 26 deletions.
2 changes: 1 addition & 1 deletion application/config/version.php
Expand Up @@ -13,7 +13,7 @@
*/

$config['versionnumber'] = "2.0RC2";
$config['dbversionnumber'] = 158;
$config['dbversionnumber'] = 159;
$config['buildnumber'] = '';
$config['updatable'] = false;

Expand Down
15 changes: 4 additions & 11 deletions application/controllers/admin/authentication.php
Expand Up @@ -33,8 +33,7 @@ class Authentication extends Survey_Common_Action
public function index()
{
$this->_redirectIfLoggedIn();
$sIp = Yii::app()->request->getUserHostAddress();
$bCanLogin = $this->_userCanLogin($sIp);
$bCanLogin = $this->_userCanLogin();

if ($bCanLogin && !is_array($bCanLogin))
{
Expand All @@ -44,7 +43,7 @@ public function index()

if (!isset($aData['errormsg']))
{
Failed_login_attempts::model()->deleteAttempts($sIp);
Failed_login_attempts::model()->deleteAttempts();

$this->getController()->_GetSessionUserRights(Yii::app()->session['loginID']);
Yii::app()->session['just_logged_in'] = true;
Expand Down Expand Up @@ -209,20 +208,14 @@ private function _redirectIfLoggedIn()

/**
* Check if a user can log in
* @param string $sIp IP Address
* @return bool|array
*/
private function _userCanLogin($sIp = '')
private function _userCanLogin()
{
if (empty($sIp))
{
$sIp = Yii::app()->request->getUserHostAddress();
}

$failed_login_attempts = Failed_login_attempts::model();
$failed_login_attempts->cleanOutOldAttempts();

if ($failed_login_attempts->isLockedOut($sIp))
if ($failed_login_attempts->isLockedOut())
{
return $this->_getAuthenticationFailedErrorMessage();
}
Expand Down
6 changes: 3 additions & 3 deletions application/controllers/admin/remotecontrol.php
Expand Up @@ -279,14 +279,14 @@ public function add_participants($session_key, $sid, $participant_data, $create_
*/
protected function _doLogin($sUsername, $sPassword)
{
if (Failed_login_attempts::model()->isLockedOut(Yii::app()->request->getUserHostAddress()))
if (Failed_login_attempts::model()->isLockedOut())
return false;

$identity = new UserIdentity(sanitize_user($sUsername), $sPassword);

if (!$identity->authenticate())
{
Failed_login_attempts::model()->addAttempt(Yii::app()->request->getUserHostAddress());
Failed_login_attempts::model()->addAttempt();
return false;
}
else
Expand Down Expand Up @@ -345,4 +345,4 @@ protected function _checkSessionKey($session_key)
return true;
}
}
}
}
6 changes: 6 additions & 0 deletions application/helpers/update/updatedb_helper.php
Expand Up @@ -995,6 +995,12 @@ function db_upgrade_all($oldversion) {
LimeExpressionManager::UpgradeConditionsToRelevance();
Yii::app()->db->createCommand()->update('{{settings_global}}',array('stg_value'=>158),"stg_name='DBVersion'");
}
if ($oldversion < 159)
{
alterColumn('{{failed_login_attempts}}', 'ip', "{$sVarchar}(40)",false);
Yii::app()->db->createCommand()->update('{{settings_global}}',array('stg_value'=>159),"stg_name='DBVersion'");
}


fixLanguageConsistencyAllSurveys();
echo '<br /><br />'.sprintf($clang->gT('Database update finished (%s)'),date('Y-m-d H:i:s')).'<br /><br />';
Expand Down
14 changes: 6 additions & 8 deletions application/models/Failed_login_attempts.php
Expand Up @@ -53,24 +53,23 @@ public function tableName()
* Deletes all the attempts by IP
*
* @access public
* @param string $ip
* @return void
*/
public function deleteAttempts($ip)
public function deleteAttempts()
{
$ip = substr(Yii::app()->request->getUserHostAddress(),0,40);
$this->deleteAllByAttributes(array('ip' => $ip));
}

/**
* Check if an IP address is allowed to login or not
*
* @param string $sIPAddress IP Address to check
* @return boolean Returns true if the user is blocked
*/
public function isLockedOut($ip)
public function isLockedOut()

This comment has been minimized.

Copy link
@olleharstedt

olleharstedt May 26, 2021

Contributor

I don't like this change. $ip should be an argument to the function, to make it testable, without mocking global app object. Also makes the signature of the function more clear.

This comment has been minimized.

Copy link
@olleharstedt

olleharstedt May 26, 2021

Contributor

Hm, 9 years old commit. 😄

This comment has been minimized.

Copy link
@Shnoulle

Shnoulle May 26, 2021

Author Collaborator

TODO for 1.92

Unsure it's done in 1.92 version …

{
$isLockedOut = false;

$ip = substr(Yii::app()->request->getUserHostAddress(),0,40);
$criteria = new CDbCriteria;
$criteria->condition = 'number_attempts > :attempts AND ip = :ip';
$criteria->params = array(':attempts' => Yii::app()->getConfig('maxLoginAttempt'), ':ip' => $ip);
Expand Down Expand Up @@ -103,13 +102,12 @@ public function cleanOutOldAttempts()
* Creates an attempt
*
* @access public
* @param string $ip
* @return true
*/
public function addAttempt($ip)
public function addAttempt()
{
$timestamp = date("Y-m-d H:i:s");

$ip = substr(Yii::app()->request->getUserHostAddress(),0,40);
$row = $this->findByAttributes(array('ip' => $ip));

if ($row !== null)
Expand Down
2 changes: 1 addition & 1 deletion installer/sql/create-mssql.sql
Expand Up @@ -82,7 +82,7 @@ CREATE TABLE [prefix_expression_errors] (
--
CREATE TABLE [prefix_failed_login_attempts] (
[id] int NOT NULL IDENTITY (1,1) PRIMARY KEY,
[ip] varchar(37) NOT NULL,
[ip] varchar(40) NOT NULL,
[last_attempt] varchar(20) NOT NULL,
[number_attempts] int NOT NULL
);
Expand Down
2 changes: 1 addition & 1 deletion installer/sql/create-mysql.sql
Expand Up @@ -82,7 +82,7 @@ CREATE TABLE `prefix_expression_errors` (
--
CREATE TABLE `prefix_failed_login_attempts` (
`id` int(11) NOT NULL AUTO_INCREMENT,
`ip` varchar(37) NOT NULL,
`ip` varchar(40) NOT NULL,
`last_attempt` varchar(20) NOT NULL,
`number_attempts` int(11) NOT NULL,
PRIMARY KEY (`id`)
Expand Down
2 changes: 1 addition & 1 deletion installer/sql/create-pgsql.sql
Expand Up @@ -91,7 +91,7 @@ CREATE TABLE prefix_expression_errors (
--
CREATE TABLE prefix_failed_login_attempts (
id serial PRIMARY KEY NOT NULL,
ip character varying(37) NOT NULL,
ip character varying(40) NOT NULL,
last_attempt character varying(20) NOT NULL,
number_attempts integer NOT NULL
);
Expand Down

0 comments on commit ef300ee

Please sign in to comment.