1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2025-01-19 03:01:11 +01:00

Prevent file download without POST + CSRF

Summary: This prevents <applet /> attacks unless the attacker can upload an
applet which has a viewable MIME type as detected by `file`. I'm not sure if
this is possible or not. It should, at least, narrow the attack window. There
are no real tradeoffs here, this is probably a strictly better application
behavior regardless of the security issues.
Test Plan:
  - Tried to download a file via GET, got redirected to info.
  - Downloaded a file via POST + CSRF from the info page.

Reviewers: andrewjcg, erling, aran, jungejason, tuomaspelkonen
CC: aran
Differential Revision: 759
This commit is contained in:
epriestley 2011-08-01 21:01:37 -07:00
parent 3aa17c7443
commit 355b753df7
3 changed files with 12 additions and 10 deletions

View file

@ -96,7 +96,7 @@ class PhabricatorFileListController extends PhabricatorFileController {
phutil_render_tag( phutil_render_tag(
'a', 'a',
array( array(
'href' => $file->getViewURI(), 'href' => $file->getBestURI(),
), ),
phutil_escape_html($file->getName())), phutil_escape_html($file->getName())),
phutil_escape_html(number_format($file->getByteSize()).' bytes'), phutil_escape_html(number_format($file->getByteSize()).' bytes'),
@ -108,13 +108,6 @@ class PhabricatorFileListController extends PhabricatorFileController {
), ),
'Info'), 'Info'),
$view_button, $view_button,
phutil_render_tag(
'a',
array(
'class' => 'small button grey',
'href' => '/file/download/'.$file->getPHID().'/',
),
'Download'),
phabricator_date($file->getDateCreated(), $user), phabricator_date($file->getDateCreated(), $user),
phabricator_time($file->getDateCreated(), $user), phabricator_time($file->getDateCreated(), $user),
); );
@ -130,7 +123,6 @@ class PhabricatorFileListController extends PhabricatorFileController {
'Size', 'Size',
'', '',
'', '',
'',
'Created', 'Created',
'', '',
)); ));
@ -142,7 +134,6 @@ class PhabricatorFileListController extends PhabricatorFileController {
'right', 'right',
'action', 'action',
'action', 'action',
'action',
'', '',
'right', 'right',
)); ));

View file

@ -55,6 +55,16 @@ class PhabricatorFileViewController extends PhabricatorFileController {
$download = true; $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) { if ($download) {
$mime_type = $file->getMimeType(); $mime_type = $file->getMimeType();
} else { } else {

View file

@ -9,6 +9,7 @@
phutil_require_module('phabricator', 'aphront/response/400'); phutil_require_module('phabricator', 'aphront/response/400');
phutil_require_module('phabricator', 'aphront/response/404'); phutil_require_module('phabricator', 'aphront/response/404');
phutil_require_module('phabricator', 'aphront/response/file'); 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/controller/base');
phutil_require_module('phabricator', 'applications/files/storage/file'); phutil_require_module('phabricator', 'applications/files/storage/file');
phutil_require_module('phabricator', 'applications/files/storage/transformed'); phutil_require_module('phabricator', 'applications/files/storage/transformed');