From 6058d3305f82cec7e0d149e4d93d26706a9617e5 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 5 Dec 2016 10:25:18 -0800
Subject: [PATCH] Normalize remote IP addresses when writing to logs, etc

Summary:
Ref T11939. IPv4 addresses can normally only be written in one way, but IPv6 addresses have several formats.

For example, the addresses "FFF::", "FfF::", "fff::", "0ffF::", "0fFf:0::", and "0FfF:0:0:0:0:0:0:0" are all the same address.

Normalize all addresses before writing them to logs, etc, so we store the most-preferred form ("fff::", above).

Test Plan:
Ran an SSH clone over IPv6:

```
$ git fetch ssh://local@::1/diffusion/26/locktopia.git
```

It worked; verified that address read out of `SSH_CLIENT` sensibly.

Faked my remote address as a non-preferred-form IPv6 address using `preamble.php`.

Failed to login, verified that the preferred-form version of the address appeared in the user activity log.

Made IPv6 requests over HTTP:

```
$ curl -H "Host: local.phacility.com" "http://[::1]/"
```

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11939

Differential Revision: https://secure.phabricator.com/D16987
---
 src/aphront/AphrontRequest.php                  |  8 +++++---
 .../AphrontApplicationConfiguration.php         | 10 +++++++++-
 .../people/storage/PhabricatorUserLog.php       | 16 +++++++++++++---
 src/infrastructure/env/PhabricatorEnv.php       | 17 +++++++++++++++--
 .../ssh/PhabricatorSSHWorkflow.php              |  8 +++++++-
 5 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/src/aphront/AphrontRequest.php b/src/aphront/AphrontRequest.php
index 6045c0d728..ea0313e2a3 100644
--- a/src/aphront/AphrontRequest.php
+++ b/src/aphront/AphrontRequest.php
@@ -557,11 +557,13 @@ final class AphrontRequest extends Phobject {
   }
 
   public function getRemoteAddress() {
-    $address = $_SERVER['REMOTE_ADDR'];
-    if (!strlen($address)) {
+    $address = PhabricatorEnv::getRemoteAddress();
+
+    if (!$address) {
       return null;
     }
-    return substr($address, 0, 64);
+
+    return $address->getAddress();
   }
 
   public function isHTTPS() {
diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php
index bcbc325b87..bbfcaf0b46 100644
--- a/src/aphront/configuration/AphrontApplicationConfiguration.php
+++ b/src/aphront/configuration/AphrontApplicationConfiguration.php
@@ -106,10 +106,18 @@ abstract class AphrontApplicationConfiguration extends Phobject {
     PhabricatorAccessLog::init();
     $access_log = PhabricatorAccessLog::getLog();
     PhabricatorStartup::setAccessLog($access_log);
+
+    $address = PhabricatorEnv::getRemoteAddress();
+    if ($address) {
+      $address_string = $address->getAddress();
+    } else {
+      $address_string = '-';
+    }
+
     $access_log->setData(
       array(
         'R' => AphrontRequest::getHTTPHeader('Referer', '-'),
-        'r' => idx($_SERVER, 'REMOTE_ADDR', '-'),
+        'r' => $address_string,
         'M' => idx($_SERVER, 'REQUEST_METHOD', '-'),
       ));
 
diff --git a/src/applications/people/storage/PhabricatorUserLog.php b/src/applications/people/storage/PhabricatorUserLog.php
index 9727c81c63..82819f8f5b 100644
--- a/src/applications/people/storage/PhabricatorUserLog.php
+++ b/src/applications/people/storage/PhabricatorUserLog.php
@@ -108,18 +108,28 @@ final class PhabricatorUserLog extends PhabricatorUserDAO
     $log->setUserPHID((string)$object_phid);
     $log->setAction($action);
 
-    $log->remoteAddr = (string)idx($_SERVER, 'REMOTE_ADDR', '');
+    $address = PhabricatorEnv::getRemoteAddress();
+    if ($address) {
+      $log->remoteAddr = $address->getAddress();
+    } else {
+      $log->remoteAddr = '';
+    }
 
     return $log;
   }
 
   public static function loadRecentEventsFromThisIP($action, $timespan) {
+    $address = PhabricatorEnv::getRemoteAddress();
+    if (!$address) {
+      return array();
+    }
+
     return id(new PhabricatorUserLog())->loadAllWhere(
       'action = %s AND remoteAddr = %s AND dateCreated > %d
         ORDER BY dateCreated DESC',
       $action,
-      idx($_SERVER, 'REMOTE_ADDR'),
-      time() - $timespan);
+      $address->getAddress(),
+      PhabricatorTime::getNow() - $timespan);
   }
 
   public function save() {
diff --git a/src/infrastructure/env/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php
index 15873a7f9c..b50c85c0f0 100644
--- a/src/infrastructure/env/PhabricatorEnv.php
+++ b/src/infrastructure/env/PhabricatorEnv.php
@@ -818,12 +818,12 @@ final class PhabricatorEnv extends Phobject {
       return false;
     }
 
-    $address = idx($_SERVER, 'REMOTE_ADDR');
+    $address = self::getRemoteAddress();
     if (!$address) {
       throw new Exception(
         pht(
           'Unable to test remote address against cluster whitelist: '.
-          'REMOTE_ADDR is not defined.'));
+          'REMOTE_ADDR is not defined or not valid.'));
     }
 
     return self::isClusterAddress($address);
@@ -844,6 +844,19 @@ final class PhabricatorEnv extends Phobject {
       ->containsAddress($address);
   }
 
+  public static function getRemoteAddress() {
+    $address = idx($_SERVER, 'REMOTE_ADDR');
+    if (!$address) {
+      return null;
+    }
+
+    try {
+      return PhutilIPAddress::newAddress($address);
+    } catch (Exception $ex) {
+      return null;
+    }
+  }
+
 /* -(  Internals  )---------------------------------------------------------- */
 
 
diff --git a/src/infrastructure/ssh/PhabricatorSSHWorkflow.php b/src/infrastructure/ssh/PhabricatorSSHWorkflow.php
index beee356951..b6af62d35a 100644
--- a/src/infrastructure/ssh/PhabricatorSSHWorkflow.php
+++ b/src/infrastructure/ssh/PhabricatorSSHWorkflow.php
@@ -95,7 +95,13 @@ abstract class PhabricatorSSHWorkflow extends PhabricatorManagementWorkflow {
     // This has the format "<ip> <remote-port> <local-port>". Grab the IP.
     $remote_address = head(explode(' ', $ssh_client));
 
-    return $remote_address;
+    try {
+      $address = PhutilIPAddress::newAddress($remote_address);
+    } catch (Exception $ex) {
+      return null;
+    }
+
+    return $address->getAddress();
   }
 
 }