From c8b4bfdcd1cc2f5aab54ee41eb43039b8ca650d7 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 14 Feb 2012 14:51:51 -0800
Subject: [PATCH] Encode "<" and ">" in JSON/Ajax responses to prevent
 content-sniffing attacks

Summary:
Some browsers will still sniff content types even with "Content-Type" and
"X-Content-Type-Options: nosniff". Encode "<" and ">" to prevent them from
sniffing the content as HTML.

See T865.

Also unified some of the code on this pathway.

Test Plan: Verified Opera no longer sniffs the Conduit response into HTML for
the test case in T865. Unit tests pass.

Reviewers: cbg, btrahan

Reviewed By: cbg

CC: aran, epriestley

Maniphest Tasks: T139, T865

Differential Revision: https://secure.phabricator.com/D1606
---
 scripts/conduit/api.php                       |  2 +-
 src/__phutil_library_map__.php                |  2 +
 .../response/ajax/AphrontAjaxResponse.php     | 10 ++--
 src/aphront/response/base/AphrontResponse.php | 34 +++++++++++++-
 .../response/json/AphrontJSONResponse.php     | 46 +++++++++++++++++++
 src/aphront/response/json/__init__.php        | 12 +++++
 src/aphront/sink/base/AphrontHTTPSink.php     | 17 +++++++
 .../__tests__/AphrontHTTPSinkTestCase.php     | 16 +++++++
 src/aphront/sink/base/__tests__/__init__.php  |  3 ++
 .../api/PhabricatorConduitAPIController.php   |  5 +-
 .../conduit/controller/api/__init__.php       |  2 +-
 .../protocol/response/ConduitAPIResponse.php  |  4 --
 .../CelerityStaticResourceResponse.php        |  4 +-
 webroot/index.php                             |  2 +
 14 files changed, 143 insertions(+), 16 deletions(-)
 create mode 100644 src/aphront/response/json/AphrontJSONResponse.php
 create mode 100644 src/aphront/response/json/__init__.php

diff --git a/scripts/conduit/api.php b/scripts/conduit/api.php
index 2844c2f393..e2da82ce6e 100644
--- a/scripts/conduit/api.php
+++ b/scripts/conduit/api.php
@@ -85,7 +85,7 @@ $response = id(new ConduitAPIResponse())
   ->setResult($result)
   ->setErrorCode($error_code)
   ->setErrorInfo($error_info);
-echo $response->toJSON(), "\n";
+echo json_encode($response->toDictionary()), "\n";
 
 // TODO -- how get $connection_id from SSH?
 $connection_id = null;
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index fb58757510..77037eed33 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -55,6 +55,7 @@ phutil_register_library_map(array(
     'AphrontIsolatedDatabaseConnection' => 'storage/connection/isolated',
     'AphrontIsolatedDatabaseConnectionTestCase' => 'storage/connection/isolated/__tests__',
     'AphrontIsolatedHTTPSink' => 'aphront/sink/test',
+    'AphrontJSONResponse' => 'aphront/response/json',
     'AphrontJavelinView' => 'view/javelin-view',
     'AphrontKeyboardShortcutsAvailableView' => 'view/widget/keyboardshortcuts',
     'AphrontListFilterView' => 'view/layout/listfilter',
@@ -880,6 +881,7 @@ phutil_register_library_map(array(
     'AphrontIsolatedDatabaseConnection' => 'AphrontDatabaseConnection',
     'AphrontIsolatedDatabaseConnectionTestCase' => 'PhabricatorTestCase',
     'AphrontIsolatedHTTPSink' => 'AphrontHTTPSink',
+    'AphrontJSONResponse' => 'AphrontResponse',
     'AphrontJavelinView' => 'AphrontView',
     'AphrontKeyboardShortcutsAvailableView' => 'AphrontView',
     'AphrontListFilterView' => 'AphrontView',
diff --git a/src/aphront/response/ajax/AphrontAjaxResponse.php b/src/aphront/response/ajax/AphrontAjaxResponse.php
index b64d053f17..a22cbc6eeb 100644
--- a/src/aphront/response/ajax/AphrontAjaxResponse.php
+++ b/src/aphront/response/ajax/AphrontAjaxResponse.php
@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -19,7 +19,7 @@
 /**
  * @group aphront
  */
-class AphrontAjaxResponse extends AphrontResponse {
+final class AphrontAjaxResponse extends AphrontResponse {
 
   private $content;
   private $error;
@@ -31,9 +31,13 @@ class AphrontAjaxResponse extends AphrontResponse {
 
   public function buildResponseString() {
     $response = CelerityAPI::getStaticResourceResponse();
-    return $response->renderAjaxResponse(
+    $object = $response->buildAjaxResponse(
       $this->content,
       $this->error);
+
+    return $this->encodeJSONForHTTPResponse(
+      $object,
+      $use_javelin_shield = true);
   }
 
   public function getHeaders() {
diff --git a/src/aphront/response/base/AphrontResponse.php b/src/aphront/response/base/AphrontResponse.php
index c4b4ce8b31..6c32b7f744 100644
--- a/src/aphront/response/base/AphrontResponse.php
+++ b/src/aphront/response/base/AphrontResponse.php
@@ -70,6 +70,37 @@ abstract class AphrontResponse {
     return $this;
   }
 
+  protected function encodeJSONForHTTPResponse(
+    array $object,
+    $use_javelin_shield) {
+
+    $response = json_encode($object);
+
+    // Prevent content sniffing attacks by encoding "<" and ">", so browsers
+    // won't try to execute the document as HTML even if they ignore
+    // Content-Type and X-Content-Type-Options. See T865.
+    $response = str_replace(
+      array('<', '>'),
+      array('\u003c', '\u003e'),
+      $response);
+
+    // Add a shield to prevent "JSON Hijacking" attacks where an attacker
+    // requests a JSON response using a normal <script /> tag and then uses
+    // Object.prototype.__defineSetter__() or similar to read response data.
+    // This header causes the browser to loop infinitely instead of handing over
+    // sensitive data.
+
+    // TODO: This is massively stupid: Javelin and Conduit use different
+    // shields.
+    $shield = $use_javelin_shield
+      ? 'for (;;);'
+      : 'for(;;);';
+
+    $response = $shield.$response;
+
+    return $response;
+  }
+
   public function getCacheHeaders() {
     $headers = array();
     if ($this->cacheable) {
@@ -94,7 +125,8 @@ abstract class AphrontResponse {
     // IE has a feature where it may override an explicit Content-Type
     // declaration by inferring a content type. This can be a security risk
     // and we always explicitly transmit the correct Content-Type header, so
-    // prevent IE from using inferred content types.
+    // prevent IE from using inferred content types. This only offers protection
+    // on recent versions of IE; IE6/7 and Opera currently ignore this header.
     $headers[] = array('X-Content-Type-Options', 'nosniff');
 
     return $headers;
diff --git a/src/aphront/response/json/AphrontJSONResponse.php b/src/aphront/response/json/AphrontJSONResponse.php
new file mode 100644
index 0000000000..456fae4eaa
--- /dev/null
+++ b/src/aphront/response/json/AphrontJSONResponse.php
@@ -0,0 +1,46 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * @group aphront
+ */
+final class AphrontJSONResponse extends AphrontResponse {
+
+  private $content;
+
+  public function setContent($content) {
+    $this->content = $content;
+    return $this;
+  }
+
+  public function buildResponseString() {
+    $response = $this->encodeJSONForHTTPResponse(
+      $this->content,
+      $use_javelin_shield = false);
+    return $response;
+  }
+
+  public function getHeaders() {
+    $headers = array(
+      array('Content-Type', 'application/json'),
+    );
+    $headers = array_merge(parent::getHeaders(), $headers);
+    return $headers;
+  }
+
+}
diff --git a/src/aphront/response/json/__init__.php b/src/aphront/response/json/__init__.php
new file mode 100644
index 0000000000..d6d6e8e97a
--- /dev/null
+++ b/src/aphront/response/json/__init__.php
@@ -0,0 +1,12 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'aphront/response/base');
+
+
+phutil_require_source('AphrontJSONResponse.php');
diff --git a/src/aphront/sink/base/AphrontHTTPSink.php b/src/aphront/sink/base/AphrontHTTPSink.php
index 417085ad56..63493bdf09 100644
--- a/src/aphront/sink/base/AphrontHTTPSink.php
+++ b/src/aphront/sink/base/AphrontHTTPSink.php
@@ -105,6 +105,23 @@ abstract class AphrontHTTPSink {
   }
 
 
+  /**
+   * Write an entire @{class:AphrontResponse} to the output.
+   *
+   * @param AphrontResponse The response object to write.
+   * @return void
+   */
+  final public function writeResponse(AphrontResponse $response) {
+    $all_headers = array_merge(
+      $response->getHeaders(),
+      $response->getCacheHeaders());
+
+    $this->writeHTTPStatus($response->getHTTPResponseCode());
+    $this->writeHeaders($all_headers);
+    $this->writeData($response->buildResponseString());
+  }
+
+
 /* -(  Emitting the Response  )---------------------------------------------- */
 
 
diff --git a/src/aphront/sink/base/__tests__/AphrontHTTPSinkTestCase.php b/src/aphront/sink/base/__tests__/AphrontHTTPSinkTestCase.php
index 7d6e7edc05..3aa09ce72b 100644
--- a/src/aphront/sink/base/__tests__/AphrontHTTPSinkTestCase.php
+++ b/src/aphront/sink/base/__tests__/AphrontHTTPSinkTestCase.php
@@ -79,4 +79,20 @@ final class AphrontHTTPSinkTestCase extends PhabricatorTestCase {
     $sink->writeHeaders(array(array($input, 'value')));
   }
 
+  public function testJSONContentSniff() {
+    $response = id(new AphrontJSONResponse())
+      ->setContent(
+        array(
+          'x' => '<iframe>',
+        ));
+    $sink = new AphrontIsolatedHTTPSink();
+    $sink->writeResponse($response);
+
+    $this->assertEqual(
+      'for(;;);{"x":"\u003ciframe\u003e"}',
+      $sink->getEmittedData(),
+      "JSONResponse should prevent content-sniffing attacks.");
+  }
+
+
 }
diff --git a/src/aphront/sink/base/__tests__/__init__.php b/src/aphront/sink/base/__tests__/__init__.php
index 515c8033f1..8f404ff42f 100644
--- a/src/aphront/sink/base/__tests__/__init__.php
+++ b/src/aphront/sink/base/__tests__/__init__.php
@@ -6,8 +6,11 @@
 
 
 
+phutil_require_module('phabricator', 'aphront/response/json');
 phutil_require_module('phabricator', 'aphront/sink/test');
 phutil_require_module('phabricator', 'infrastructure/testing/testcase');
 
+phutil_require_module('phutil', 'utils');
+
 
 phutil_require_source('AphrontHTTPSinkTestCase.php');
diff --git a/src/applications/conduit/controller/api/PhabricatorConduitAPIController.php b/src/applications/conduit/controller/api/PhabricatorConduitAPIController.php
index a471485632..e5be6eaf0a 100644
--- a/src/applications/conduit/controller/api/PhabricatorConduitAPIController.php
+++ b/src/applications/conduit/controller/api/PhabricatorConduitAPIController.php
@@ -194,9 +194,8 @@ class PhabricatorConduitAPIController
           $response->toDictionary());
       case 'json':
       default:
-        return id(new AphrontFileResponse())
-          ->setMimeType('application/json')
-          ->setContent('for(;;);'.$response->toJSON());
+        return id(new AphrontJSONResponse())
+          ->setContent($response->toDictionary());
     }
   }
 
diff --git a/src/applications/conduit/controller/api/__init__.php b/src/applications/conduit/controller/api/__init__.php
index c4bd197750..912ea00381 100644
--- a/src/applications/conduit/controller/api/__init__.php
+++ b/src/applications/conduit/controller/api/__init__.php
@@ -6,7 +6,7 @@
 
 
 
-phutil_require_module('phabricator', 'aphront/response/file');
+phutil_require_module('phabricator', 'aphront/response/json');
 phutil_require_module('phabricator', 'aphront/writeguard');
 phutil_require_module('phabricator', 'applications/conduit/controller/base');
 phutil_require_module('phabricator', 'applications/conduit/method/base');
diff --git a/src/applications/conduit/protocol/response/ConduitAPIResponse.php b/src/applications/conduit/protocol/response/ConduitAPIResponse.php
index 83b8f3bb24..f206b4cc4e 100644
--- a/src/applications/conduit/protocol/response/ConduitAPIResponse.php
+++ b/src/applications/conduit/protocol/response/ConduitAPIResponse.php
@@ -56,8 +56,4 @@ class ConduitAPIResponse {
       'error_info' => $this->getErrorInfo(),
     );
   }
-
-  public function toJSON() {
-    return json_encode($this->toDictionary());
-  }
 }
diff --git a/src/infrastructure/celerity/response/CelerityStaticResourceResponse.php b/src/infrastructure/celerity/response/CelerityStaticResourceResponse.php
index 86b707ff7b..c9416f8e5d 100644
--- a/src/infrastructure/celerity/response/CelerityStaticResourceResponse.php
+++ b/src/infrastructure/celerity/response/CelerityStaticResourceResponse.php
@@ -189,7 +189,7 @@ final class CelerityStaticResourceResponse {
     }
   }
 
-  public function renderAjaxResponse($payload, $error = null) {
+  public function buildAjaxResponse($payload, $error = null) {
     $response = array(
       'error'   => $error,
       'payload' => $payload,
@@ -205,9 +205,7 @@ final class CelerityStaticResourceResponse {
       $this->behaviors = array();
     }
 
-    $response = 'for (;;);'.json_encode($response);
     return $response;
   }
 
-
 }
diff --git a/webroot/index.php b/webroot/index.php
index 2fc1a4cec6..afcc80fa0b 100644
--- a/webroot/index.php
+++ b/webroot/index.php
@@ -147,6 +147,8 @@ try {
 
 $write_guard->dispose();
 
+// TODO: Share the $sink->writeResponse() pathway here?
+
 $sink = new AphrontPHPHTTPSink();
 $sink->writeHTTPStatus($response->getHTTPResponseCode());