Fix an anchor redirect issue with OAuth server, plus modernize the application a bit

Summary:
Ref T4593. Via HackerOne. An attacker can use the anchor reattachment, combined with the Facebook token workflow, combined with redirection on OAuth errors to capture access tokens. The attack works roughly like this:

  - Create an OAuth application on Phabricator.
  - Set the domain to `evil.com`.
  - Grab the OAuth URI for it (something like `https://phabricator.com/oauthserver/auth/?redirect_uri=http://evil.com&...`).
  - Add an invalid `scope` parameter (`scope=xyz`).
  - Use //that// URI to build a Facebook OAuth URI (something like `https://facebook.com/oauth/?redirect_uri=http://phabricator.com/...&response_type=token`).
  - After the user authorizes the application on Facebook (or instantly if they've already authorized it), they're redirected to the OAuth server, which processes the request. Since this is the 'token' workflow, it has auth information in the URL anchor/fragment.
  - The OAuth server notices the `scope` error and 302's to the attacker's domain, preserving the anchor in most browsers through anchor reattachment.
  - The attacker reads the anchor in JS and can do client workflow stuff.

To fix this, I've made several general changes/modernizations:

  - Add a new application and make it beta. This is mostly cleanup, but also turns the server off for typical installs (it's not generally useful quite yet).
  - Add a "Console" page to make it easier to navigate.
  - Modernize some of the UI, since I was touching most of it anyways.

Then I've made specific security-focused changes:

  - In the web-based OAuth workflow, send back a human-readable page when errors occur. I //think// this is universally correct. Previously, humans would get a blob of JSON if they entered an invalid URI, etc. This type of response is correct for the companion endpoint ("ServerTokenController") since it's called by programs, but I believe not correct for this endpoint ("AuthController") since it's used by humans. Most of this is general cleanup (give humans human-readable errors instead of JSON blobs).
  - Never 302 off this endpoint automatically. Previously, a small set of errors (notably, bad `scope`) would cause a 302 with 'error'. This exposes us to anchor reattachment, and isn't generally helpful to anyone, since the requesting application did something wrong and even if it's prepared to handle the error, it can't really do anything better than we can.
  - The only time we'll 'error' back now from this workflow is if a user explicitly cancels the workflow. This isn't a 302, but a normal link (the cancel button), so the anchor is lost.
  - Even if the application is already approved, don't blindly 302. Instead, show the user a confirmation dialog with a 'continue' link. This is perhaps slightly less user-friendly than the straight redirect, but I think it's pretty reasonable in general, and it gives us a lot of protection against these classes of attack. This redirect is then through a link, not a 302, so the anchor is again detached.
  -

Test Plan: I attempted to hit everything I touched. See screenshots.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4593

Differential Revision: https://secure.phabricator.com/D8517

src/__phutil_library_map__.php

@@ -1100,6 +1100,7 @@ phutil_register_library_map(array(
     'PhabricatorApplicationMetaMTA' => 'applications/metamta/application/PhabricatorApplicationMetaMTA.php',
     'PhabricatorApplicationNotifications' => 'applications/notification/application/PhabricatorApplicationNotifications.php',
     'PhabricatorApplicationNuance' => 'applications/nuance/application/PhabricatorApplicationNuance.php',
+    'PhabricatorApplicationOAuthServer' => 'applications/oauthserver/application/PhabricatorApplicationOAuthServer.php',
     'PhabricatorApplicationOwners' => 'applications/owners/application/PhabricatorApplicationOwners.php',
     'PhabricatorApplicationPHIDTypeApplication' => 'applications/meta/phid/PhabricatorApplicationPHIDTypeApplication.php',
     'PhabricatorApplicationPHPAST' => 'applications/phpast/application/PhabricatorApplicationPHPAST.php',
@@ -1717,6 +1718,7 @@ phutil_register_library_map(array(
     'PhabricatorOAuthServerAuthorizationCode' => 'applications/oauthserver/storage/PhabricatorOAuthServerAuthorizationCode.php',
     'PhabricatorOAuthServerClient' => 'applications/oauthserver/storage/PhabricatorOAuthServerClient.php',
     'PhabricatorOAuthServerClientQuery' => 'applications/oauthserver/query/PhabricatorOAuthServerClientQuery.php',
+    'PhabricatorOAuthServerConsoleController' => 'applications/oauthserver/controller/PhabricatorOAuthServerConsoleController.php',
     'PhabricatorOAuthServerController' => 'applications/oauthserver/controller/PhabricatorOAuthServerController.php',
     'PhabricatorOAuthServerDAO' => 'applications/oauthserver/storage/PhabricatorOAuthServerDAO.php',
     'PhabricatorOAuthServerScope' => 'applications/oauthserver/PhabricatorOAuthServerScope.php',
@@ -3753,6 +3755,7 @@ phutil_register_library_map(array(
     'PhabricatorApplicationMetaMTA' => 'PhabricatorApplication',
     'PhabricatorApplicationNotifications' => 'PhabricatorApplication',
     'PhabricatorApplicationNuance' => 'PhabricatorApplication',
+    'PhabricatorApplicationOAuthServer' => 'PhabricatorApplication',
     'PhabricatorApplicationOwners' => 'PhabricatorApplication',
     'PhabricatorApplicationPHIDTypeApplication' => 'PhabricatorPHIDType',
     'PhabricatorApplicationPHPAST' => 'PhabricatorApplication',
@@ -4454,6 +4457,7 @@ phutil_register_library_map(array(
     'PhabricatorOAuthServerAuthorizationCode' => 'PhabricatorOAuthServerDAO',
     'PhabricatorOAuthServerClient' => 'PhabricatorOAuthServerDAO',
     'PhabricatorOAuthServerClientQuery' => 'PhabricatorOffsetPagedQuery',
+    'PhabricatorOAuthServerConsoleController' => 'PhabricatorOAuthServerController',
     'PhabricatorOAuthServerController' => 'PhabricatorController',
     'PhabricatorOAuthServerDAO' => 'PhabricatorLiskDAO',
     'PhabricatorOAuthServerTestCase' => 'PhabricatorTestCase',

src/aphront/configuration/AphrontDefaultApplicationConfiguration.php

@@ -19,26 +19,6 @@ class AphrontDefaultApplicationConfiguration
 
   public function getURIMap() {
     return $this->getResourceURIMapRules() + array(
-      '/oauthserver/' => array(
-        'auth/'          => 'PhabricatorOAuthServerAuthController',
-        'test/'          => 'PhabricatorOAuthServerTestController',
-        'token/'         => 'PhabricatorOAuthServerTokenController',
-        'clientauthorization/' => array(
-          '' => 'PhabricatorOAuthClientAuthorizationListController',
-          'delete/(?P<phid>[^/]+)/' =>
-            'PhabricatorOAuthClientAuthorizationDeleteController',
-          'edit/(?P<phid>[^/]+)/' =>
-            'PhabricatorOAuthClientAuthorizationEditController',
-        ),
-        'client/' => array(
-          ''                        => 'PhabricatorOAuthClientListController',
-          'create/'                 => 'PhabricatorOAuthClientEditController',
-          'delete/(?P<phid>[^/]+)/' => 'PhabricatorOAuthClientDeleteController',
-          'edit/(?P<phid>[^/]+)/'   => 'PhabricatorOAuthClientEditController',
-          'view/(?P<phid>[^/]+)/'   => 'PhabricatorOAuthClientViewController',
-        ),
-      ),
-
       '/~/' => array(
         '' => 'DarkConsoleController',
         'data/(?P<key>[^/]+)/' => 'DarkConsoleDataController',

src/applications/oauthserver/application/PhabricatorApplicationOAuthServer.php

@@ -0,0 +1,62 @@
+<?php
+
+final class PhabricatorApplicationOAuthServer extends PhabricatorApplication {
+
+  public function getBaseURI() {
+    return '/oauthserver/';
+  }
+
+  public function getShortDescription() {
+    return pht('OAuth Provider');
+  }
+
+  public function getIconName() {
+    return 'oauthserver';
+  }
+
+  public function getTitleGlyph() {
+    return "~";
+  }
+
+  public function getFlavorText() {
+    return pht('yerps');
+  }
+
+  public function getApplicationGroup() {
+    return self::GROUP_UTILITIES;
+  }
+
+  public function canUninstall() {
+    return false;
+  }
+
+  public function isBeta() {
+    return true;
+  }
+
+  public function getRoutes() {
+    return array(
+      '/oauthserver/' => array(
+        '' => 'PhabricatorOAuthServerConsoleController',
+        'auth/'          => 'PhabricatorOAuthServerAuthController',
+        'test/'          => 'PhabricatorOAuthServerTestController',
+        'token/'         => 'PhabricatorOAuthServerTokenController',
+        'clientauthorization/' => array(
+          '' => 'PhabricatorOAuthClientAuthorizationListController',
+          'delete/(?P<phid>[^/]+)/' =>
+            'PhabricatorOAuthClientAuthorizationDeleteController',
+          'edit/(?P<phid>[^/]+)/' =>
+            'PhabricatorOAuthClientAuthorizationEditController',
+        ),
+        'client/' => array(
+          ''                        => 'PhabricatorOAuthClientListController',
+          'create/'                 => 'PhabricatorOAuthClientEditController',
+          'delete/(?P<phid>[^/]+)/' => 'PhabricatorOAuthClientDeleteController',
+          'edit/(?P<phid>[^/]+)/'   => 'PhabricatorOAuthClientEditController',
+          'view/(?P<phid>[^/]+)/'   => 'PhabricatorOAuthClientViewController',
+        ),
+      ),
+    );
+  }
+
+}

src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php

@@ -12,139 +12,176 @@ extends PhabricatorAuthController {
 
   public function processRequest() {
     $request = $this->getRequest();
-    $current_user  = $request->getUser();
+    $viewer = $request->getUser();
+
     $server        = new PhabricatorOAuthServer();
     $client_phid   = $request->getStr('client_id');
     $scope         = $request->getStr('scope', array());
     $redirect_uri  = $request->getStr('redirect_uri');
-    $state         = $request->getStr('state');
     $response_type = $request->getStr('response_type');
-    $response      = new PhabricatorOAuthResponse();
 
     // state is an opaque value the client sent us for their own purposes
     // we just need to send it right back to them in the response!
-    if ($state) {
+    $state = $request->getStr('state');
-      $response->setState($state);
+
-    }
     if (!$client_phid) {
-      $response->setError('invalid_request');
+      return $this->buildErrorResponse(
-      $response->setErrorDescription(
+        'invalid_request',
-        'Required parameter client_id not specified.');
+        pht('Malformed Request'),
-      return $response;
+        pht(
+          'Required parameter %s was not present in the request.',
+          phutil_tag('strong', array(), 'client_id')));
     }
-    $server->setUser($current_user);
+
+    $server->setUser($viewer);
+    $is_authorized = false;
+    $authorization = null;
+    $uri = null;
+    $name = null;
 
     // one giant try / catch around all the exciting database stuff so we
     // can return a 'server_error' response if something goes wrong!
     try {
       $client = id(new PhabricatorOAuthServerClient())
         ->loadOneWhere('phid = %s', $client_phid);
+
       if (!$client) {
-        $response->setError('invalid_request');
+        return $this->buildErrorResponse(
-        $response->setErrorDescription(
+          'invalid_request',
-          'Client with id '.$client_phid.' not found.');
+          pht('Invalid Client Application'),
-        return $response;
+          pht(
+            'Request parameter %s does not specify a valid client application.',
+            phutil_tag('strong', array(), 'client_id')));
       }
+
+      $name = $client->getName();
       $server->setClient($client);
       if ($redirect_uri) {
         $client_uri   = new PhutilURI($client->getRedirectURI());
         $redirect_uri = new PhutilURI($redirect_uri);
         if (!($server->validateSecondaryRedirectURI($redirect_uri,
                                                     $client_uri))) {
-          $response->setError('invalid_request');
+          return $this->buildErrorResponse(
-          $response->setErrorDescription(
+            'invalid_request',
-            'The specified redirect URI is invalid. The redirect URI '.
+            pht('Invalid Redirect URI'),
-            'must be a fully-qualified domain with no fragments and '.
+            pht(
-            'must have the same domain and at least the same query '.
+              'Request parameter %s specifies an invalid redirect URI. '.
-            'parameters as the redirect URI the client registered.');
+              'The redirect URI must be a fully-qualified domain with no '.
-          return $response;
+              'fragments, and must have the same domain and at least '.
+              'the same query parameters as the redirect URI the client '.
+              'registered.',
+              phutil_tag('strong', array(), 'redirect_uri')));
         }
         $uri = $redirect_uri;
       } else {
         $uri = new PhutilURI($client->getRedirectURI());
       }
-      // we've now validated this request enough overall such that we
-      // can safely redirect to the client with the response
-      $response->setClientURI($uri);
 
       if (empty($response_type)) {
-        $response->setError('invalid_request');
+        return $this->buildErrorResponse(
-        $response->setErrorDescription(
+          'invalid_request',
-          'Required parameter response_type not specified.');
+          pht('Invalid Response Type'),
-        return $response;
+          pht(
+            'Required request parameter %s is missing.',
+            phutil_tag('strong', array(), 'response_type')));
       }
+
       if ($response_type != 'code') {
-        $response->setError('unsupported_response_type');
+        return $this->buildErrorResponse(
-        $response->setErrorDescription(
+          'unsupported_response_type',
-          'The authorization server does not support obtaining an '.
+          pht('Unsupported Response Type'),
-          'authorization code using the specified response_type. '.
+          pht(
-          'You must specify the response_type as "code".');
+            'Request parameter %s specifies an unsupported response type. '.
-        return $response;
+            'Valid response types are: %s.',
+            phutil_tag('strong', array(), 'response_type'),
+            implode(', ', array('code'))));
       }
+
       if ($scope) {
         if (!PhabricatorOAuthServerScope::validateScopesList($scope)) {
-          $response->setError('invalid_scope');
+          return $this->buildErrorResponse(
-          $response->setErrorDescription(
+            'invalid_scope',
-            'The requested scope is invalid, unknown, or malformed.');
+            pht('Invalid Scope'),
-          return $response;
+            pht(
+              'Request parameter %s specifies an unsupported scope.',
+              phutil_tag('strong', array(), 'scope')));
         }
         $scope = PhabricatorOAuthServerScope::scopesListToDict($scope);
       }
 
-      list($is_authorized,
+      // NOTE: We're always requiring a confirmation dialog to redirect.
-           $authorization) = $server->userHasAuthorizedClient($scope);
+      // Partly this is a general defense against redirect attacks, and
-      if ($is_authorized) {
+      // partly this shakes off anchors in the URI (which are not shaken
-        $return_auth_code = true;
+      // by 302'ing).
-        $unguarded_write  = AphrontWriteGuard::beginScopedUnguardedWrites();
+
-      } else if ($request->isFormPost()) {
+      $auth_info = $server->userHasAuthorizedClient($scope);
+      list($is_authorized, $authorization) = $auth_info;
+
+      if ($request->isFormPost()) {
+        // TODO: We should probably validate this more? It feels a little funky.
         $scope = PhabricatorOAuthServerScope::getScopesFromRequest($request);
+
         if ($authorization) {
           $authorization->setScope($scope)->save();
         } else {
           $authorization = $server->authorizeClient($scope);
         }
-        $return_auth_code = true;
+
-        $unguarded_write  = null;
+        $is_authorized = true;
-      } else {
+      }
-        $return_auth_code = false;
+    } catch (Exception $e) {
-        $unguarded_write  = null;
+      return $this->buildErrorResponse(
+        'server_error',
+        pht('Server Error'),
+        pht(
+          'The authorization server encountered an unexpected condition '.
+          'which prevented it from fulfilling the request.'));
     }
 
-      if ($return_auth_code) {
+    // When we reach this part of the controller, we can be in two states:
-        // step 1 -- generate authorization code
+    //
-        $auth_code =
+    //   1. The user has not authorized the application yet. We want to
-          $server->generateAuthorizationCode($uri);
+    //      give them an "Authorize this application?" dialog.
+    //   2. The user has authorized the application. We want to give them
+    //      a "Confirm Login" dialog.
 
-        // step 2 return it
+    if ($is_authorized) {
-        $content = array(
+
+      // The second case is simpler, so handle it first. The user either
+      // authorized the application previously, or has just authorized the
+      // application. Show them a confirm dialog with a normal link back to
+      // the application. This shakes anchors from the URI.
+
+      $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+        $auth_code = $server->generateAuthorizationCode($uri);
+      unset($unguarded);
+
+      $full_uri = $this->addQueryParams(
+        $uri,
+        array(
           'code'  => $auth_code->getCode(),
           'scope' => $authorization->getScopeString(),
-        );
+          'state' => $state,
-        $response->setContent($content);
+        ));
-        return $response;
+
-      }
+      // TODO: It would be nice to give the user more options here, like
-      unset($unguarded_write);
+      // reviewing permissions, canceling the authorization, or aborting
-    } catch (Exception $e) {
+      // the workflow.
-      // Note we could try harder to determine between a server_error
+
-      // vs temporarily_unavailable.  Good enough though.
+      $dialog = id(new AphrontDialogView())
-      $response->setError('server_error');
+        ->setUser($viewer)
-      $response->setErrorDescription(
+        ->setTitle(pht('Authenticate: %s', $name))
-        'The authorization server encountered an unexpected condition '.
+        ->appendParagraph(
-        'which prevented it from fulfilling the request. ');
+          pht(
-      return $response;
+            'This application ("%s") is authorized to use your Phabricator '.
+            'credentials. Continue to complete the authentication workflow.',
+            phutil_tag('strong', array(), $name)))
+        ->addCancelButton((string)$full_uri, pht('Continue to Application'));
+
+      return id(new AphrontDialogResponse())->setDialog($dialog);
     }
 
-    // display time -- make a nice form for the user to grant the client
+    // Here, we're confirming authorization for the application.
-    // access to the granularity specified by $scope
-    $name = $client->getName();
-    $title = pht('Authorize %s?', $name);
-    $panel = new AphrontPanelView();
-    $panel->setWidth(AphrontPanelView::WIDTH_FORM);
-    $panel->setHeader($title);
-
-    $description =
-      "Do want to authorize {$name} to access your ".
-      "Phabricator account data?";
 
     if ($scope) {
       if ($authorization) {
@@ -154,10 +191,10 @@ extends PhabricatorAuthController {
         $desired_scopes = $scope;
       }
       if (!PhabricatorOAuthServerScope::validateScopesDict($desired_scopes)) {
-        $response->setError('invalid_scope');
+        return $this->buildErrorResponse(
-        $response->setErrorDescription(
+          'invalid_scope',
-          'The requested scope is invalid, unknown, or malformed.');
+          pht('Invalid Scope'),
-        return $response;
+          pht('The requested scope is invalid, unknown, or malformed.'));
       }
     } else {
       $desired_scopes = array(
@@ -166,31 +203,67 @@ extends PhabricatorAuthController {
       );
     }
 
-    $cancel_uri = clone $uri;
-    $cancel_params = array(
-      'error' => 'access_denied',
-      'error_description' =>
-        'The resource owner (aka the user) denied the request.'
-    );
-    $cancel_uri->setQueryParams($cancel_params);
-
     $form = id(new AphrontFormView())
-      ->setUser($current_user)
+      ->addHiddenInput('client_id', $client_phid)
+      ->addHiddenInput('redirect_uri', $redirect_uri)
+      ->addHiddenInput('response_type', $response_type)
+      ->addHiddenInput('state', $state)
+      ->addHiddenInput('scope', $request->getStr('scope'))
+      ->setUser($viewer)
       ->appendChild(
-        id(new AphrontFormStaticControl())
+        PhabricatorOAuthServerScope::getCheckboxControl($desired_scopes));
-        ->setValue($description))
-      ->appendChild(
-        PhabricatorOAuthServerScope::getCheckboxControl($desired_scopes))
-      ->appendChild(
-        id(new AphrontFormSubmitControl())
-        ->setValue('Authorize')
-        ->addCancelButton($cancel_uri));
 
-    $panel->appendChild($form);
+    $cancel_msg = pht('The user declined to authorize this application.');
+    $cancel_uri = $this->addQueryParams(
+      $uri,
+      array(
+        'error' => 'access_denied',
+        'error_description' => $cancel_msg,
+      ));
 
-    return $this->buildStandardPageResponse(
+    $dialog = id(new AphrontDialogView())
-            $panel,
+      ->setUser($viewer)
-      array('title' => $title));
+      ->setTitle(pht('Authorize "%s"?', $name))
+      ->setSubmitURI($request->getRequestURI()->getPath())
+      ->setWidth(AphrontDialogView::WIDTH_FORM)
+      ->appendParagraph(
+        pht(
+          'Do you want to authorize the external application "%s" to '.
+          'access your Phabricator account data?',
+          phutil_tag('strong', array(), $name)))
+      ->appendChild($form->buildLayoutView())
+      ->addSubmitButton(pht('Authorize Access'))
+      ->addCancelButton((string)$cancel_uri, pht('Do Not Authorize'));
+
+    return id(new AphrontDialogResponse())->setDialog($dialog);
+  }
+
+
+  private function buildErrorResponse($code, $title, $message) {
+    $viewer = $this->getRequest()->getUser();
+
+    $dialog = id(new AphrontDialogView())
+      ->setUser($viewer)
+      ->setTitle(pht('OAuth: %s', $title))
+      ->appendParagraph($message)
+      ->appendParagraph(
+        pht('OAuth Error Code: %s', phutil_tag('tt', array(), $code)))
+      ->addCancelButton('/', pht('Alas!'));
+
+    return id(new AphrontDialogResponse())->setDialog($dialog);
+  }
+
+
+  private function addQueryParams(PhutilURI $uri, array $params) {
+    $full_uri = clone $uri;
+
+    foreach ($params as $key => $value) {
+      if (strlen($value)) {
+        $full_uri->setQueryParam($key, $value);
+      }
+    }
+
+    return $full_uri;
   }
 
 }

src/applications/oauthserver/controller/PhabricatorOAuthServerConsoleController.php

@@ -0,0 +1,43 @@
+<?php
+
+final class PhabricatorOAuthServerConsoleController
+  extends PhabricatorOAuthServerController {
+
+  public function processRequest() {
+    $request = $this->getRequest();
+    $viewer = $request->getUser();
+
+    $menu = id(new PHUIObjectItemListView())
+      ->setUser($viewer);
+
+    $menu->addItem(
+      id(new PHUIObjectItemView())
+        ->setHeader(pht('Authorizations'))
+        ->setHref($this->getApplicationURI('clientauthorization/'))
+        ->addAttribute(
+          pht(
+            'Review your authorizations.')));
+
+    $menu->addItem(
+      id(new PHUIObjectItemView())
+        ->setHeader(pht('Applications'))
+        ->setHref($this->getApplicationURI('client/'))
+        ->addAttribute(
+          pht(
+            'Create a new OAuth application.')));
+
+    $crumbs = $this->buildApplicationCrumbs();
+    $crumbs->addTextCrumb(pht('Console'));
+
+    return $this->buildApplicationPage(
+      array(
+        $crumbs,
+        $menu,
+      ),
+      array(
+        'title'  => pht('OAuth Server Console'),
+        'device' => true,
+      ));
+  }
+
+}