From 38c83ef846c139c2ea377c2fc402df10bcf42644 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 22 Oct 2012 10:49:06 -0700
Subject: [PATCH] Defuse a "Host:" header attack

Summary:
Django released a security update recently dealing with malicious "Host" headers:

https://www.djangoproject.com/weblog/2012/oct/17/security/

We're vulnerable to the same attack. Plug the hole.

The risk here is that an attacker does something like this:

  # Register "evil.com".
  # Point it at secure.phabricator.com in DNS.
  # Send a legitimate user a link to "secure.phabricator.com:ignored@evil.com".
  # They login and get cookies. Normally Phabricator refuses to set cookies on domains it does not recognize.
  # The attacker now points "evil.com" at his own servers and reads the auth cookies on the next request.

Test Plan: Unit tests.

Reviewers: vrana, btrahan

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D3766
---
 src/aphront/AphrontRequest.php                | 12 ++++++++----
 .../__tests__/AphrontRequestTestCase.php      | 19 ++++++++++++++++++-
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/src/aphront/AphrontRequest.php b/src/aphront/AphrontRequest.php
index 75fb1d9f28..642e2bbf0f 100644
--- a/src/aphront/AphrontRequest.php
+++ b/src/aphront/AphrontRequest.php
@@ -65,10 +65,14 @@ final class AphrontRequest {
   }
 
   final public function getHost() {
-    // The "Host" header may include a port number; if so, ignore it. We can't
-    // use PhutilURI since there's no URI scheme.
-    list($actual_host) = explode(':', $this->host, 2);
-    return $actual_host;
+    // The "Host" header may include a port number, or may be a malicious
+    // header in the form "realdomain.com:ignored@evil.com". Invoke the full
+    // parser to extract the real domain correctly. See here for coverage of
+    // a similar issue in Django:
+    //
+    //  https://www.djangoproject.com/weblog/2012/oct/17/security/
+    $uri = new PhutilURI('http://'.$this->host);
+    return $uri->getDomain();
   }
 
 
diff --git a/src/aphront/__tests__/AphrontRequestTestCase.php b/src/aphront/__tests__/AphrontRequestTestCase.php
index 2d781b3e1f..75645ee53d 100644
--- a/src/aphront/__tests__/AphrontRequestTestCase.php
+++ b/src/aphront/__tests__/AphrontRequestTestCase.php
@@ -19,7 +19,7 @@
 final class AphrontRequestTestCase extends PhabricatorTestCase {
 
   public function testRequestDataAccess() {
-    $r = new AphrontRequest('http://example.com/', '/');
+    $r = new AphrontRequest('example.com', '/');
     $r->setRequestData(
       array(
         'str_empty' => '',
@@ -78,4 +78,21 @@ final class AphrontRequestTestCase extends PhabricatorTestCase {
     $this->assertEqual(false, $r->getExists('does-not-exist'));
   }
 
+  public function testHostAttacks() {
+    static $tests = array(
+      'domain.com'                    => 'domain.com',
+      'domain.com:80'                 => 'domain.com',
+      'evil.com:evil.com@real.com'    => 'real.com',
+      'evil.com:evil.com@real.com:80' => 'real.com',
+    );
+
+    foreach ($tests as $input => $expect) {
+      $r = new AphrontRequest($input, '/');
+      $this->assertEqual(
+        $expect,
+        $r->getHost(),
+        'Host: '.$input);
+    }
+  }
+
 }