From 549146bc7cc445af769484cd4170983ee675c0be Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 14 Feb 2012 14:52:27 -0800 Subject: [PATCH] Move ALL files to serve from the alternate file domain, not just files without "Content-Disposition: attachment" Summary: We currently serve some files off the primary domain (with "Content-Disposition: attachment" + a CSRF check) and some files off the alternate domain (without either). This is not sufficient, because some UAs (like the iPad) ignore "Content-Disposition: attachment". So there's an attack that goes like this: - Alice uploads xss.html - Alice says to Bob "hey download this file on your iPad" - Bob clicks "Download" on Phabricator on his iPad, gets XSS'd. NOTE: This removes the CSRF check for downloading files. The check is nice to have but only raises the barrier to entry slightly. Between iPad / sniffing / flash bytecode attacks, single-domain installs are simply insecure. We could restore the check at some point in conjunction with a derived authentication cookie (i.e., a mini-session-token which is only useful for downloading files), but that's a lot of complexity to drop all at once. (Because files are now authenticated only by knowing the PHID and secret key, this also fixes the "no profile pictures in public feed while logged out" issue.) Test Plan: Viewed, info'd, and downloaded files Reviewers: btrahan, arice, alok Reviewed By: arice CC: aran, epriestley Maniphest Tasks: T843 Differential Revision: https://secure.phabricator.com/D1608 --- conf/default.conf.php | 33 +++------- src/__phutil_library_map__.php | 8 +-- ...AphrontDefaultApplicationConfiguration.php | 13 ++-- .../PhabricatorFileDataController.php} | 33 ++++++---- .../controller/{altview => data}/__init__.php | 3 +- .../PhabricatorFileInfoController.php} | 62 +------------------ .../controller/{view => info}/__init__.php | 7 +-- .../files/storage/file/PhabricatorFile.php | 12 +--- src/infrastructure/env/PhabricatorEnv.php | 10 +++ src/infrastructure/setup/PhabricatorSetup.php | 4 +- 10 files changed, 62 insertions(+), 123 deletions(-) rename src/applications/files/controller/{altview/PhabricatorFileAltViewController.php => data/PhabricatorFileDataController.php} (62%) rename src/applications/files/controller/{altview => data}/__init__.php (83%) rename src/applications/files/controller/{view/PhabricatorFileViewController.php => info/PhabricatorFileInfoController.php} (73%) rename src/applications/files/controller/{view => info}/__init__.php (71%) diff --git a/conf/default.conf.php b/conf/default.conf.php index 00a610bf1b..c2d48261eb 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -38,31 +38,20 @@ return array( // -- 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. + // application lives on. This is convenient but not secure: it creates a large + // class of vulnerabilities which can not be generally mitigated. // // 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. + // domain (with protocol) here. This will enforce that 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. + // It is STRONGLY RECOMMENDED that you configure this. Your install is NOT + // SECURE unless you do so. 'security.alternate-file-domain' => null, // Default key for HMAC digests where the key is not important (i.e., the @@ -480,17 +469,15 @@ return array( // Lists which uploaded file types may be viewed in the browser. If a file // has a mime type which does not appear in this list, it will always be - // downloaded instead of displayed. This is a security consideration: if a - // user uploads a file of type "text/html" and it is displayed as - // "text/html", they can easily execute XSS attacks. This is also a usability + // downloaded instead of displayed. This is mainly a usability // consideration, since browsers tend to freak out when viewing enormous // binary files. // // 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. + // IMPORTANT: Configure 'security.alternate-file-domain' above! Your install + // is NOT safe if it is left unconfigured. '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 77037eed33..a7d1492631 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -513,12 +513,13 @@ 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', + 'PhabricatorFileDataController' => 'applications/files/controller/data', 'PhabricatorFileDeleteController' => 'applications/files/controller/delete', 'PhabricatorFileDropUploadController' => 'applications/files/controller/dropupload', 'PhabricatorFileImageMacro' => 'applications/files/storage/imagemacro', + 'PhabricatorFileInfoController' => 'applications/files/controller/info', 'PhabricatorFileListController' => 'applications/files/controller/list', 'PhabricatorFileMacroDeleteController' => 'applications/files/controller/macrodelete', 'PhabricatorFileMacroEditController' => 'applications/files/controller/macroedit', @@ -533,7 +534,6 @@ phutil_register_library_map(array( 'PhabricatorFileUploadController' => 'applications/files/controller/upload', 'PhabricatorFileUploadException' => 'applications/files/exception/upload', 'PhabricatorFileUploadView' => 'applications/files/view/upload', - 'PhabricatorFileViewController' => 'applications/files/controller/view', 'PhabricatorGarbageCollectorDaemon' => 'infrastructure/daemon/garbagecollector', 'PhabricatorGoodForNothingWorker' => 'infrastructure/daemon/workers/worker/goodfornothing', 'PhabricatorHandleObjectSelectorDataView' => 'applications/phid/handle/view/selector', @@ -1240,12 +1240,13 @@ phutil_register_library_map(array( 'PhabricatorFeedStreamController' => 'PhabricatorFeedController', 'PhabricatorFeedView' => 'AphrontView', 'PhabricatorFile' => 'PhabricatorFileDAO', - 'PhabricatorFileAltViewController' => 'PhabricatorFileController', 'PhabricatorFileController' => 'PhabricatorController', 'PhabricatorFileDAO' => 'PhabricatorLiskDAO', + 'PhabricatorFileDataController' => 'PhabricatorFileController', 'PhabricatorFileDeleteController' => 'PhabricatorFileController', 'PhabricatorFileDropUploadController' => 'PhabricatorFileController', 'PhabricatorFileImageMacro' => 'PhabricatorFileDAO', + 'PhabricatorFileInfoController' => 'PhabricatorFileController', 'PhabricatorFileListController' => 'PhabricatorFileController', 'PhabricatorFileMacroDeleteController' => 'PhabricatorFileController', 'PhabricatorFileMacroEditController' => 'PhabricatorFileController', @@ -1257,7 +1258,6 @@ phutil_register_library_map(array( 'PhabricatorFileTransformController' => 'PhabricatorFileController', 'PhabricatorFileUploadController' => 'PhabricatorFileController', 'PhabricatorFileUploadView' => 'AphrontView', - 'PhabricatorFileViewController' => 'PhabricatorFileController', 'PhabricatorGarbageCollectorDaemon' => 'PhabricatorDaemon', 'PhabricatorGoodForNothingWorker' => 'PhabricatorWorker', 'PhabricatorHelpController' => 'PhabricatorController', diff --git a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php index 4e9bfbfcf3..270415aab1 100644 --- a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php @@ -55,12 +55,15 @@ class AphrontDefaultApplicationConfiguration 'upload/$' => 'PhabricatorFileUploadController', 'dropupload/$' => 'PhabricatorFileDropUploadController', 'delete/(?P\d+)/$' => 'PhabricatorFileDeleteController', - '(?Pinfo)/(?P[^/]+)/' => 'PhabricatorFileViewController', - '(?Pview)/(?P[^/]+)/' => 'PhabricatorFileViewController', - '(?Pdownload)/(?P[^/]+)/' - => 'PhabricatorFileViewController', + 'info/(?P[^/]+)/' => 'PhabricatorFileInfoController', + + 'data/(?P[^/]+)/(?P[^/]+)/' + => 'PhabricatorFileDataController', + // TODO: This is a deprecated version of /data/. Remove it after + // old links have had a chance to rot. 'alt/(?P[^/]+)/(?P[^/]+)/' - => 'PhabricatorFileAltViewController', + => 'PhabricatorFileDataController', + 'macro/' => array( '$' => 'PhabricatorFileMacroListController', 'edit/(?:(?P\d+)/)?$' => 'PhabricatorFileMacroEditController', diff --git a/src/applications/files/controller/altview/PhabricatorFileAltViewController.php b/src/applications/files/controller/data/PhabricatorFileDataController.php similarity index 62% rename from src/applications/files/controller/altview/PhabricatorFileAltViewController.php rename to src/applications/files/controller/data/PhabricatorFileDataController.php index 37e9104b33..f55b8cb318 100644 --- a/src/applications/files/controller/altview/PhabricatorFileAltViewController.php +++ b/src/applications/files/controller/data/PhabricatorFileDataController.php @@ -16,7 +16,7 @@ * limitations under the License. */ -class PhabricatorFileAltViewController extends PhabricatorFileController { +final class PhabricatorFileDataController extends PhabricatorFileController { private $phid; private $key; @@ -31,16 +31,11 @@ class PhabricatorFileAltViewController extends PhabricatorFileController { } public function processRequest() { - - $alt = PhabricatorEnv::getEnvConfig('security.alternate-file-domain'); - if (!$alt) { - return new Aphront400Response(); - } - $request = $this->getRequest(); + $alt = PhabricatorEnv::getEnvConfig('security.alternate-file-domain'); $alt_domain = id(new PhutilURI($alt))->getDomain(); - if ($alt_domain != $request->getHost()) { + if ($alt_domain && ($alt_domain != $request->getHost())) { return new Aphront400Response(); } @@ -55,15 +50,29 @@ class PhabricatorFileAltViewController extends PhabricatorFileController { return new Aphront403Response(); } - // 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->setMimeType($file->getMimeType()); $response->setCacheDurationInSeconds(60 * 60 * 24 * 30); + $is_view = $file->isViewableInBrowser(); + + if ($is_view) { + $response->setMimeType($file->getViewableMimeType()); + } else { + if (!$request->isHTTPPost()) { + // NOTE: Require POST to download files. We'd rather go full-bore and + // do a real CSRF check, but can't currently authenticate users on the + // file domain. This should blunt any attacks based on iframes, script + // tags, applet tags, etc., at least. Send the user to the "info" page + // if they're using some other method. + return id(new AphrontRedirectResponse()) + ->setURI(PhabricatorEnv::getProductionURI($file->getBestURI())); + } + $response->setMimeType($file->getMimeType()); + $response->setDownload($file->getName()); + } + return $response; } } diff --git a/src/applications/files/controller/altview/__init__.php b/src/applications/files/controller/data/__init__.php similarity index 83% rename from src/applications/files/controller/altview/__init__.php rename to src/applications/files/controller/data/__init__.php index 02d23e0bfc..f50147fa13 100644 --- a/src/applications/files/controller/altview/__init__.php +++ b/src/applications/files/controller/data/__init__.php @@ -10,6 +10,7 @@ phutil_require_module('phabricator', 'aphront/response/400'); phutil_require_module('phabricator', 'aphront/response/403'); phutil_require_module('phabricator', 'aphront/response/404'); phutil_require_module('phabricator', 'aphront/response/file'); +phutil_require_module('phabricator', 'aphront/response/redirect'); phutil_require_module('phabricator', 'applications/files/controller/base'); phutil_require_module('phabricator', 'applications/files/storage/file'); phutil_require_module('phabricator', 'infrastructure/env'); @@ -18,4 +19,4 @@ phutil_require_module('phutil', 'parser/uri'); phutil_require_module('phutil', 'utils'); -phutil_require_source('PhabricatorFileAltViewController.php'); +phutil_require_source('PhabricatorFileDataController.php'); diff --git a/src/applications/files/controller/view/PhabricatorFileViewController.php b/src/applications/files/controller/info/PhabricatorFileInfoController.php similarity index 73% rename from src/applications/files/controller/view/PhabricatorFileViewController.php rename to src/applications/files/controller/info/PhabricatorFileInfoController.php index 8d4525db89..e289f40eb0 100644 --- a/src/applications/files/controller/view/PhabricatorFileViewController.php +++ b/src/applications/files/controller/info/PhabricatorFileInfoController.php @@ -16,14 +16,12 @@ * limitations under the License. */ -class PhabricatorFileViewController extends PhabricatorFileController { +final class PhabricatorFileInfoController extends PhabricatorFileController { private $phid; - private $view; public function willProcessRequest(array $data) { $this->phid = $data['phid']; - $this->view = $data['view']; } public function processRequest() { @@ -38,61 +36,6 @@ class PhabricatorFileViewController extends PhabricatorFileController { return new Aphront404Response(); } - switch ($this->view) { - case 'download': - case 'view': - $data = $file->loadFileData(); - $response = new AphrontFileResponse(); - $response->setContent($data); - $response->setCacheDurationInSeconds(60 * 60 * 24 * 30); - - if ($this->view == 'view') { - if (!$file->isViewableInBrowser()) { - return new Aphront400Response(); - } - $download = false; - } else { - $download = true; - } - - if ($download) { - if (!$request->isFormPost()) { - // Require a POST to download files to hinder attacks where you - // on some - // other domain. - return id(new AphrontRedirectResponse()) - ->setURI($file->getInfoURI()); - } - } - - if ($download) { - $mime_type = $file->getMimeType(); - } else { - $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) { - $response->setDownload($file->getName()); - } - return $response; - default: - break; - } - $author_child = null; if ($file->getAuthorPHID()) { $author = id(new PhabricatorUser())->loadOneWhere( @@ -111,11 +54,10 @@ class PhabricatorFileViewController extends PhabricatorFileController { $submit = new AphrontFormSubmitControl(); + $form->setAction($file->getViewURI()); if ($file->isViewableInBrowser()) { - $form->setAction($file->getViewURI()); $submit->setValue('View File'); } else { - $form->setAction('/file/download/'.$file->getPHID().'/'); $submit->setValue('Download File'); } diff --git a/src/applications/files/controller/view/__init__.php b/src/applications/files/controller/info/__init__.php similarity index 71% rename from src/applications/files/controller/view/__init__.php rename to src/applications/files/controller/info/__init__.php index 067d3a607a..5b4f9efe5a 100644 --- a/src/applications/files/controller/view/__init__.php +++ b/src/applications/files/controller/info/__init__.php @@ -6,15 +6,11 @@ -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', 'aphront/response/redirect'); phutil_require_module('phabricator', 'applications/files/controller/base'); phutil_require_module('phabricator', 'applications/files/storage/file'); phutil_require_module('phabricator', 'applications/files/storage/transformed'); 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,8 +19,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'); -phutil_require_source('PhabricatorFileViewController.php'); +phutil_require_source('PhabricatorFileInfoController.php'); diff --git a/src/applications/files/storage/file/PhabricatorFile.php b/src/applications/files/storage/file/PhabricatorFile.php index 2bbbe73721..519e93b174 100644 --- a/src/applications/files/storage/file/PhabricatorFile.php +++ b/src/applications/files/storage/file/PhabricatorFile.php @@ -230,16 +230,8 @@ class PhabricatorFile extends PhabricatorFileDAO { $name = phutil_escape_uri($this->getName()); - $alt = PhabricatorEnv::getEnvConfig('security.alternate-file-domain'); - if ($alt) { - $path = '/file/alt/'.$this->getSecretKey().'/'.$this->getPHID().'/'.$name; - $uri = new PhutilURI($alt); - $uri->setPath($path); - - return (string)$uri; - } else { - return '/file/view/'.$this->getPHID().'/'.$name; - } + $path = '/file/data/'.$this->getSecretKey().'/'.$this->getPHID().'/'.$name; + return PhabricatorEnv::getCDNURI($path); } public function getInfoURI() { diff --git a/src/infrastructure/env/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php index ad049a3980..4f91448eb9 100644 --- a/src/infrastructure/env/PhabricatorEnv.php +++ b/src/infrastructure/env/PhabricatorEnv.php @@ -46,6 +46,16 @@ final class PhabricatorEnv { return rtrim($uri, '/').$path; } + public static function getCDNURI($path) { + $alt = self::getEnvConfig('security.alternate-file-domain'); + if (!$alt) { + $alt = self::getEnvConfig('phabricator.base-uri'); + } + $uri = new PhutilURI($alt); + $uri->setPath($path); + return (string)$uri; + } + public static function getAllConfigKeys() { return self::$env; } diff --git a/src/infrastructure/setup/PhabricatorSetup.php b/src/infrastructure/setup/PhabricatorSetup.php index 110ddb39c8..ca4e8c8c8f 100644 --- a/src/infrastructure/setup/PhabricatorSetup.php +++ b/src/infrastructure/setup/PhabricatorSetup.php @@ -117,8 +117,8 @@ class PhabricatorSetup { 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 ". + "This makes your installation vulnerable to attack. Make sure you ". + "read the documentation for this parameter and understand the ". "consequences of leaving it unconfigured.\n"); }