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 `<form />` 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
2024-11-22 14:52:41 +01:00 · 2018-02-28 15:22:10 -08:00 · 2018-02-28 15:22:10 -08:00 · ab579f2511
commit ab579f2511
parent afc98f5d5d
16 changed files with 118 additions and 82 deletions
--- 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',
--- 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()) {
--- 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;
--- 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'));
    }

--- 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(),
            ));
      }
    }
--- a/src/applications/diffusion/controller/DiffusionServeController.php
+++ b/src/applications/diffusion/controller/DiffusionServeController.php
@ -1046,7 +1046,7 @@ final class DiffusionServeController extends DiffusionController {
          // <https://github.com/github/git-lfs/issues/1088>
          $no_authorization = 'Basic '.base64_encode('none');

-          $get_uri = $file->getCDNURI();
+          $get_uri = $file->getCDNURI('data');
          $actions['download'] = array(
            'href' => $get_uri,
            'header' => array(
--- 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/'.
+      '(?P<kind>data|download)/'.
        '(?:@(?P<instance>[^/]+)/)?'.
        '(?P<key>[^/]+)/'.
        '(?P<phid>[^/]+)/'.
@ -132,7 +132,7 @@ final class PhabricatorFilesApplication extends PhabricatorApplication {

  public function getQuicksandURIPatternBlacklist() {
    return array(
-      '/file/data/.*',
+      '/file/(data|download)/.*',
    );
  }

--- 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 "<iframe />" and
-      // "<script />" (which can issue GET requests, but can not easily issue
-      // POST requests) more difficult to execute.
-
-      // The best defense against these attacks is to use an alternate file
-      // domain, which is why we strongly recommend doing so.
-
-      $is_safe = ($is_alternate_domain || $is_lfs || $is_post || $is_public);
+      $is_safe = ($is_alternate_domain || $is_lfs || $is_post);
      if (!$is_safe) {
-        // This is marked as "external" because it is fully qualified.
-        return id(new AphrontRedirectResponse())
-          ->setIsExternal(true)
-          ->setURI(PhabricatorEnv::getProductionURI($file->getBestURI()));
+        return $this->newDialog()
+          ->setSubmitURI($file->getDownloadURI())
+          ->setTitle(pht('Download File'))
+          ->appendParagraph(
+            pht(
+              'Download file %s (%s)?',
+              phutil_tag('strong', array(), $file->getName()),
+              phutil_format_bytes($file->getByteSize())))
+          ->addCancelButton($file->getURI())
+          ->addSubmitButton(pht('Download File'));
      }

      $response->setMimeType($file->getMimeType());
--- a/src/applications/files/controller/PhabricatorFileInfoController.php
+++ b/src/applications/files/controller/PhabricatorFileInfoController.php
@ -137,11 +137,10 @@ final class PhabricatorFileInfoController extends PhabricatorFileController {
      $curtain->addAction(
        id(new PhabricatorActionView())
          ->setUser($viewer)
-          ->setRenderAsForm($can_download)
          ->setDownload($can_download)
          ->setName(pht('Download File'))
          ->setIcon('fa-download')
-          ->setHref($file->getViewURI())
+          ->setHref($file->getDownloadURI())
          ->setDisabled(!$can_download)
          ->setWorkflow(!$can_download));
    }
--- a/src/applications/files/markup/PhabricatorEmbedFileRemarkupRule.php
+++ b/src/applications/files/markup/PhabricatorEmbedFileRemarkupRule.php
@ -144,7 +144,7 @@ final class PhabricatorEmbedFileRemarkupRule

          $existing_xform = $file->getTransform($preview_key);
          if ($existing_xform) {
-            $xform_uri = $existing_xform->getCDNURI();
+            $xform_uri = $existing_xform->getCDNURI('data');
          } else {
            $xform_uri = $file->getURIForTransform($xform);
          }
--- a/src/applications/files/storage/PhabricatorFile.php
+++ b/src/applications/files/storage/PhabricatorFile.php
@ -810,16 +810,24 @@ final class PhabricatorFile extends PhabricatorFileDAO
        pht('You must save a file before you can generate a view URI.'));
    }

-    return $this->getCDNURI();
+    return $this->getCDNURI('data');
  }

-  public function getCDNURI() {
+  public function getCDNURI($request_kind) {
+    if (($request_kind !== 'data') &&
+        ($request_kind !== 'download')) {
+      throw new Exception(
+        pht(
+          'Unknown file content request kind "%s".',
+          $request_kind));
+    }
+
    $name = self::normalizeFileName($this->getName());
    $name = phutil_escape_uri($name);

    $parts = array();
    $parts[] = 'file';
-    $parts[] = 'data';
+    $parts[] = $request_kind;

    // If this is an instanced install, add the instance identifier to the URI.
    // Instanced configurations behind a CDN may not be able to control the
@ -861,9 +869,7 @@ final class PhabricatorFile extends PhabricatorFileDAO
  }

  public function getDownloadURI() {
-    $uri = id(new PhutilURI($this->getViewURI()))
-      ->setQueryParam('download', true);
-    return (string)$uri;
+    return $this->getCDNURI('download');
  }

  public function getURIForTransform(PhabricatorFileTransform $transform) {
@ -1469,6 +1475,16 @@ final class PhabricatorFile extends PhabricatorFileDAO
      ->setURI($uri);
  }

+  public function newDownloadResponse() {
+    // We're cheating a little bit here and relying on the fact that
+    // getDownloadURI() always returns a fully qualified URI with a complete
+    // domain.
+    return id(new AphrontRedirectResponse())
+      ->setIsExternal(true)
+      ->setCloseDialogBeforeRedirect(true)
+      ->setURI($this->getDownloadURI());
+  }
+
  public function attachTransforms(array $map) {
    $this->transforms = $map;
    return $this;
--- a/src/applications/harbormaster/controller/HarbormasterBuildLogDownloadController.php
+++ b/src/applications/harbormaster/controller/HarbormasterBuildLogDownloadController.php
@ -44,19 +44,7 @@ final class HarbormasterBuildLogDownloadController
        ->addCancelButton($cancel_uri);
    }

-    $size = $file->getByteSize();
-
-    return $this->newDialog()
-      ->setTitle(pht('Download Build Log'))
-      ->appendParagraph(
-        pht(
-          'This log has a total size of %s. If you insist, you may '.
-          'download it.',
-          phutil_tag('strong', array(), phutil_format_bytes($size))))
-      ->setDisableWorkflowOnSubmit(true)
-      ->addSubmitButton(pht('Download Log'))
-      ->setSubmitURI($file->getDownloadURI())
-      ->addCancelButton($cancel_uri, pht('Done'));
+    return $file->newDownloadResponse();
  }

 }
--- a/src/applications/harbormaster/view/HarbormasterBuildLogView.php
+++ b/src/applications/harbormaster/view/HarbormasterBuildLogView.php
@ -34,12 +34,14 @@ final class HarbormasterBuildLogView extends AphrontView {

    $download_uri = "/harbormaster/log/download/{$id}/";

+    $can_download = (bool)$log->getFilePHID();
+
    $download_button = id(new PHUIButtonView())
      ->setTag('a')
      ->setHref($download_uri)
      ->setIcon('fa-download')
-      ->setDisabled(!$log->getFilePHID())
-      ->setWorkflow(true)
+      ->setDisabled(!$can_download)
+      ->setWorkflow(!$can_download)
      ->setText(pht('Download Log'));

    $header->addActionLink($download_button);
--- a/src/applications/search/controller/PhabricatorApplicationSearchController.php
+++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php
@ -512,15 +512,7 @@ final class PhabricatorApplicationSearchController
            ->setURI($job->getMonitorURI());
        } else {
          $file = $export_engine->exportFile();
-
-          return $this->newDialog()
-            ->setTitle(pht('Download Results'))
-            ->appendParagraph(
-              pht('Click the download button to download the exported data.'))
-            ->addCancelButton($cancel_uri, pht('Done'))
-            ->setSubmitURI($file->getDownloadURI())
-            ->setDisableWorkflowOnSubmit(true)
-            ->addSubmitButton(pht('Download Data'));
+          return $file->newDownloadResponse();
        }
      }
    }
@ -535,12 +527,18 @@ final class PhabricatorApplicationSearchController
          ->setValue($format_key)
          ->setOptions($format_options));

+    if ($is_large_export) {
+      $submit_button = pht('Continue');
+    } else {
+      $submit_button = pht('Download Data');
+    }
+
    return $this->newDialog()
      ->setTitle(pht('Export Results'))
      ->setErrors($errors)
      ->appendForm($export_form)
      ->addCancelButton($cancel_uri)
-      ->addSubmitButton(pht('Continue'));
+      ->addSubmitButton($submit_button);
  }

  private function processEditRequest() {
--- a/src/infrastructure/export/engine/PhabricatorExportEngineBulkJobType.php
+++ b/src/infrastructure/export/engine/PhabricatorExportEngineBulkJobType.php
@ -36,7 +36,6 @@ final class PhabricatorExportEngineBulkJobType
          ->setName(pht('Temporary File Expired'));
      } else {
        $actions[] = id(new PhabricatorActionView())
-          ->setRenderAsForm(true)
          ->setHref($file->getDownloadURI())
          ->setIcon('fa-download')
          ->setName(pht('Download Data Export'));
--- a/webroot/rsrc/externals/javelin/lib/Workflow.js
+++ b/webroot/rsrc/externals/javelin/lib/Workflow.js
@ -276,6 +276,13 @@ JX.install('Workflow', {
      // It is permissible to send back a falsey redirect to force a page
      // reload, so we need to take this branch if the key is present.
      if (r && (typeof r.redirect != 'undefined')) {
+        // Before we redirect to file downloads, we close the dialog. These
+        // redirects aren't real navigation events so we end up stuck in the
+        // dialog otherwise.
+        if (r.close) {
+          this._pop();
+        }
+
        JX.$U(r.redirect).go();
      } else if (r && r.dialog) {
        this._push();