From d3e700ce193195d0789fa3d0d930239fe7b545f9 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 7 Aug 2013 16:09:25 -0700
Subject: [PATCH] Further mitigate BREACH by reducing reflectiveness

Summary:
Ref T3684. The URI itself is reflected in a few places. It is generally not dangerous because we only let you add random stuff to the end of it for one or two controllers (e.g., the file download controller lets you add "/whatever.jpg"), but:

  - Remove it entirely in the main request, since it serves no purpose.
  - Remove query parameters in Ajax requests. These are available in DarkConsole proper.

Also mask a few things in the "Request" tab; I've never used these fields when debugging or during support, and they leak quasi-sensitive information that could get screenshotted or over-the-shoulder'd.

I didn't mitgate `__metablock__` because I think the threat is so close to 0 that it's not worthwhile.

Test Plan: Used Darkconsole, examined Requests tab.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3684

Differential Revision: https://secure.phabricator.com/D6699
---
 .../plugin/DarkConsoleRequestPlugin.php       | 19 +++++++++++++++----
 src/aphront/response/AphrontAjaxResponse.php  |  9 ++++++++-
 src/view/page/PhabricatorStandardPageView.php |  5 ++++-
 3 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/src/aphront/console/plugin/DarkConsoleRequestPlugin.php b/src/aphront/console/plugin/DarkConsoleRequestPlugin.php
index 0d04d58dc0..5d43f93745 100644
--- a/src/aphront/console/plugin/DarkConsoleRequestPlugin.php
+++ b/src/aphront/console/plugin/DarkConsoleRequestPlugin.php
@@ -38,14 +38,25 @@ final class DarkConsoleRequestPlugin extends DarkConsolePlugin {
 
     $sections = array_merge($sections, $data);
 
+    $mask = array(
+      'HTTP_COOKIE' => true,
+      'HTTP_X_PHABRICATOR_CSRF' => true,
+    );
+
     $out = array();
     foreach ($sections as $header => $map) {
       $rows = array();
       foreach ($map as $key => $value) {
-        $rows[] = array(
-          $key,
-          (is_array($value) ? json_encode($value) : $value),
-        );
+        if (isset($mask[$key])) {
+          $rows[] = array(
+            $key,
+            phutil_tag('em', array(), '(Masked)'));
+        } else {
+          $rows[] = array(
+            $key,
+            (is_array($value) ? json_encode($value) : $value),
+          );
+        }
       }
 
       $table = new AphrontTableView($rows);
diff --git a/src/aphront/response/AphrontAjaxResponse.php b/src/aphront/response/AphrontAjaxResponse.php
index 61542225ad..032f11e120 100644
--- a/src/aphront/response/AphrontAjaxResponse.php
+++ b/src/aphront/response/AphrontAjaxResponse.php
@@ -37,10 +37,17 @@ final class AphrontAjaxResponse extends AphrontResponse {
   public function buildResponseString() {
     $console = $this->getConsole();
     if ($console) {
+      // NOTE: We're stripping query parameters here both for readability and
+      // to mitigate BREACH and similar attacks. The parameters are available
+      // in the "Request" tab, so this should not impact usability. See T3684.
+      $uri = $this->getRequest()->getRequestURI();
+      $uri = new PhutilURI($uri);
+      $uri->setQueryParams(array());
+
       Javelin::initBehavior(
         'dark-console',
         array(
-          'uri'     => (string)$this->getRequest()->getRequestURI(),
+          'uri'     => (string)$uri,
           'key'     => $console->getKey($this->getRequest()),
           'color'   => $console->getColor(),
         ));
diff --git a/src/view/page/PhabricatorStandardPageView.php b/src/view/page/PhabricatorStandardPageView.php
index 0643c8b2ab..9771a9f23f 100644
--- a/src/view/page/PhabricatorStandardPageView.php
+++ b/src/view/page/PhabricatorStandardPageView.php
@@ -199,7 +199,10 @@ final class PhabricatorStandardPageView extends PhabricatorBarePageView {
       Javelin::initBehavior(
         'dark-console',
         array(
-          'uri' => $request ? (string)$request->getRequestURI() : '?',
+          // NOTE: We use a generic label here to prevent input reflection
+          // and mitigate compression attacks like BREACH. See discussion in
+          // T3684.
+          'uri' => pht('Main Request'),
           'selected' => $user ? $user->getConsoleTab() : null,
           'visible'  => $user ? (int)$user->getConsoleVisible() : true,
           'headers' => $headers,