Fixed issue: [security] Unsafe way to detect IP address against brute…

…-force attacks
LimeSurvey · May 30, 2023 · 6575099 · 6575099
1 parent 45c166e
commit 6575099
Show file tree

Hide file tree

Showing 4 changed files with 21 additions and 5 deletions.
diff --git a/application/extensions/LimeDebug/LimeDebug.php b/application/extensions/LimeDebug/LimeDebug.php
@@ -7,7 +7,7 @@ class LimeDebug extends CWidget
     {
         public function run()
         {
-            if (YII_DEBUG && in_array(getIPAddress(), array("127.0.0.1","::1")))
+            if (YII_DEBUG && in_array(getRealIPAddress(), array("127.0.0.1","::1")))
             {                
                 $data = array(
                     'session' => $_SESSION,

diff --git a/application/helpers/common_helper.php b/application/helpers/common_helper.php
@@ -4610,6 +4610,22 @@ function getIPAddress()
     return $sIPAddress;
 }
 
+
+/**
+ * This function returns the real IP address and should mainly be used for security sensitive purposes
+ * If you want to use the IP address for language detection or similar, use getIPAddress() instead
+ * 
+ * @return  string  Client IP Address
+ */
+function getRealIPAddress()
+{
+    $sIPAddress = '127.0.0.1';
+    if (!empty($_SERVER['REMOTE_ADDR']) && filter_var($_SERVER['REMOTE_ADDR'], FILTER_VALIDATE_IP) !== false) {
+        $sIPAddress = $_SERVER['REMOTE_ADDR'];
+    }
+    return $sIPAddress;
+}
+
 /**
 * This function tries to find out a valid language code for the language of the browser used
 * If it cannot find it it will return the default language from global settings

diff --git a/application/models/FailedLoginAttempt.php b/application/models/FailedLoginAttempt.php
@@ -63,7 +63,7 @@ public function tableName()
      */
     public function deleteAttempts(string $attemptType = 'login')
     {
-        $ip = substr(getIPAddress(), 0, 40);
+        $ip = substr(getRealIPAddress(), 0, 40);
 
         if (Yii::app()->getConfig('DBVersion') <= 480) {
             $this->deleteAllByAttributes(array('ip' => $ip));
@@ -82,7 +82,7 @@ public function deleteAttempts(string $attemptType = 'login')
     public function isLockedOut(string $attemptType): bool
     {
         $isLockedOut = false;
-        $ip = substr(getIPAddress(), 0, 40);
+        $ip = substr(getRealIPAddress(), 0, 40);
 
         // Return false if IP is whitelisted
         if ($this->isWhitelisted($ip, $attemptType)) {
@@ -144,7 +144,7 @@ public function addAttempt($attemptType = self::TYPE_LOGIN)
     {
         if (!$this->isLockedOut($attemptType)) {
             $timestamp = date("Y-m-d H:i:s");
-            $ip = substr(getIPAddress(), 0, 40);
+            $ip = substr(getRealIPAddress(), 0, 40);
 
             if (Yii::app()->getConfig('DBVersion') <= 480) {
                 $row = $this->findByAttributes(array('ip' => $ip));

diff --git a/tests/unit/models/FailedLoginAttemptTest.php b/tests/unit/models/FailedLoginAttemptTest.php
@@ -9,7 +9,7 @@ class FailedLoginAttempTest extends TestBaseClass
     public function testAddDeleteAttemp()
     {
         // Save Ip
-        $ip = substr(getIPAddress(), 0, 40);
+        $ip = substr(getRealIPAddress(), 0, 40);
 
         // Make sure there are no records for the ip
         FailedLoginAttempt::model()->deleteAttempts(FailedLoginAttempt::TYPE_LOGIN);