From 2e72e9ff31fda13cf5a5f8aa98308a8db34f0a0a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 26 Mar 2015 11:11:52 -0700
Subject: [PATCH] Rate limit outbound requests in Macros

Summary:
Ref T6755. Although we do not return response bodies, it is possible to perform crude portscanning if you can execute a DNS rebinding attack (which, for now, remains theoretical).

Limit users to 60 requests / hour to make it less feasible. This would require ~30 years to portscan all ports on a `/32` netblock.

Users who can guess that services may exist can confirm their existence more quickly than this, but if the attacker already had a very small set of candidate services it seems unlikely that portscanning would be of much use in executing the attack.

This protection should eventually be applied to T4190, too (that task also has other considerations).

Test Plan: Set rate limit very low, hit rate limit.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6755

Differential Revision: https://secure.phabricator.com/D12168
---
 src/__phutil_library_map__.php                |  2 ++
 .../PhabricatorFilesOutboundRequestAction.php | 22 +++++++++++++++++++
 .../PhabricatorMacroEditController.php        |  7 ++++++
 3 files changed, 31 insertions(+)
 create mode 100644 src/applications/files/action/PhabricatorFilesOutboundRequestAction.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index a1d2ac3a54..e91bd4d1ef 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -1844,6 +1844,7 @@ phutil_register_library_map(array(
     'PhabricatorFilesManagementPurgeWorkflow' => 'applications/files/management/PhabricatorFilesManagementPurgeWorkflow.php',
     'PhabricatorFilesManagementRebuildWorkflow' => 'applications/files/management/PhabricatorFilesManagementRebuildWorkflow.php',
     'PhabricatorFilesManagementWorkflow' => 'applications/files/management/PhabricatorFilesManagementWorkflow.php',
+    'PhabricatorFilesOutboundRequestAction' => 'applications/files/action/PhabricatorFilesOutboundRequestAction.php',
     'PhabricatorFlag' => 'applications/flag/storage/PhabricatorFlag.php',
     'PhabricatorFlagColor' => 'applications/flag/constants/PhabricatorFlagColor.php',
     'PhabricatorFlagConstants' => 'applications/flag/constants/PhabricatorFlagConstants.php',
@@ -5160,6 +5161,7 @@ phutil_register_library_map(array(
     'PhabricatorFilesManagementPurgeWorkflow' => 'PhabricatorFilesManagementWorkflow',
     'PhabricatorFilesManagementRebuildWorkflow' => 'PhabricatorFilesManagementWorkflow',
     'PhabricatorFilesManagementWorkflow' => 'PhabricatorManagementWorkflow',
+    'PhabricatorFilesOutboundRequestAction' => 'PhabricatorSystemAction',
     'PhabricatorFlag' => array(
       'PhabricatorFlagDAO',
       'PhabricatorPolicyInterface',
diff --git a/src/applications/files/action/PhabricatorFilesOutboundRequestAction.php b/src/applications/files/action/PhabricatorFilesOutboundRequestAction.php
new file mode 100644
index 0000000000..acba2f8882
--- /dev/null
+++ b/src/applications/files/action/PhabricatorFilesOutboundRequestAction.php
@@ -0,0 +1,22 @@
+<?php
+
+final class PhabricatorFilesOutboundRequestAction
+  extends PhabricatorSystemAction {
+
+  const TYPECONST = 'files.outbound';
+
+  public function getActionConstant() {
+    return self::TYPECONST;
+  }
+
+  public function getScoreThreshold() {
+    return 60 / phutil_units('1 hour in seconds');
+  }
+
+  public function getLimitExplanation() {
+    return pht(
+      'You have initiated too many outbound requests to fetch remote URIs '.
+      'recently.');
+  }
+
+}
diff --git a/src/applications/macro/controller/PhabricatorMacroEditController.php b/src/applications/macro/controller/PhabricatorMacroEditController.php
index e7d8d40b39..8122a7243b 100644
--- a/src/applications/macro/controller/PhabricatorMacroEditController.php
+++ b/src/applications/macro/controller/PhabricatorMacroEditController.php
@@ -72,6 +72,13 @@ final class PhabricatorMacroEditController extends PhabricatorMacroController {
           ));
       } else if ($uri) {
         try {
+          // Rate limit outbound fetches to make this mechanism less useful for
+          // scanning networks and ports.
+          PhabricatorSystemActionEngine::willTakeAction(
+            array($user->getPHID()),
+            new PhabricatorFilesOutboundRequestAction(),
+            1);
+
           $file = PhabricatorFile::newFromFileDownload(
             $uri,
             array(