From 41b9752ba8a3d3d5cac14d2afc47baf3c437945b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 19 Feb 2013 16:09:36 -0800
Subject: [PATCH] Fix an OAuthServer issue where an attacker could make a link
 function over HTTP when it should be HTTPS-only

Summary:
Two behavioral changes:

  - If the redirect URI for an application is "https", require HTTPS always.
  - According to my reading of http://tools.ietf.org/html/draft-ietf-oauth-v2-23#section-3.1.2 we need to check both names //and values// for parameters. Add value checking. I think this makes more sense in general? No one uses this, soooo...

iiam

Test Plan: This has good coverage already; added some tests for the new cases.

Reviewers: vrana

Reviewed By: vrana

CC: cbg, aran, btrahan

Differential Revision: https://secure.phabricator.com/D5022
---
 .../oauthserver/PhabricatorOAuthServer.php    | 69 ++++++++++++++-----
 .../PhabricatorOAuthServerTestCase.php        | 30 ++++++++
 2 files changed, 80 insertions(+), 19 deletions(-)

diff --git a/src/applications/oauthserver/PhabricatorOAuthServer.php b/src/applications/oauthserver/PhabricatorOAuthServer.php
index 46f651fede..2180e53967 100644
--- a/src/applications/oauthserver/PhabricatorOAuthServer.php
+++ b/src/applications/oauthserver/PhabricatorOAuthServer.php
@@ -206,15 +206,19 @@ final class PhabricatorOAuthServer {
    * for details on what makes a given redirect URI "valid".
    */
   public function validateRedirectURI(PhutilURI $uri) {
-    if (PhabricatorEnv::isValidRemoteWebResource($uri)) {
-      if ($uri->getFragment()) {
-        return false;
-      }
-      if ($uri->getDomain()) {
-        return true;
-      }
+    if (!PhabricatorEnv::isValidRemoteWebResource($uri)) {
+      return false;
     }
-    return false;
+
+    if ($uri->getFragment()) {
+      return false;
+    }
+
+    if (!$uri->getDomain()) {
+      return false;
+    }
+
+    return true;
   }
 
   /**
@@ -222,18 +226,45 @@ final class PhabricatorOAuthServer {
    * its own right. Further, it must have the same domain and (at least) the
    * same query parameters as the primary URI.
    */
-  public function validateSecondaryRedirectURI(PhutilURI $secondary_uri,
-                                               PhutilURI $primary_uri) {
-    $valid = $this->validateRedirectURI($secondary_uri);
-    if ($valid) {
-      $valid_domain    = ($secondary_uri->getDomain() ==
-                          $primary_uri->getDomain());
-      $good_params     = $primary_uri->getQueryParams();
-      $test_params     = $secondary_uri->getQueryParams();
-      $missing_params  = array_diff_key($good_params, $test_params);
-      $valid           = $valid_domain && empty($missing_params);
+  public function validateSecondaryRedirectURI(
+    PhutilURI $secondary_uri,
+    PhutilURI $primary_uri) {
+
+    // The secondary URI must be valid.
+    if (!$this->validateRedirectURI($secondary_uri)) {
+      return false;
     }
-    return $valid;
+
+    // Both URIs must point at the same domain.
+    if ($secondary_uri->getDomain() != $primary_uri->getDomain()) {
+      return false;
+    }
+
+    // Any query parameters present in the first URI must be exactly present
+    // in the second URI.
+    $need_params = $primary_uri->getQueryParams();
+    $have_params = $secondary_uri->getQueryParams();
+
+    foreach ($need_params as $key => $value) {
+      if (!array_key_exists($key, $have_params)) {
+        return false;
+      }
+      if ((string)$have_params[$key] != (string)$value) {
+        return false;
+      }
+    }
+
+    // If the first URI is HTTPS, the second URI must also be HTTPS. This
+    // defuses an attack where a third party with control over the network
+    // tricks you into using HTTP to authenticate over a link which is supposed
+    // to be HTTPS only and sniffs all your token cookies.
+    if (strtolower($primary_uri->getProtocol()) == 'https') {
+      if (strtolower($secondary_uri->getProtocol()) != 'https') {
+        return false;
+      }
+    }
+
+    return true;
   }
 
 }
diff --git a/src/applications/oauthserver/__tests__/PhabricatorOAuthServerTestCase.php b/src/applications/oauthserver/__tests__/PhabricatorOAuthServerTestCase.php
index 3e8e4a8ca7..070c822737 100644
--- a/src/applications/oauthserver/__tests__/PhabricatorOAuthServerTestCase.php
+++ b/src/applications/oauthserver/__tests__/PhabricatorOAuthServerTestCase.php
@@ -61,6 +61,36 @@ final class PhabricatorOAuthServerTestCase
         "relative to '{$primary_uri}'");
     }
 
+    $primary_uri = new PhutilURI('https://secure.example.com/');
+    $tests = array(
+      'https://secure.example.com/' => true,
+      'http://secure.example.com/'  => false,
+    );
+    foreach ($tests as $input => $expected) {
+      $uri = new PhutilURI($input);
+      $this->assertEqual(
+        $expected,
+        $server->validateSecondaryRedirectURI($uri, $primary_uri),
+        "Validation (https): {$input}");
+    }
+
+    $primary_uri = new PhutilURI('http://example.com/?z=2&y=3');
+    $tests = array(
+      'http://example.com?z=2&y=3'      => true,
+      'http://example.com?y=3&z=2'      => true,
+      'http://example.com?y=3&z=2&x=1'  => true,
+      'http://example.com?y=2&z=3'      => false,
+      'http://example.com?y&x'          => false,
+      'http://example.com?z=2&x=3'      => false,
+    );
+    foreach ($tests as $input => $expected) {
+      $uri = new PhutilURI($input);
+      $this->assertEqual(
+        $expected,
+        $server->validateSecondaryRedirectURI($uri, $primary_uri),
+        "Validation (params): {$input}");
+    }
+
   }
 
 }