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

src/applications/oauthserver/PhabricatorOAuthServer.php

@@ -206,34 +206,65 @@ final class PhabricatorOAuthServer {
    * for details on what makes a given redirect URI "valid".
    */
   public function validateRedirectURI(PhutilURI $uri) {
-    if (PhabricatorEnv::isValidRemoteWebResource($uri)) {
+    if (!PhabricatorEnv::isValidRemoteWebResource($uri)) {
+      return false;
+    }
+
     if ($uri->getFragment()) {
       return false;
     }
-      if ($uri->getDomain()) {
+
-        return true;
+    if (!$uri->getDomain()) {
-      }
-    }
       return false;
     }
 
+    return true;
+  }
+
   /**
    * If there's a URI specified in an OAuth request, it must be validated in
    * 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,
+  public function validateSecondaryRedirectURI(
+    PhutilURI $secondary_uri,
     PhutilURI $primary_uri) {
-    $valid = $this->validateRedirectURI($secondary_uri);
+
-    if ($valid) {
+    // The secondary URI must be valid.
-      $valid_domain    = ($secondary_uri->getDomain() ==
+    if (!$this->validateRedirectURI($secondary_uri)) {
-                          $primary_uri->getDomain());
+      return false;
-      $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);
     }
-    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;
   }
 
 }

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}");
+    }
+
   }
 
 }