From cedb0c045ad79f2fd80a6f011987dd8946f4de80 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 11 Jan 2012 16:07:36 -0800
Subject: [PATCH] Lock down accepted next URI values for redirect after login

Summary:
I locked this down a little bit recently, but make
double-extra-super-sure that we aren't sending the user anywhere suspicious or
open-redirecty. This also locks down protocol-relative URIs (//evil.com/path)
although I don't think any browsers do bad stuff with them in this context, and
header injection URIs (although I don't think any of the modern PHP runtimes are
vulnerable).

Test Plan:
  - Ran tests.
  - Hit redirect page with valid and invalid next URIs; was punted to / for
invalid ones and to the right place for valid ones.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: arice, aran, epriestley, btrahan

Differential Revision: https://secure.phabricator.com/D1369
---
 src/__phutil_library_map__.php                |  2 +
 .../PhabricatorLoginValidateController.php    | 10 +--
 .../auth/controller/validate/__init__.php     |  1 +
 ...PhabricatorDirectoryItemEditController.php |  8 +-
 .../controller/itemedit/__init__.php          |  1 +
 ...icatorMetaMTAMailingListEditController.php | 12 +--
 .../controller/mailinglistedit/__init__.php   |  2 -
 src/infrastructure/env/PhabricatorEnv.php     | 83 ++++++++++++++++++-
 src/infrastructure/env/__init__.php           |  1 +
 .../env/__tests__/PhabricatorEnvTestCase.php  | 56 +++++++++++++
 src/infrastructure/env/__tests__/__init__.php | 13 +++
 11 files changed, 168 insertions(+), 21 deletions(-)
 create mode 100644 src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php
 create mode 100644 src/infrastructure/env/__tests__/__init__.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index aa0072fada..34c2578e90 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -438,6 +438,7 @@ phutil_register_library_map(array(
     'PhabricatorEmailLoginController' => 'applications/auth/controller/email',
     'PhabricatorEmailTokenController' => 'applications/auth/controller/emailtoken',
     'PhabricatorEnv' => 'infrastructure/env',
+    'PhabricatorEnvTestCase' => 'infrastructure/env/__tests__',
     'PhabricatorEvent' => 'infrastructure/events/event',
     'PhabricatorEventEngine' => 'infrastructure/events/engine',
     'PhabricatorEventType' => 'infrastructure/events/constant/type',
@@ -1120,6 +1121,7 @@ phutil_register_library_map(array(
     'PhabricatorDraftDAO' => 'PhabricatorLiskDAO',
     'PhabricatorEmailLoginController' => 'PhabricatorAuthController',
     'PhabricatorEmailTokenController' => 'PhabricatorAuthController',
+    'PhabricatorEnvTestCase' => 'PhabricatorTestCase',
     'PhabricatorEvent' => 'PhutilEvent',
     'PhabricatorEventType' => 'PhutilEventType',
     'PhabricatorFeedController' => 'PhabricatorController',
diff --git a/src/applications/auth/controller/validate/PhabricatorLoginValidateController.php b/src/applications/auth/controller/validate/PhabricatorLoginValidateController.php
index 5f12e373cb..838dbf96b6 100644
--- a/src/applications/auth/controller/validate/PhabricatorLoginValidateController.php
+++ b/src/applications/auth/controller/validate/PhabricatorLoginValidateController.php
@@ -79,13 +79,9 @@ class PhabricatorLoginValidateController extends PhabricatorAuthController {
         ));
     }
 
-    $next = nonempty(
-      $request->getStr('next'),
-      $request->getCookie('next_uri'),
-      '/');
-     $request->clearCookie('next_uri');
-
-    if (strpos($next, '/') !== 0) {
+    $next = nonempty($request->getStr('next'), $request->getCookie('next_uri'));
+    $request->clearCookie('next_uri');
+    if (!PhabricatorEnv::isValidLocalWebResource($next)) {
       $next = '/';
     }
 
diff --git a/src/applications/auth/controller/validate/__init__.php b/src/applications/auth/controller/validate/__init__.php
index e925f7989b..b4a9c792a6 100644
--- a/src/applications/auth/controller/validate/__init__.php
+++ b/src/applications/auth/controller/validate/__init__.php
@@ -8,6 +8,7 @@
 
 phutil_require_module('phabricator', 'aphront/response/redirect');
 phutil_require_module('phabricator', 'applications/auth/controller/base');
+phutil_require_module('phabricator', 'infrastructure/env');
 phutil_require_module('phabricator', 'view/page/failure');
 
 phutil_require_module('phutil', 'markup');
diff --git a/src/applications/directory/controller/itemedit/PhabricatorDirectoryItemEditController.php b/src/applications/directory/controller/itemedit/PhabricatorDirectoryItemEditController.php
index f4da31fafe..620c86c846 100644
--- a/src/applications/directory/controller/itemedit/PhabricatorDirectoryItemEditController.php
+++ b/src/applications/directory/controller/itemedit/PhabricatorDirectoryItemEditController.php
@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -56,6 +56,12 @@ class PhabricatorDirectoryItemEditController
       if (!strlen($item->getHref())) {
         $errors[] = 'Item link is required.';
         $e_href = 'Required';
+      } else {
+        $href = $item->getHref();
+        if (!PhabricatorEnv::isValidWebResource($href)) {
+          $e_href = 'Invalid';
+          $errors[] = 'Item link must point to a valid web page.';
+        }
       }
 
       if (!$errors) {
diff --git a/src/applications/directory/controller/itemedit/__init__.php b/src/applications/directory/controller/itemedit/__init__.php
index 915fb9b7cb..436520e65d 100644
--- a/src/applications/directory/controller/itemedit/__init__.php
+++ b/src/applications/directory/controller/itemedit/__init__.php
@@ -11,6 +11,7 @@ phutil_require_module('phabricator', 'aphront/response/redirect');
 phutil_require_module('phabricator', 'applications/directory/controller/base');
 phutil_require_module('phabricator', 'applications/directory/storage/category');
 phutil_require_module('phabricator', 'applications/directory/storage/item');
+phutil_require_module('phabricator', 'infrastructure/env');
 phutil_require_module('phabricator', 'view/form/base');
 phutil_require_module('phabricator', 'view/form/control/select');
 phutil_require_module('phabricator', 'view/form/control/submit');
diff --git a/src/applications/metamta/controller/mailinglistedit/PhabricatorMetaMTAMailingListEditController.php b/src/applications/metamta/controller/mailinglistedit/PhabricatorMetaMTAMailingListEditController.php
index 395cf483de..8c75707436 100644
--- a/src/applications/metamta/controller/mailinglistedit/PhabricatorMetaMTAMailingListEditController.php
+++ b/src/applications/metamta/controller/mailinglistedit/PhabricatorMetaMTAMailingListEditController.php
@@ -58,17 +58,9 @@ class PhabricatorMetaMTAMailingListEditController
       }
 
       if ($list->getURI()) {
-        $uri = new PhutilURI($list->getURI());
-        $proto = $uri->getProtocol();
-        $allowed_protocols = PhabricatorEnv::getEnvConfig(
-          'uri.allowed-protocols');
-        if (empty($allowed_protocols[$proto])) {
+        if (!PhabricatorEnv::isValidWebResource($list->getURI())) {
           $e_uri = 'Invalid';
-          $protocol_list = implode(', ', array_keys($allowed_protocols));
-          $protocol_list = phutil_escape_html($protocol_list);
-          $errors[] =
-            'URI must use one of the allowed protocols: '.
-            $protocol_list.'.';
+          $errors[] = 'Mailing list URI must point to a valid web page.';
         }
       }
 
diff --git a/src/applications/metamta/controller/mailinglistedit/__init__.php b/src/applications/metamta/controller/mailinglistedit/__init__.php
index 1b3a0db970..907eadfaa5 100644
--- a/src/applications/metamta/controller/mailinglistedit/__init__.php
+++ b/src/applications/metamta/controller/mailinglistedit/__init__.php
@@ -18,8 +18,6 @@ phutil_require_module('phabricator', 'view/form/control/text');
 phutil_require_module('phabricator', 'view/form/error');
 phutil_require_module('phabricator', 'view/layout/panel');
 
-phutil_require_module('phutil', 'markup');
-phutil_require_module('phutil', 'parser/uri');
 phutil_require_module('phutil', 'utils');
 
 
diff --git a/src/infrastructure/env/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php
index b9b1caf9e4..ad049a3980 100644
--- a/src/infrastructure/env/PhabricatorEnv.php
+++ b/src/infrastructure/env/PhabricatorEnv.php
@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,6 +16,9 @@
  * limitations under the License.
  */
 
+/**
+ * @task uri URI Validation
+ */
 final class PhabricatorEnv {
   private static $env;
 
@@ -51,4 +54,82 @@ final class PhabricatorEnv {
     return 'http://phabricator.com/docs/phabricator/'.$resource;
   }
 
+
+/* -(  URI Validation  )----------------------------------------------------- */
+
+
+  /**
+   * Detect if a URI satisfies either @{method:isValidLocalWebResource} or
+   * @{method:isValidRemoteWebResource}, i.e. is a page on this server or the
+   * URI of some other resource which has a valid protocol. This rejects
+   * garbage URIs and URIs with protocols which do not appear in the
+   * ##uri.allowed-protocols## configuration, notably 'javascript:' URIs.
+   *
+   * NOTE: This method is generally intended to reject URIs which it may be
+   * unsafe to put in an "href" link attribute.
+   *
+   * @param string URI to test.
+   * @return bool True if the URI identifies a web resource.
+   * @task uri
+   */
+  public static function isValidWebResource($uri) {
+    return self::isValidLocalWebResource($uri) ||
+           self::isValidRemoteWebResource($uri);
+  }
+
+
+  /**
+   * Detect if a URI identifies some page on this server.
+   *
+   * NOTE: This method is generally intended to reject URIs which it may be
+   * unsafe to issue a "Location:" redirect to.
+   *
+   * @param string URI to test.
+   * @return bool True if the URI identifies a local page.
+   * @task uri
+   */
+  public static function isValidLocalWebResource($uri) {
+    $uri = (string)$uri;
+
+    if (!strlen($uri)) {
+      return false;
+    }
+
+    if (preg_match('/\s/', $uri)) {
+      // PHP hasn't been vulnerable to header injection attacks for a bunch of
+      // years, but we can safely reject these anyway since they're never valid.
+      return false;
+    }
+
+    // Valid URIs must begin with '/', followed by the end of the string or some
+    // other non-'/' character. This rejects protocol-relative URIs like
+    // "//evil.com/evil_stuff/".
+    return (bool)preg_match('@^/([^/]|$)@', $uri);
+  }
+
+
+  /**
+   * Detect if a URI identifies some valid remote resource.
+   *
+   * @param string URI to test.
+   * @return bool True if a URI idenfies a remote resource with an allowed
+   *              protocol.
+   * @task uri
+   */
+  public static function isValidRemoteWebResource($uri) {
+    $uri = (string)$uri;
+
+    $proto = id(new PhutilURI($uri))->getProtocol();
+    if (!$proto) {
+      return false;
+    }
+
+    $allowed = self::getEnvConfig('uri.allowed-protocols');
+    if (empty($allowed[$proto])) {
+      return false;
+    }
+
+    return true;
+  }
+
 }
diff --git a/src/infrastructure/env/__init__.php b/src/infrastructure/env/__init__.php
index ca8608ef16..cbda9af309 100644
--- a/src/infrastructure/env/__init__.php
+++ b/src/infrastructure/env/__init__.php
@@ -6,6 +6,7 @@
 
 
 
+phutil_require_module('phutil', 'parser/uri');
 phutil_require_module('phutil', 'utils');
 
 
diff --git a/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php b/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php
new file mode 100644
index 0000000000..9d2b367332
--- /dev/null
+++ b/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php
@@ -0,0 +1,56 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class PhabricatorEnvTestCase extends PhabricatorTestCase {
+
+  public function testLocalWebResource() {
+    $map = array(
+      '/'                     => true,
+      '/D123'                 => true,
+      '/path/to/something/'   => true,
+      "/path/to/\nHeader: x"  => false,
+      'http://evil.com/'      => false,
+      '//evil.com/evil/'      => false,
+      'javascript:lol'        => false,
+      ''                      => false,
+      null                    => false,
+    );
+
+    foreach ($map as $uri => $expect) {
+      $this->assertEqual(
+        $expect,
+        PhabricatorEnv::isValidLocalWebResource($uri),
+        "Valid local resource: {$uri}");
+    }
+  }
+
+  public function testRemoteWebResource() {
+    $map = array(
+      'http://example.com/'   => true,
+      'derp://example.com/'   => false,
+      'javascript:alert(1)'   => false,
+    );
+
+    foreach ($map as $uri => $expect) {
+      $this->assertEqual(
+        $expect,
+        PhabricatorEnv::isValidRemoteWebResource($uri),
+        "Valid remote resource: {$uri}");
+    }
+  }
+}
diff --git a/src/infrastructure/env/__tests__/__init__.php b/src/infrastructure/env/__tests__/__init__.php
new file mode 100644
index 0000000000..dfd22d9d2a
--- /dev/null
+++ b/src/infrastructure/env/__tests__/__init__.php
@@ -0,0 +1,13 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'infrastructure/env');
+phutil_require_module('phabricator', 'infrastructure/testing/testcase');
+
+
+phutil_require_source('PhabricatorEnvTestCase.php');