1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-26 16:52:41 +01:00

Prevent CSRF uploads via /file/dropupload/

Summary:
We don't currently validate CSRF tokens on this workflow. This allows an
attacker to upload arbitrary files on the user's behalf. Although I believe the
tight list of servable mime-types means that's more or less the end of the
attack, this is still a vulnerability.

In the long term, the right solution is probably to pass CSRF tokens on all Ajax
requests in an HTTP header (or just a GET param) or something like that.
However, this endpoint is unique and this is the quickest and most direct way to
close the hole.

Test Plan:
  - Drop-uploaded files to Files, Maniphest, Phriction and Differential.
  - Modified CSRF vaidator to use __csrf__.'x' and verified uploads and form
submissions don't work.

Reviewers: andrewjcg, aran, jungejason, tuomaspelkonen, erling
Commenters: andrewjcg, pedram
CC: aran, epriestley, andrewjcg, pedram
Differential Revision: 758
This commit is contained in:
epriestley 2011-08-01 20:23:01 -07:00
parent 735847865c
commit 3aa17c7443
10 changed files with 75 additions and 17 deletions

View file

@ -738,7 +738,7 @@ celerity_register_resource_map(array(
), ),
'javelin-behavior-refresh-csrf' => 'javelin-behavior-refresh-csrf' =>
array( array(
'uri' => '/res/39aa51f7/rsrc/js/application/core/behavior-refresh-csrf.js', 'uri' => '/res/a8204a73/rsrc/js/application/core/behavior-refresh-csrf.js',
'type' => 'js', 'type' => 'js',
'requires' => 'requires' =>
array( array(

View file

@ -117,15 +117,33 @@ class AphrontRequest {
return $this->getExists(self::TYPE_AJAX); return $this->getExists(self::TYPE_AJAX);
} }
final public function isFormPost() { public static function getCSRFTokenName() {
$post = $this->getExists(self::TYPE_FORM) && return '__csrf__';
$this->isHTTPPost();
if (!$post) {
return false;
} }
$valid = $this->getUser()->validateCSRFToken($this->getStr('__csrf__')); public static function getCSRFHeaderName() {
return 'X-Phabricator-Csrf';
}
final public function validateCSRF() {
$token_name = self::getCSRFTokenName();
$token = $this->getStr($token_name);
// No token in the request, check the HTTP header which is added for Ajax
// requests.
if (empty($token)) {
// PHP mangles HTTP headers by uppercasing them and replacing hyphens with
// underscores, then prepending 'HTTP_'.
$php_index = self::getCSRFHeaderName();
$php_index = strtoupper($php_index);
$php_index = str_replace('-', '_', $php_index);
$php_index = 'HTTP_'.$php_index;
$token = idx($_SERVER, $php_index);
}
$valid = $this->getUser()->validateCSRFToken($token);
if (!$valid) { if (!$valid) {
// This should only be able to happen if you load a form, pull your // This should only be able to happen if you load a form, pull your
// internet for 6 hours, and then reconnect and immediately submit, // internet for 6 hours, and then reconnect and immediately submit,
@ -143,6 +161,17 @@ class AphrontRequest {
return true; return true;
} }
final public function isFormPost() {
$post = $this->getExists(self::TYPE_FORM) &&
$this->isHTTPPost();
if (!$post) {
return false;
}
return $this->validateCSRF();
}
final public function getCookie($name, $default = null) { final public function getCookie($name, $default = null) {
return idx($_COOKIE, $name, $default); return idx($_COOKIE, $name, $default);
} }

View file

@ -22,6 +22,9 @@ class PhabricatorFileDropUploadController extends PhabricatorFileController {
$request = $this->getRequest(); $request = $this->getRequest();
$user = $request->getUser(); $user = $request->getUser();
// NOTE: Throws if valid CSRF token is not present in the request.
$request->validateCSRF();
$data = file_get_contents('php://input'); $data = file_get_contents('php://input');
$name = $request->getStr('name'); $name = $request->getStr('name');

View file

@ -6,6 +6,7 @@
phutil_require_module('phabricator', 'aphront/request');
phutil_require_module('phabricator', 'infrastructure/celerity/api'); phutil_require_module('phabricator', 'infrastructure/celerity/api');
phutil_require_module('phutil', 'markup'); phutil_require_module('phutil', 'markup');

View file

@ -56,7 +56,7 @@ function phabricator_render_form(PhabricatorUser $user, $attributes, $content) {
'input', 'input',
array( array(
'type' => 'hidden', 'type' => 'hidden',
'name' => '__csrf__', 'name' => AphrontRequest::getCSRFTokenName(),
'value' => $user->getCSRFToken(), 'value' => $user->getCSRFToken(),
)). )).
phutil_render_tag( phutil_render_tag(

View file

@ -92,7 +92,7 @@ final class AphrontFormView extends AphrontView {
$data = $this->data + array( $data = $this->data + array(
'__form__' => 1, '__form__' => 1,
'__csrf__' => $this->user->getCSRFToken(), AphrontRequest::getCSRFTokenName() => $this->user->getCSRFToken(),
); );
$inputs = array(); $inputs = array();
foreach ($data as $key => $value) { foreach ($data as $key => $value) {

View file

@ -6,6 +6,7 @@
phutil_require_module('phabricator', 'aphront/request');
phutil_require_module('phabricator', 'infrastructure/celerity/api'); phutil_require_module('phabricator', 'infrastructure/celerity/api');
phutil_require_module('phabricator', 'infrastructure/javelin/api'); phutil_require_module('phabricator', 'infrastructure/javelin/api');
phutil_require_module('phabricator', 'infrastructure/javelin/markup'); phutil_require_module('phabricator', 'infrastructure/javelin/markup');

View file

@ -121,8 +121,23 @@ class PhabricatorStandardPageView extends AphrontPageView {
require_celerity_resource('phabricator-core-buttons-css'); require_celerity_resource('phabricator-core-buttons-css');
require_celerity_resource('phabricator-standard-page-view'); require_celerity_resource('phabricator-standard-page-view');
$current_token = null;
$request = $this->getRequest();
if ($request) {
$user = $request->getUser();
if ($user) {
$current_token = $user->getCSRFToken();
}
}
Javelin::initBehavior('workflow', array()); Javelin::initBehavior('workflow', array());
Javelin::initBehavior('refresh-csrf', array()); Javelin::initBehavior(
'refresh-csrf',
array(
'tokenName' => AphrontRequest::getCSRFTokenName(),
'header' => AphrontRequest::getCSRFHeaderName(),
'current' => $current_token,
));
Javelin::initBehavior( Javelin::initBehavior(
'phabricator-keyboard-shortcuts', 'phabricator-keyboard-shortcuts',
array( array(

View file

@ -6,6 +6,7 @@
phutil_require_module('phabricator', 'aphront/request');
phutil_require_module('phabricator', 'applications/people/storage/preferences'); phutil_require_module('phabricator', 'applications/people/storage/preferences');
phutil_require_module('phabricator', 'infrastructure/celerity/api'); phutil_require_module('phabricator', 'infrastructure/celerity/api');
phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phabricator', 'infrastructure/env');

View file

@ -27,11 +27,14 @@
*/ */
JX.behavior('refresh-csrf', function(config) { JX.behavior('refresh-csrf', function(config) {
var current_token = config.current;
function refresh_csrf() { function refresh_csrf() {
new JX.Request('/login/refresh/', function(r) { new JX.Request('/login/refresh/', function(r) {
current_token = r.token;
var inputs = JX.DOM.scry(document.body, 'input'); var inputs = JX.DOM.scry(document.body, 'input');
for (var ii = 0; ii < inputs.length; ii++) { for (var ii = 0; ii < inputs.length; ii++) {
if (inputs[ii].name == '__csrf__') { if (inputs[ii].name == config.tokenName) {
inputs[ii].value = r.token; inputs[ii].value = r.token;
} }
} }
@ -41,4 +44,9 @@ JX.behavior('refresh-csrf', function(config) {
// Refresh the CSRF tokens every 55 minutes. // Refresh the CSRF tokens every 55 minutes.
setInterval(refresh_csrf, 1000 * 60 * 55); setInterval(refresh_csrf, 1000 * 60 * 55);
// Additionally, add the CSRF token as an HTTP header to every AJAX request.
JX.Request.listen('open', function(r) {
r.getTransport().setRequestHeader(config.header, current_token);
});
}); });