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"); }