From 45665dd3b458a506ba9e44bcdbd95b04c96426ac Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 15 Apr 2020 05:17:22 -0700
Subject: [PATCH] Hide "notification.servers" configuration and don't follow
 redirects from Aphlict

Summary:
See <https://hackerone.com/reports/850114>.

An attacker with administrator privileges can configure "notification.servers" to connect to internal services, either directly or with chosen parameters by selecting an attacker-controlled service and having it issue a "Location" redirect.

Generally, we allow this attack to occur. The same administrator can use an authentication provider or a VCS repository to perform the same attack, and we can't reasonably harden these workflows without breaking things that users expect to be able to do.

There's no reason this particular variation of the attack needs to be allowable, though, and the current behavior isn't consistent with how other similar things work.

  - Hide the "notification.servers" configuration, which also locks it. This is similar to other modern service/server configuration.
  - Don't follow redirects on these requests. Aphlict should never issue a "Location" header, so if we encounter one something is misconfigured. Declining to follow this header likely makes the issue easier to debug.

Test Plan:
  - Viewed configuration in web UI.
  - Configured a server that "Location: ..." redirects, got a followed redirect before and a failure afterward.

{F7365973}

Differential Revision: https://secure.phabricator.com/D21123
---
 .../PhabricatorNotificationConfigOptions.php  |  1 +
 .../PhabricatorNotificationServerRef.php      | 29 ++++++++++++++++---
 .../configuration_locked.diviner              |  6 ++++
 .../user/configuration/notifications.diviner  |  3 +-
 4 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/src/applications/config/option/PhabricatorNotificationConfigOptions.php b/src/applications/config/option/PhabricatorNotificationConfigOptions.php
index 9f06bc3c2d..e92017d2c5 100644
--- a/src/applications/config/option/PhabricatorNotificationConfigOptions.php
+++ b/src/applications/config/option/PhabricatorNotificationConfigOptions.php
@@ -52,6 +52,7 @@ EOTEXT
 
     return array(
       $this->newOption('notification.servers', $servers_type, array())
+        ->setHidden(true)
         ->setSummary(pht('Configure real-time notifications.'))
         ->setDescription($servers_help)
         ->addExample(
diff --git a/src/applications/notification/client/PhabricatorNotificationServerRef.php b/src/applications/notification/client/PhabricatorNotificationServerRef.php
index 46d03a5c3a..714ad5f5d7 100644
--- a/src/applications/notification/client/PhabricatorNotificationServerRef.php
+++ b/src/applications/notification/client/PhabricatorNotificationServerRef.php
@@ -209,8 +209,7 @@ final class PhabricatorNotificationServerRef
 
     $server_uri = $this->getURI('/status/');
 
-    list($body) = id(new HTTPSFuture($server_uri))
-      ->setTimeout(2)
+    list($body) = $this->newFuture($server_uri)
       ->resolvex();
 
     return phutil_json_decode($body);
@@ -225,10 +224,32 @@ final class PhabricatorNotificationServerRef
     $server_uri = $this->getURI('/');
     $payload = phutil_json_encode($data);
 
-    id(new HTTPSFuture($server_uri, $payload))
+    $this->newFuture($server_uri, $payload)
       ->setMethod('POST')
-      ->setTimeout(2)
       ->resolvex();
   }
 
+  private function newFuture($uri, $data = null) {
+    if ($data === null) {
+      $future = new HTTPSFuture($uri);
+    } else {
+      $future = new HTTPSFuture($uri, $data);
+    }
+
+    $future->setTimeout(2);
+
+    // At one point, a HackerOne researcher reported a "Location:" redirect
+    // attack here (if the attacker can gain control of the notification
+    // server or the configuration).
+
+    // Although this attack is not particularly concerning, we don't expect
+    // Aphlict to ever issue a "Location:" header, so receiving one indicates
+    // something is wrong and declining to follow the header may make debugging
+    // easier.
+
+    $future->setFollowLocation(false);
+
+    return $future;
+  }
+
 }
diff --git a/src/docs/user/configuration/configuration_locked.diviner b/src/docs/user/configuration/configuration_locked.diviner
index f96adc2d82..57ed76c5c7 100644
--- a/src/docs/user/configuration/configuration_locked.diviner
+++ b/src/docs/user/configuration/configuration_locked.diviner
@@ -146,6 +146,12 @@ To clear this setup warning and avoid surprise behavioral changes in the future,
 you should move these configuration values from the database to a local config
 file. Usually, you'll do this by first copying the value from the database:
 
+```
+phabricator/ $ ./bin/config get <key>
+```
+
+...into local configuration:
+
 ```
 phabricator/ $ ./bin/config set <key> <value>
 ```
diff --git a/src/docs/user/configuration/notifications.diviner b/src/docs/user/configuration/notifications.diviner
index 8e70315460..13d93317f9 100644
--- a/src/docs/user/configuration/notifications.diviner
+++ b/src/docs/user/configuration/notifications.diviner
@@ -156,7 +156,8 @@ Verifying Server Status
 =======================
 
 After configuring `notification.servers`, navigate to
-{nav Config > Notification Servers} to verify that things are operational.
+{nav Config > Services > Notification Servers} to verify that things are
+operational.
 
 
 Troubleshooting