From 7176240717f2b0170ed0e8411ea15b686dc6e52b Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 12 Mar 2014 11:30:04 -0700 Subject: [PATCH] Whitelist controllers which can receive a 'code' parameter Summary: Ref T4593. There are a variety of clever attacks against OAuth which involve changing the redirect URI to some other URI on the same domain which exhibits unexpected behavior in response to an OAuth request. The best approach to dealing with this is for providers to lock to a specific path and refuse to redirect elsewhere, but not all providers do this. We haven't had any specific issues related to this, but the anchor issue in T4593 was only a step away. To mitigate this in general, we can reject the OAuth2 `'code'` parameter on //every// page by default, and then whitelist it on the tiny number of controllers which should be able to receive it. This is very coarse, kind of overkill, and has some fallout (we can't use `'code'` as a normal parameter in the application), but I think it's relatively well-contained and seems reasonable. A better approach might be to whitelist parameters on every controller (i.e., have each controller specify the parameters it can receive), but that would be a ton of work and probably cause a lot of false positives for a long time. Since we don't use `'code'` normally anywhere (as far as I can tell), the coarseness of this approach seems reasonable. Test Plan: - Logged in with OAuth. - Hit any other page with `?code=...` in the URL, got an exception. - Grepped for `'code'` and `"code"`, and examined each use to see if it was impacted. Reviewers: btrahan Reviewed By: btrahan Subscribers: aran, epriestley Maniphest Tasks: T4593 Differential Revision: https://secure.phabricator.com/D8499 --- .../PhabricatorAuthLoginController.php | 9 +++++++ ...bricatorAuthOldOAuthRedirectController.php | 7 +++++ .../base/controller/PhabricatorController.php | 26 ++++++++++++++++++- .../PhabricatorOAuthServerTokenController.php | 7 +++++ 4 files changed, 48 insertions(+), 1 deletion(-) diff --git a/src/applications/auth/controller/PhabricatorAuthLoginController.php b/src/applications/auth/controller/PhabricatorAuthLoginController.php index fd780cb1d0..af0708b57a 100644 --- a/src/applications/auth/controller/PhabricatorAuthLoginController.php +++ b/src/applications/auth/controller/PhabricatorAuthLoginController.php @@ -11,6 +11,15 @@ final class PhabricatorAuthLoginController return false; } + public function shouldAllowRestrictedParameter($parameter_name) { + // Whitelist the OAuth 'code' parameter. + + if ($parameter_name == 'code') { + return true; + } + return parent::shouldAllowRestrictedParameter($parameter_name); + } + public function willProcessRequest(array $data) { $this->providerKey = $data['pkey']; $this->extraURIData = idx($data, 'extra'); diff --git a/src/applications/auth/controller/PhabricatorAuthOldOAuthRedirectController.php b/src/applications/auth/controller/PhabricatorAuthOldOAuthRedirectController.php index 84de83f7d2..cc7f362583 100644 --- a/src/applications/auth/controller/PhabricatorAuthOldOAuthRedirectController.php +++ b/src/applications/auth/controller/PhabricatorAuthOldOAuthRedirectController.php @@ -9,6 +9,13 @@ final class PhabricatorAuthOldOAuthRedirectController return false; } + public function shouldAllowRestrictedParameter($parameter_name) { + if ($parameter_name == 'code') { + return true; + } + return parent::shouldAllowRestrictedParameter($parameter_name); + } + public function willProcessRequest(array $data) { $this->provider = $data['provider']; } diff --git a/src/applications/base/controller/PhabricatorController.php b/src/applications/base/controller/PhabricatorController.php index eb31ff9a18..e1d3db8668 100644 --- a/src/applications/base/controller/PhabricatorController.php +++ b/src/applications/base/controller/PhabricatorController.php @@ -24,9 +24,13 @@ abstract class PhabricatorController extends AphrontController { return PhabricatorUserEmail::isEmailVerificationRequired(); } - public function willBeginExecution() { + public function shouldAllowRestrictedParameter($parameter_name) { + return false; + } + public function willBeginExecution() { $request = $this->getRequest(); + if ($request->getUser()) { // NOTE: Unit tests can set a user explicitly. Normal requests are not // permitted to do this. @@ -85,6 +89,26 @@ abstract class PhabricatorController extends AphrontController { } } + // NOTE: We want to set up the user first so we can render a real page + // here, but fire this before any real logic. + $restricted = array( + 'code', + ); + foreach ($restricted as $parameter) { + if ($request->getExists($parameter)) { + if (!$this->shouldAllowRestrictedParameter($parameter)) { + throw new Exception( + pht( + 'Request includes restricted parameter "%s", but this '. + 'controller ("%s") does not whitelist it. Refusing to '. + 'serve this request because it might be part of a redirection '. + 'attack.', + $parameter, + get_class($this))); + } + } + } + if ($this->shouldRequireEnabledUser()) { if ($user->isLoggedIn() && !$user->getIsApproved()) { $controller = new PhabricatorAuthNeedsApprovalController($request); diff --git a/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php b/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php index 5dc6238815..2b538f03e4 100644 --- a/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php +++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php @@ -10,6 +10,13 @@ extends PhabricatorAuthController { return false; } + public function shouldAllowRestrictedParameter($parameter_name) { + if ($parameter_name == 'code') { + return true; + } + return parent::shouldAllowRestrictedParameter($parameter_name); + } + public function processRequest() { $request = $this->getRequest(); $grant_type = $request->getStr('grant_type'); -- 2.51.2