Provide a setting which forces all file views to be served from an alternate

domain

Summary:
See D758, D759.

  - Provide a strongly recommended setting which permits configuration of an
alternate domain.
  - Lock cookies down better: set them on the exact domain, and use SSL-only if
the configuration is HTTPS.
  - Prevent Phabriator from setting cookies on other domains.

This assumes D759 will land, it is not effective without that change.

Test Plan:
  - Attempted to login from a different domain and was rejected.
  - Logged out, logged back in normally.
  - Put install in setup mode and verified it revealed a warning.
  - Configured an alterate domain.
  - Tried to view an image with an old URI, got a 400.
  - Went to /files/ and verified links rendered to the alternate domain.
  - Viewed an alternate domain file.
  - Tried to view an alternate domain file without the secret key, got a 404.

Reviewers: andrewjcg, erling, aran, tuomaspelkonen, jungejason, codeblock
CC: aran
Differential Revision: 760

conf/default.conf.php

@@ -35,6 +35,35 @@ return array(
   // be 50x50px.
   'user.default-profile-image-phid' => 'PHID-FILE-4d61229816cfe6f2b2a3',
 
+// -- IMPORTANT! Security! -------------------------------------------------- //
+
+  // IMPORTANT: By default, Phabricator serves files from the same domain the
+  // application lives on. This is convenient but not secure: it creates
+  // a vulnerability where an external attacker can:
+  //
+  //  - Convince a privileged user to upload a file which appears to be an
+  //    image or some other inoccuous type of file (the file is actually both
+  //    a JAR and an image); and
+  //  - convince the user to give them the URI for the image; and
+  //  - convince the user to click a link to a site which embeds the "image"
+  //    using an <applet /> tag. This steals the user's credentials.
+  //
+  // If the attacker is internal, they can execute the first two steps
+  // themselves and need only convince another user to click a link in order to
+  // steal their credentials.
+  //
+  // To avoid this, you should configure a second domain in the same way you
+  // have the primary domain configured (e.g., point it at the same machine and
+  // set up the same vhost rules) and provide it here. For instance, if your
+  // primary install is on "http://www.phabricator-example.com/", you could
+  // configure "http://www.phabricator-files.com/" and specify the entire
+  // domain (with protocol) here. This will enforce that viewable files are
+  // served only from the alternate domain. Ideally, you should use a completely
+  // separate domain name rather than just a different subdomain.
+  //
+  // It is STRONGLY RECOMMENDED that you configure this. Phabricator makes this
+  // attack difficult, but it is viable unless you isolate the file domain.
+  'security.alternate-file-domain'  => null,
 
 // -- DarkConsole ----------------------------------------------------------- //
 
@@ -314,6 +343,12 @@ return array(
   // addresses.
   'phabricator.mail-key'        => '5ce3e7e8787f6e40dfae861da315a5cdf1018f12',
 
+
+  // This is hashed with other inputs to generate file secret keys. Changing
+  // it will invalidate all file URIs if you have an alternate file domain
+  // configured (see 'security.alternate-file-domain').
+  'phabricator.file-key'        => 'ade8dadc8b4382067069a4d4798112191af8a190',
+
   // Version string displayed in the footer. You probably should leave this
   // alone.
   'phabricator.version'         => 'UNSTABLE',
@@ -338,6 +373,9 @@ return array(
   //
   // The keys in this array are viewable mime types; the values are the mime
   // types they will be delivered as when they are viewed in the browser.
+  //
+  // IMPORTANT: Making any file types viewable is a security vulnerability if
+  // you do not configure 'security.alternate-file-domain' above.
   'files.viewable-mime-types' => array(
     'image/jpeg'  => 'image/jpeg',
     'image/jpg'   => 'image/jpg',

src/__phutil_library_map__.php

@@ -404,6 +404,7 @@ phutil_register_library_map(array(
     'PhabricatorFeedStreamController' => 'applications/feed/controller/stream',
     'PhabricatorFeedView' => 'applications/feed/view/base',
     'PhabricatorFile' => 'applications/files/storage/file',
+    'PhabricatorFileAltViewController' => 'applications/files/controller/altview',
     'PhabricatorFileController' => 'applications/files/controller/base',
     'PhabricatorFileDAO' => 'applications/files/storage/base',
     'PhabricatorFileDropUploadController' => 'applications/files/controller/dropupload',
@@ -1000,6 +1001,7 @@ phutil_register_library_map(array(
     'PhabricatorFeedStreamController' => 'PhabricatorFeedController',
     'PhabricatorFeedView' => 'AphrontView',
     'PhabricatorFile' => 'PhabricatorFileDAO',
+    'PhabricatorFileAltViewController' => 'PhabricatorFileController',
     'PhabricatorFileController' => 'PhabricatorController',
     'PhabricatorFileDAO' => 'PhabricatorLiskDAO',
     'PhabricatorFileDropUploadController' => 'PhabricatorFileController',

src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php

@@ -56,6 +56,7 @@ class AphrontDefaultApplicationConfiguration
         '(?P<view>info)/(?P<phid>[^/]+)/' => 'PhabricatorFileViewController',
         '(?P<view>view)/(?P<phid>[^/]+)/' => 'PhabricatorFileViewController',
         '(?P<view>download)/(?P<phid>[^/]+)/' => 'PhabricatorFileViewController',
+        'alt/(?<key>[^/]+)/(?<phid>[^/]+)/' => 'PhabricatorFileAltViewController',
         'macro/' => array(
           '$' => 'PhabricatorFileMacroListController',
           'edit/(?:(?P<id>\d+)/)?$' => 'PhabricatorFileMacroEditController',

src/aphront/request/AphrontRequest.php

@@ -181,16 +181,48 @@ class AphrontRequest {
   }
 
   final public function setCookie($name, $value, $expire = null) {
+
+    // Ensure cookies are only set on the configured domain.
+
+    $base_uri = PhabricatorEnv::getEnvConfig('phabricator.base-uri');
+    $base_uri = new PhutilURI($base_uri);
+
+    $base_domain = $base_uri->getDomain();
+    $base_protocol = $base_uri->getProtocol();
+
+    $actual_host = $this->getHost();
+    if ($base_domain != $actual_host) {
+      throw new Exception(
+        "This install of Phabricator is configured as '{$base_domain}' but ".
+        "you are accessing it via '{$actual_host}'. Access Phabricator via ".
+        "the primary configured domain.");
+    }
+
     if ($expire === null) {
       $expire = time() + (60 * 60 * 24 * 365 * 5);
     }
+
+    if ($value == '') {
+      // NOTE: If we're clearing the cookie, also clear it on the entire
+      // domain. This allows us to clear older cookies which we didn't scope
+      // as tightly.
+      setcookie(
+        $name,
+        $value,
+        $expire,
+        $path = '/',
+        $domain = '',
+        $secure = ($base_protocol == 'https'),
+        $http_only = true);
+    }
+
     setcookie(
       $name,
       $value,
       $expire,
       $path = '/',
-      $domain = '',
+      $base_domain,
-      $secure = false,
+      $secure = ($base_protocol == 'https'),
       $http_only = true);
   }
 

src/aphront/request/__init__.php

@@ -7,6 +7,7 @@
 
 
 phutil_require_module('phabricator', 'aphront/exception/csrf');
+phutil_require_module('phabricator', 'infrastructure/env');
 
 phutil_require_module('phutil', 'parser/uri');
 phutil_require_module('phutil', 'utils');

src/applications/files/controller/altview/PhabricatorFileAltViewController.php

@@ -0,0 +1,68 @@
+<?php
+
+/*
+ * Copyright 2011 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.
+ */
+
+class PhabricatorFileAltViewController extends PhabricatorFileController {
+
+  private $phid;
+  private $key;
+
+  public function willProcessRequest(array $data) {
+    $this->phid = $data['phid'];
+    $this->key  = $data['key'];
+  }
+
+  public function shouldRequireLogin() {
+    return false;
+  }
+
+  public function processRequest() {
+
+    $alt = PhabricatorEnv::getEnvConfig('security.alternate-file-domain');
+    if (!$alt) {
+      return new Aphront400Response();
+    }
+
+    $request = $this->getRequest();
+
+    $alt_domain = id(new PhutilURI($alt))->getDomain();
+    if ($alt_domain != $request->getHost()) {
+      return new Aphront400Response();
+    }
+
+    $file = id(new PhabricatorFile())->loadOneWhere(
+      'phid = %s',
+      $this->phid);
+    if (!$file) {
+      return new Aphront404Response();
+    }
+
+    if (!$file->validateSecretKey($this->key)) {
+      return new Aphront404Response();
+    }
+
+    // It's safe to bypass view restrictions because we know we are being served
+    // off an alternate domain which we will not set cookies on.
+
+    $data = $file->loadFileData();
+    $response = new AphrontFileResponse();
+    $response->setContent($data);
+    $response->setCacheDurationInSeconds(60 * 60 * 24 * 30);
+
+    return $response;
+  }
+}

src/applications/files/controller/altview/__init__.php

@@ -0,0 +1,20 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'aphront/response/400');
+phutil_require_module('phabricator', 'aphront/response/404');
+phutil_require_module('phabricator', 'aphront/response/file');
+phutil_require_module('phabricator', 'applications/files/controller/base');
+phutil_require_module('phabricator', 'applications/files/storage/file');
+phutil_require_module('phabricator', 'infrastructure/env');
+
+phutil_require_module('phutil', 'parser/uri');
+phutil_require_module('phutil', 'utils');
+
+
+phutil_require_source('PhabricatorFileAltViewController.php');

src/applications/files/controller/list/PhabricatorFileListController.php

@@ -75,7 +75,7 @@ class PhabricatorFileListController extends PhabricatorFileController {
           'a',
           array(
             'class' => 'small button grey',
-            'href'  => '/file/view/'.$file->getPHID().'/',
+            'href'  => $file->getViewURI(),
           ),
           'View');
       } else {

src/applications/files/controller/view/PhabricatorFileViewController.php

@@ -71,6 +71,18 @@ class PhabricatorFileViewController extends PhabricatorFileController {
           $mime_type = $file->getViewableMimeType();
         }
 
+        // If an alternate file domain is configured, forbid all views which
+        // don't originate from it.
+        if (!$download) {
+          $alt = PhabricatorEnv::getEnvConfig('security.alternate-file-domain');
+          if ($alt) {
+            $domain = id(new PhutilURI($alt))->getDomain();
+            if ($domain != $request->getHost()) {
+              return new Aphront400Response();
+            }
+          }
+        }
+
         $response->setMimeType($mime_type);
 
         if ($download) {
@@ -98,7 +110,7 @@ class PhabricatorFileViewController extends PhabricatorFileController {
     $form = new AphrontFormView();
 
     if ($file->isViewableInBrowser()) {
-      $form->setAction('/file/view/'.$file->getPHID().'/');
+      $form->setAction($file->getViewURI());
       $button_name = 'View File';
     } else {
       $form->setAction('/file/download/'.$file->getPHID().'/');

src/applications/files/controller/view/__init__.php

@@ -15,6 +15,7 @@ phutil_require_module('phabricator', 'applications/files/storage/file');
 phutil_require_module('phabricator', 'applications/files/storage/transformed');
 phutil_require_module('phabricator', 'applications/files/uri');
 phutil_require_module('phabricator', 'applications/people/storage/user');
+phutil_require_module('phabricator', 'infrastructure/env');
 phutil_require_module('phabricator', 'view/control/table');
 phutil_require_module('phabricator', 'view/form/base');
 phutil_require_module('phabricator', 'view/form/control/static');
@@ -23,6 +24,7 @@ phutil_require_module('phabricator', 'view/layout/panel');
 phutil_require_module('phabricator', 'view/utils');
 
 phutil_require_module('phutil', 'markup');
+phutil_require_module('phutil', 'parser/uri');
 phutil_require_module('phutil', 'utils');
 
 

src/applications/files/storage/file/PhabricatorFile.php

@@ -216,7 +216,16 @@ class PhabricatorFile extends PhabricatorFileDAO {
   }
 
   public function getViewURI() {
-    return PhabricatorFileURI::getViewURIForPHID($this->getPHID());
+    $alt = PhabricatorEnv::getEnvConfig('security.alternate-file-domain');
+    if ($alt) {
+      $path = '/file/alt/'.$this->generateSecretKey().'/'.$this->getPHID().'/';
+      $uri = new PhutilURI($alt);
+      $uri->setPath($path);
+
+      return (string)$uri;
+    } else {
+      return '/file/view/'.$this->getPHID().'/';
+    }
   }
 
   public function getInfoURI() {
@@ -302,4 +311,14 @@ class PhabricatorFile extends PhabricatorFileDAO {
     return idx($mime_map, $mime_type);
   }
 
+  public function validateSecretKey($key) {
+    return ($key == $this->generateSecretKey());
+  }
+
+  private function generateSecretKey() {
+    $file_key = PhabricatorEnv::getEnvConfig('phabricator.file-key');
+    $hash = sha1($this->phid.$this->storageHandle.$file_key);
+    return substr($hash, 0, 20);
+  }
+
 }

src/applications/files/storage/file/__init__.php

@@ -8,7 +8,6 @@
 
 phutil_require_module('phabricator', 'applications/files/exception/upload');
 phutil_require_module('phabricator', 'applications/files/storage/base');
-phutil_require_module('phabricator', 'applications/files/uri');
 phutil_require_module('phabricator', 'applications/phid/constants');
 phutil_require_module('phabricator', 'applications/phid/storage/phid');
 phutil_require_module('phabricator', 'infrastructure/env');

src/applications/files/uri/PhabricatorFileURI.php

@@ -19,7 +19,18 @@
 final class PhabricatorFileURI {
 
   public static function getViewURIForPHID($phid) {
-    return '/file/view/'.$phid.'/';
+
+    // TODO: Get rid of this class, the advent of the applet attack makes the
+    // tiny optimization it represented effectively obsolete.
+
+    $file = id(new PhabricatorFile())->loadOneWhere(
+      'phid = %s',
+      $phid);
+    if ($file) {
+      return $file->getViewURI();
+    }
+
+    return null;
   }
 
 }

src/applications/files/uri/__init__.php

@@ -6,5 +6,9 @@
 
 
 
+phutil_require_module('phabricator', 'applications/files/storage/file');
+
+phutil_require_module('phutil', 'utils');
+
 
 phutil_require_source('PhabricatorFileURI.php');

src/infrastructure/setup/PhabricatorSetup.php

@@ -114,6 +114,14 @@ class PhabricatorSetup {
       self::write(" okay  'open_basedir' is not set.\n");
     }
 
+    if (!PhabricatorEnv::getEnvConfig('security.alternate-file-domain')) {
+      self::write(
+        "[WARN] You have not configured 'security.alternate-file-domain'. ".
+        "This may make your installation vulnerable to attack. Make sure ".
+        "you read the documentation for this parameter and understand the ".
+        "consequences of leaving it unconfigured.\n");
+    }
+
     self::write("[OKAY] Core configuration OKAY.\n");
 
     self::writeHeader("REQUIRED PHP EXTENSIONS");