From 68c30e1a714af891afa3c49d90a30e75a60ef12b Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 1 Aug 2011 22:24:00 -0700 Subject: [PATCH] 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 | 38 +++++++++++ src/__phutil_library_map__.php | 2 + ...AphrontDefaultApplicationConfiguration.php | 1 + src/aphront/request/AphrontRequest.php | 36 +++++++++- src/aphront/request/__init__.php | 1 + .../PhabricatorFileAltViewController.php | 68 +++++++++++++++++++ .../files/controller/altview/__init__.php | 20 ++++++ .../list/PhabricatorFileListController.php | 2 +- .../view/PhabricatorFileViewController.php | 14 +++- .../files/controller/view/__init__.php | 2 + .../files/storage/file/PhabricatorFile.php | 21 +++++- .../files/storage/file/__init__.php | 1 - .../files/uri/PhabricatorFileURI.php | 13 +++- src/applications/files/uri/__init__.php | 4 ++ src/infrastructure/setup/PhabricatorSetup.php | 8 +++ 15 files changed, 224 insertions(+), 7 deletions(-) create mode 100644 src/applications/files/controller/altview/PhabricatorFileAltViewController.php create mode 100644 src/applications/files/controller/altview/__init__.php diff --git a/conf/default.conf.php b/conf/default.conf.php index ebbf3f5bc7..77ec9c166f 100644 --- a/conf/default.conf.php +++ b/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 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', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 0824b0a5ba..8e097b6371 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php index 2209350a02..ce6c979a76 100644 --- a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php @@ -56,6 +56,7 @@ class AphrontDefaultApplicationConfiguration '(?Pinfo)/(?P[^/]+)/' => 'PhabricatorFileViewController', '(?Pview)/(?P[^/]+)/' => 'PhabricatorFileViewController', '(?Pdownload)/(?P[^/]+)/' => 'PhabricatorFileViewController', + 'alt/(?[^/]+)/(?[^/]+)/' => 'PhabricatorFileAltViewController', 'macro/' => array( '$' => 'PhabricatorFileMacroListController', 'edit/(?:(?P\d+)/)?$' => 'PhabricatorFileMacroEditController', diff --git a/src/aphront/request/AphrontRequest.php b/src/aphront/request/AphrontRequest.php index 30a23b238a..fdb672b7b3 100644 --- a/src/aphront/request/AphrontRequest.php +++ b/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 = '', - $secure = false, + $base_domain, + $secure = ($base_protocol == 'https'), $http_only = true); } diff --git a/src/aphront/request/__init__.php b/src/aphront/request/__init__.php index f3296f0d91..7e0a7e8115 100644 --- a/src/aphront/request/__init__.php +++ b/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'); diff --git a/src/applications/files/controller/altview/PhabricatorFileAltViewController.php b/src/applications/files/controller/altview/PhabricatorFileAltViewController.php new file mode 100644 index 0000000000..aaa55a6bae --- /dev/null +++ b/src/applications/files/controller/altview/PhabricatorFileAltViewController.php @@ -0,0 +1,68 @@ +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; + } +} diff --git a/src/applications/files/controller/altview/__init__.php b/src/applications/files/controller/altview/__init__.php new file mode 100644 index 0000000000..a447af1b27 --- /dev/null +++ b/src/applications/files/controller/altview/__init__.php @@ -0,0 +1,20 @@ + 'small button grey', - 'href' => '/file/view/'.$file->getPHID().'/', + 'href' => $file->getViewURI(), ), 'View'); } else { diff --git a/src/applications/files/controller/view/PhabricatorFileViewController.php b/src/applications/files/controller/view/PhabricatorFileViewController.php index a5dc977037..6201e67fd3 100644 --- a/src/applications/files/controller/view/PhabricatorFileViewController.php +++ b/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().'/'); diff --git a/src/applications/files/controller/view/__init__.php b/src/applications/files/controller/view/__init__.php index afe7459ce5..715df129d0 100644 --- a/src/applications/files/controller/view/__init__.php +++ b/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'); diff --git a/src/applications/files/storage/file/PhabricatorFile.php b/src/applications/files/storage/file/PhabricatorFile.php index 45d878c06a..f380a89660 100644 --- a/src/applications/files/storage/file/PhabricatorFile.php +++ b/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); + } + } diff --git a/src/applications/files/storage/file/__init__.php b/src/applications/files/storage/file/__init__.php index 4947cce2b8..258006cd1f 100644 --- a/src/applications/files/storage/file/__init__.php +++ b/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'); diff --git a/src/applications/files/uri/PhabricatorFileURI.php b/src/applications/files/uri/PhabricatorFileURI.php index db14d2daea..8f813a82bf 100644 --- a/src/applications/files/uri/PhabricatorFileURI.php +++ b/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; } } diff --git a/src/applications/files/uri/__init__.php b/src/applications/files/uri/__init__.php index d53daa769b..bedf04f978 100644 --- a/src/applications/files/uri/__init__.php +++ b/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'); diff --git a/src/infrastructure/setup/PhabricatorSetup.php b/src/infrastructure/setup/PhabricatorSetup.php index 2bcf41bd4f..88701f7a2c 100644 --- a/src/infrastructure/setup/PhabricatorSetup.php +++ b/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");