archive/phabricator
1
0
Fork 0
Code Issues 1 Pull Requests Packages Projects Releases Wiki Activity

Move ALL files to serve from the alternate file domain, not just files without

Browse Source 
"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
This commit is contained in:
epriestley
2012-02-14 14:52:27 -08:00
parent c8b4bfdcd1
commit 549146bc7c
10 changed files with 62 additions and 123 deletions
Download Patch File Download Diff File Expand all files Collapse all files

33
src/applications/files/controller/altview/PhabricatorFileAltViewController.php → src/applications/files/controller/data/PhabricatorFileDataController.php
View File

@@ -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;
}
}

3
src/applications/files/controller/altview/__init__.php → src/applications/files/controller/data/__init__.php
View File

@@ -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');

62
src/applications/files/controller/view/PhabricatorFileViewController.php → src/applications/files/controller/info/PhabricatorFileInfoController.php
View File

@@ -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
// <applet src="http://phabricator.example.com/file/..." /> 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');
}

7
src/applications/files/controller/view/__init__.php → src/applications/files/controller/info/__init__.php
View File

@@ -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');