From: VirtualTam <virtualtam@flibidi.net>
Date: Fri, 16 Feb 2018 20:51:44 +0000 (+0100)
Subject: Refactor client session hijacking protection
X-Git-Tag: v0.10.0~23^2~14
X-Git-Url: https://git.immae.eu/?p=github%2Fshaarli%2FShaarli.git;a=commitdiff_plain;h=88110550b89617dcda16441212599b8a40faa20c

Refactor client session hijacking protection

Signed-off-by: VirtualTam <virtualtam@flibidi.net>
---

diff --git a/application/HttpUtils.php b/application/HttpUtils.php
index 83a4c5e2..e9282506 100644
--- a/application/HttpUtils.php
+++ b/application/HttpUtils.php
@@ -1,7 +1,7 @@
 <?php
 /**
  * GET an HTTP URL to retrieve its content
- * Uses the cURL library or a fallback method 
+ * Uses the cURL library or a fallback method
  *
  * @param string          $url               URL to get (http://...)
  * @param int             $timeout           network timeout (in seconds)
@@ -415,6 +415,37 @@ function getIpAddressFromProxy($server, $trustedIps)
     return array_pop($ips);
 }
 
+
+/**
+ * Return an identifier based on the advertised client IP address(es)
+ *
+ * This aims at preventing session hijacking from users behind the same proxy
+ * by relying on HTTP headers.
+ *
+ * See:
+ * - https://secure.php.net/manual/en/reserved.variables.server.php
+ * - https://stackoverflow.com/questions/3003145/how-to-get-the-client-ip-address-in-php
+ * - https://stackoverflow.com/questions/12233406/preventing-session-hijacking
+ * - https://stackoverflow.com/questions/21354859/trusting-x-forwarded-for-to-identify-a-visitor
+ *
+ * @param array $server The $_SERVER array
+ *
+ * @return string An identifier based on client IP address information
+ */
+function client_ip_id($server)
+{
+    $ip = $server['REMOTE_ADDR'];
+
+    if (isset($server['HTTP_X_FORWARDED_FOR'])) {
+        $ip = $ip . '_' . $server['HTTP_X_FORWARDED_FOR'];
+    }
+    if (isset($server['HTTP_CLIENT_IP'])) {
+        $ip = $ip . '_' . $server['HTTP_CLIENT_IP'];
+    }
+    return $ip;
+}
+
+
 /**
  * Returns true if Shaarli's currently browsed in HTTPS.
  * Supports reverse proxies (if the headers are correctly set).
diff --git a/index.php b/index.php
index 2fe3f821..08a69327 100644
--- a/index.php
+++ b/index.php
@@ -207,7 +207,7 @@ function setup_login_state($conf)
     }
     // If session does not exist on server side, or IP address has changed, or session has expired, logout.
     if (empty($_SESSION['uid'])
-        || ($conf->get('security.session_protection_disabled') === false && $_SESSION['ip'] != allIPs())
+        || ($conf->get('security.session_protection_disabled') === false && $_SESSION['ip'] != client_ip_id($_SERVER))
         || time() >= $_SESSION['expires_on'])
     {
         logout();
@@ -231,16 +231,6 @@ $userIsLoggedIn = setup_login_state($conf);
 // ------------------------------------------------------------------------------------------
 // Session management
 
-// Returns the IP address of the client (Used to prevent session cookie hijacking.)
-function allIPs()
-{
-    $ip = $_SERVER['REMOTE_ADDR'];
-    // Then we use more HTTP headers to prevent session hijacking from users behind the same proxy.
-    if (isset($_SERVER['HTTP_X_FORWARDED_FOR'])) { $ip=$ip.'_'.$_SERVER['HTTP_X_FORWARDED_FOR']; }
-    if (isset($_SERVER['HTTP_CLIENT_IP'])) { $ip=$ip.'_'.$_SERVER['HTTP_CLIENT_IP']; }
-    return $ip;
-}
-
 /**
  * Load user session.
  *
@@ -249,7 +239,7 @@ function allIPs()
 function fillSessionInfo($conf)
 {
     $_SESSION['uid'] = sha1(uniqid('',true).'_'.mt_rand()); // Generate unique random number (different than phpsessionid)
-    $_SESSION['ip']=allIPs();                // We store IP address(es) of the client to make sure session is not hijacked.
+    $_SESSION['ip'] = client_ip_id($_SERVER);
     $_SESSION['username']= $conf->get('credentials.login');
     $_SESSION['expires_on']=time()+INACTIVITY_TIMEOUT;  // Set session expiration.
 }
diff --git a/tests/HttpUtils/ClientIpIdTest.php b/tests/HttpUtils/ClientIpIdTest.php
new file mode 100644
index 00000000..c15ac5cc
--- /dev/null
+++ b/tests/HttpUtils/ClientIpIdTest.php
@@ -0,0 +1,52 @@
+<?php
+/**
+ * HttpUtils' tests
+ */
+
+require_once 'application/HttpUtils.php';
+
+/**
+ * Unitary tests for client_ip_id()
+ */
+class ClientIpIdTest extends PHPUnit_Framework_TestCase
+{
+    /**
+     * Get a remote client ID based on its IP
+     */
+    public function testClientIpIdRemote()
+    {
+        $this->assertEquals(
+            '10.1.167.42',
+            client_ip_id(['REMOTE_ADDR' => '10.1.167.42'])
+        );
+    }
+
+    /**
+     * Get a remote client ID based on its IP and proxy information (1)
+     */
+    public function testClientIpIdRemoteForwarded()
+    {
+        $this->assertEquals(
+            '10.1.167.42_127.0.1.47',
+            client_ip_id([
+                'REMOTE_ADDR' => '10.1.167.42',
+                'HTTP_X_FORWARDED_FOR' => '127.0.1.47'
+            ])
+        );
+    }
+
+    /**
+     * Get a remote client ID based on its IP and proxy information (2)
+     */
+    public function testClientIpIdRemoteForwardedClient()
+    {
+        $this->assertEquals(
+            '10.1.167.42_10.1.167.56_127.0.1.47',
+            client_ip_id([
+                'REMOTE_ADDR' => '10.1.167.42',
+                'HTTP_X_FORWARDED_FOR' => '10.1.167.56',
+                'HTTP_CLIENT_IP' => '127.0.1.47'
+            ])
+        );
+    }
+}