From ab579f2511102775d27bda92c6f3d3c8fc302d3c Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 28 Feb 2018 15:22:10 -0800 Subject: [PATCH] Never generate file download forms which point to the CDN domain, tighten "form-action" CSP Summary: Depends on D19155. Ref T13094. Ref T4340. We can't currently implement a strict `form-action 'self'` content security policy because some file downloads rely on a `
` which sometimes POSTs to the CDN domain. Broadly, stop generating these forms. We just redirect instead, and show an interstitial confirm dialog if no CDN domain is configured. This makes the UX for installs with no CDN domain a little worse and the UX for everyone else better. Then, implement the stricter Content-Security-Policy. This also removes extra confirm dialogs for downloading Harbormaster build logs and data exports. Test Plan: - Went through the plain data export, data export with bulk jobs, ssh key generation, calendar ICS download, Diffusion data, Paste data, Harbormaster log data, and normal file data download workflows with a CDN domain. - Went through all those workflows again without a CDN domain. - Grepped for affected symbols (`getCDNURI()`, `getDownloadURI()`). - Added an evil form to a page, tried to submit it, was rejected. - Went through the ReCaptcha and Stripe flows again to see if they're submitting any forms. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13094, T4340 Differential Revision: https://secure.phabricator.com/D19156 --- resources/celerity/map.php | 28 ++++++------- .../response/AphrontRedirectResponse.php | 10 +++++ src/aphront/response/AphrontResponse.php | 7 ++++ ...habricatorAuthSSHKeyGenerateController.php | 30 +++++++++----- .../base/controller/PhabricatorController.php | 1 + .../controller/DiffusionServeController.php | 2 +- .../PhabricatorFilesApplication.php | 4 +- .../PhabricatorFileDataController.php | 39 +++++++++---------- .../PhabricatorFileInfoController.php | 3 +- .../PhabricatorEmbedFileRemarkupRule.php | 2 +- .../files/storage/PhabricatorFile.php | 28 ++++++++++--- ...HarbormasterBuildLogDownloadController.php | 14 +------ .../view/HarbormasterBuildLogView.php | 6 ++- ...PhabricatorApplicationSearchController.php | 18 ++++----- .../PhabricatorExportEngineBulkJobType.php | 1 - .../rsrc/externals/javelin/lib/Workflow.js | 7 ++++ 16 files changed, 118 insertions(+), 82 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index a59db7ed05..42a4c854ef 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', 'core.pkg.css' => '2fa91e14', - 'core.pkg.js' => '7aa5bd92', + 'core.pkg.js' => 'e7ce7bba', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => '113e692c', 'differential.pkg.js' => 'f6d809c0', @@ -255,7 +255,7 @@ return array( 'rsrc/externals/javelin/lib/URI.js' => 'c989ade3', 'rsrc/externals/javelin/lib/Vector.js' => '2caa8fb8', 'rsrc/externals/javelin/lib/WebSocket.js' => '3ffe32d6', - 'rsrc/externals/javelin/lib/Workflow.js' => '1e911d0f', + 'rsrc/externals/javelin/lib/Workflow.js' => '0eb1db0c', 'rsrc/externals/javelin/lib/__tests__/Cookie.js' => '5ed109e8', 'rsrc/externals/javelin/lib/__tests__/DOM.js' => 'c984504b', 'rsrc/externals/javelin/lib/__tests__/JSON.js' => '837a7d68', @@ -757,7 +757,7 @@ return array( 'javelin-workboard-card' => 'c587b80f', 'javelin-workboard-column' => '758b4758', 'javelin-workboard-controller' => '26167537', - 'javelin-workflow' => '1e911d0f', + 'javelin-workflow' => '0eb1db0c', 'maniphest-report-css' => '9b9580b7', 'maniphest-task-edit-css' => 'fda62a9b', 'maniphest-task-summary-css' => '11cc5344', @@ -977,6 +977,17 @@ return array( 'javelin-dom', 'javelin-router', ), + '0eb1db0c' => array( + 'javelin-stratcom', + 'javelin-request', + 'javelin-dom', + 'javelin-vector', + 'javelin-install', + 'javelin-util', + 'javelin-mask', + 'javelin-uri', + 'javelin-routable', + ), '0f764c35' => array( 'javelin-install', 'javelin-util', @@ -1035,17 +1046,6 @@ return array( 'javelin-request', 'javelin-uri', ), - '1e911d0f' => array( - 'javelin-stratcom', - 'javelin-request', - 'javelin-dom', - 'javelin-vector', - 'javelin-install', - 'javelin-util', - 'javelin-mask', - 'javelin-uri', - 'javelin-routable', - ), '1f6794f6' => array( 'javelin-behavior', 'javelin-stratcom', diff --git a/src/aphront/response/AphrontRedirectResponse.php b/src/aphront/response/AphrontRedirectResponse.php index 5b4b009796..390ad193c9 100644 --- a/src/aphront/response/AphrontRedirectResponse.php +++ b/src/aphront/response/AphrontRedirectResponse.php @@ -8,6 +8,7 @@ class AphrontRedirectResponse extends AphrontResponse { private $uri; private $stackWhenCreated; private $isExternal; + private $closeDialogBeforeRedirect; public function setIsExternal($external) { $this->isExternal = $external; @@ -37,6 +38,15 @@ class AphrontRedirectResponse extends AphrontResponse { return PhabricatorEnv::getEnvConfig('debug.stop-on-redirect'); } + public function setCloseDialogBeforeRedirect($close) { + $this->closeDialogBeforeRedirect = $close; + return $this; + } + + public function getCloseDialogBeforeRedirect() { + return $this->closeDialogBeforeRedirect; + } + public function getHeaders() { $headers = array(); if (!$this->shouldStopForDebugging()) { diff --git a/src/aphront/response/AphrontResponse.php b/src/aphront/response/AphrontResponse.php index 56d074bd6f..ed3d6d3435 100644 --- a/src/aphront/response/AphrontResponse.php +++ b/src/aphront/response/AphrontResponse.php @@ -147,6 +147,13 @@ abstract class AphrontResponse extends Phobject { // Block relics of the old world: Flash, Java applets, and so on. $csp[] = "object-src 'none'"; + // Don't allow forms to submit offsite. + + // This can result in some trickiness with file downloads if applications + // try to start downloads by submitting a dialog. Redirect to the file's + // download URI instead of submitting a form to it. + $csp[] = "form-action 'self'"; + $csp = implode('; ', $csp); return $csp; diff --git a/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php b/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php index f39707618d..206a2f1c4a 100644 --- a/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php +++ b/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php @@ -24,10 +24,12 @@ final class PhabricatorAuthSSHKeyGenerateController $keys = PhabricatorSSHKeyGenerator::generateKeypair(); list($public_key, $private_key) = $keys; + $key_name = $default_name.'.key'; + $file = PhabricatorFile::newFromFileData( $private_key, array( - 'name' => $default_name.'.key', + 'name' => $key_name, 'ttl.relative' => phutil_units('10 minutes in seconds'), 'viewPolicy' => $viewer->getPHID(), )); @@ -62,25 +64,33 @@ final class PhabricatorAuthSSHKeyGenerateController ->setContentSourceFromRequest($request) ->applyTransactions($key, $xactions); - // NOTE: We're disabling workflow on submit so the download works. We're - // disabling workflow on cancel so the page reloads, showing the new - // key. + $download_link = phutil_tag( + 'a', + array( + 'href' => $file->getDownloadURI(), + ), + array( + id(new PHUIIconView())->setIcon('fa-download'), + ' ', + pht('Download Private Key (%s)', $key_name), + )); + $download_link = phutil_tag('strong', array(), $download_link); + + // NOTE: We're disabling workflow on cancel so the page reloads, showing + // the new key. return $this->newDialog() ->setTitle(pht('Download Private Key')) - ->setDisableWorkflowOnCancel(true) - ->setDisableWorkflowOnSubmit(true) - ->setSubmitURI($file->getDownloadURI()) ->appendParagraph( pht( 'A keypair has been generated, and the public key has been '. - 'added as a recognized key. Use the button below to download '. - 'the private key.')) + 'added as a recognized key.')) + ->appendParagraph($download_link) ->appendParagraph( pht( 'After you download the private key, it will be destroyed. '. 'You will not be able to retrieve it if you lose your copy.')) - ->addSubmitButton(pht('Download Private Key')) + ->setDisableWorkflowOnCancel(true) ->addCancelButton($cancel_uri, pht('Done')); } diff --git a/src/applications/base/controller/PhabricatorController.php b/src/applications/base/controller/PhabricatorController.php index 5952a806ed..df0c94c13d 100644 --- a/src/applications/base/controller/PhabricatorController.php +++ b/src/applications/base/controller/PhabricatorController.php @@ -298,6 +298,7 @@ abstract class PhabricatorController extends AphrontController { ->setContent( array( 'redirect' => $response->getURI(), + 'close' => $response->getCloseDialogBeforeRedirect(), )); } } diff --git a/src/applications/diffusion/controller/DiffusionServeController.php b/src/applications/diffusion/controller/DiffusionServeController.php index c0f72ae404..ac0b993b2f 100644 --- a/src/applications/diffusion/controller/DiffusionServeController.php +++ b/src/applications/diffusion/controller/DiffusionServeController.php @@ -1046,7 +1046,7 @@ final class DiffusionServeController extends DiffusionController { // $no_authorization = 'Basic '.base64_encode('none'); - $get_uri = $file->getCDNURI(); + $get_uri = $file->getCDNURI('data'); $actions['download'] = array( 'href' => $get_uri, 'header' => array( diff --git a/src/applications/files/application/PhabricatorFilesApplication.php b/src/applications/files/application/PhabricatorFilesApplication.php index c3903ffdce..2d3a1b35c6 100644 --- a/src/applications/files/application/PhabricatorFilesApplication.php +++ b/src/applications/files/application/PhabricatorFilesApplication.php @@ -101,7 +101,7 @@ final class PhabricatorFilesApplication extends PhabricatorApplication { private function getResourceSubroutes() { return array( - 'data/'. + '(?Pdata|download)/'. '(?:@(?P[^/]+)/)?'. '(?P[^/]+)/'. '(?P[^/]+)/'. @@ -132,7 +132,7 @@ final class PhabricatorFilesApplication extends PhabricatorApplication { public function getQuicksandURIPatternBlacklist() { return array( - '/file/data/.*', + '/file/(data|download)/.*', ); } diff --git a/src/applications/files/controller/PhabricatorFileDataController.php b/src/applications/files/controller/PhabricatorFileDataController.php index 98f2cdbd51..91dfb4fa4f 100644 --- a/src/applications/files/controller/PhabricatorFileDataController.php +++ b/src/applications/files/controller/PhabricatorFileDataController.php @@ -26,6 +26,9 @@ final class PhabricatorFileDataController extends PhabricatorFileController { $req_domain = $request->getHost(); $main_domain = id(new PhutilURI($base_uri))->getDomain(); + $request_kind = $request->getURIData('kind'); + $is_download = ($request_kind === 'download'); + if (!strlen($alt) || $main_domain == $alt_domain) { // No alternate domain. $should_redirect = false; @@ -50,7 +53,7 @@ final class PhabricatorFileDataController extends PhabricatorFileController { if ($should_redirect) { return id(new AphrontRedirectResponse()) ->setIsExternal(true) - ->setURI($file->getCDNURI()); + ->setURI($file->getCDNURI($request_kind)); } $response = new AphrontFileResponse(); @@ -71,34 +74,30 @@ final class PhabricatorFileDataController extends PhabricatorFileController { } $is_viewable = $file->isViewableInBrowser(); - $force_download = $request->getExists('download'); - $request_type = $request->getHTTPHeader('X-Phabricator-Request-Type'); $is_lfs = ($request_type == 'git-lfs'); - if ($is_viewable && !$force_download) { + if ($is_viewable && !$is_download) { $response->setMimeType($file->getViewableMimeType()); } else { - $is_public = !$viewer->isLoggedIn(); $is_post = $request->isHTTPPost(); - // NOTE: Require POST to download files from the primary domain if the - // request includes credentials. The "Download File" links we generate - // in the web UI are forms which use POST to satisfy this requirement. + // NOTE: Require POST to download files from the primary domain. If the + // request is not a POST request but arrives on the primary domain, we + // render a confirmation dialog. For discussion, see T13094. - // The intent is to make attacks based on tags like "