From 5854de8c1c475913765cb0d6c836a0f692a146cc Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 10 Mar 2014 16:21:07 -0700 Subject: [PATCH] Don't 302 to an external URI, even after CSRF POST Summary: Via HackerOne. This defuses an attack which allows users to steal OAuth tokens through a clever sequence of steps: - The attacker begins the OAuth workflow and copies the Facebook URL. - The attacker mutates the URL to use the JS/anchor workflow, and to redirect to `/phame/live/X/` instead of `/login/facebook:facebook.com/`, where `X` is the ID of some blog they control. Facebook isn't strict about paths, so this is allowed. - The blog has an external domain set (`blog.evil.com`), and the attacker controls that domain. - The user gets stopped on the "live" controller with credentials in the page anchor (`#access_token=...`) and a message ("This blog has moved...") in a dialog. They click "Continue", which POSTs a CSRF token. - When a user POSTs a `
` with no `action` attribute, the browser retains the page anchor. So visiting `/phame/live/8/#anchor` and clicking the "Continue" button POSTs you to a page with `#anchor` intact. - Some browsers (including Firefox and Chrome) retain the anchor after a 302 redirect. - The OAuth credentials are thus preserved when the user reaches `blog.evil.com`, and the attacker's site can read them. This 302'ing after CSRF post is unusual in Phabricator and unique to Phame. It's not necessary -- instead, just use normal links, which drop anchors. I'm going to pursue further steps to mitigate this class of attack more thoroughly: - Ideally, we should render forms with an explicit `action` attribute, but this might be a lot of work. I might render them with `#` if no action is provided. We never expect anchors to survive POST, and it's surprising to me that they do. - I'm going to blacklist OAuth parameters (like `access_token`) from appearing in GET on all pages except whitelisted pages (login pages). Although it's not important here, I think these could be captured from referrers in some cases. See also T4342. Test Plan: Browsed all the affected Phame interfaces. Reviewers: btrahan Reviewed By: btrahan CC: aran, arice Differential Revision: https://secure.phabricator.com/D8481 --- .../blog/PhameBlogLiveController.php | 51 ++++++++++--------- .../blog/PhameBlogViewController.php | 5 +- .../post/PhamePostViewController.php | 6 +-- src/applications/phame/storage/PhameBlog.php | 15 ++++++ 4 files changed, 45 insertions(+), 32 deletions(-) diff --git a/src/applications/phame/controller/blog/PhameBlogLiveController.php b/src/applications/phame/controller/blog/PhameBlogLiveController.php index 9b3e1fec80..3fe3073c99 100644 --- a/src/applications/phame/controller/blog/PhameBlogLiveController.php +++ b/src/applications/phame/controller/blog/PhameBlogLiveController.php @@ -30,39 +30,42 @@ final class PhameBlogLiveController extends PhameController { } if ($blog->getDomain() && ($request->getHost() != $blog->getDomain())) { - $base_uri = 'http://'.$blog->getDomain().'/'; - if ($request->isFormPost()) { - return id(new AphrontRedirectResponse()) - ->setURI($base_uri.$this->more); - } else { - // If we don't have CSRF, return a dialog instead of automatically - // redirecting, to prevent this endpoint from serving semi-open - // redirects. - $dialog = id(new AphrontDialogView()) - ->setTitle(pht('Blog Moved')) - ->setUser($user) - ->appendChild( - pht('This blog is now hosted at %s.', - $base_uri)) - ->addSubmitButton(pht('Continue')); - return id(new AphrontDialogResponse())->setDialog($dialog); - } + $base_uri = $blog->getLiveURI(); + + // Don't redirect directly, since the domain is user-controlled and there + // are a bevy of security issues associated with automatic redirects to + // external domains. + + // Previously we CSRF'd this and someone found a way to pass OAuth + // information through it using anchors. Just make users click a normal + // link so that this is no more dangerous than any other external link + // on the site. + + $dialog = id(new AphrontDialogView()) + ->setTitle(pht('Blog Moved')) + ->setUser($user) + ->appendParagraph(pht('This blog is now hosted here:')) + ->appendParagraph( + phutil_tag( + 'a', + array( + 'href' => $base_uri, + ), + $base_uri)) + ->addCancelButton('/'); + + return id(new AphrontDialogResponse())->setDialog($dialog); } $phame_request = clone $request; $phame_request->setPath('/'.ltrim($this->more, '/')); - if ($blog->getDomain()) { - $uri = new PhutilURI('http://'.$blog->getDomain().'/'); - } else { - $uri = '/phame/live/'.$blog->getID().'/'; - $uri = PhabricatorEnv::getURI($uri); - } + $uri = $blog->getLiveURI(); $skin = $blog->getSkinRenderer($phame_request); $skin ->setBlog($blog) - ->setBaseURI((string)$uri); + ->setBaseURI($uri); $skin->willProcessRequest(array()); return $skin->processRequest(); diff --git a/src/applications/phame/controller/blog/PhameBlogViewController.php b/src/applications/phame/controller/blog/PhameBlogViewController.php index 5bd9fabbbd..9d975055fc 100644 --- a/src/applications/phame/controller/blog/PhameBlogViewController.php +++ b/src/applications/phame/controller/blog/PhameBlogViewController.php @@ -158,8 +158,6 @@ final class PhameBlogViewController extends PhameController { $blog, PhabricatorPolicyCapability::CAN_JOIN); - $must_use_form = $blog->getDomain(); - $actions->addAction( id(new PhabricatorActionView()) ->setIcon('new') @@ -172,8 +170,7 @@ final class PhameBlogViewController extends PhameController { id(new PhabricatorActionView()) ->setUser($user) ->setIcon('world') - ->setHref($this->getApplicationURI('live/'.$blog->getID().'/')) - ->setRenderAsForm($must_use_form) + ->setHref($blog->getLiveURI()) ->setName(pht('View Live'))); $actions->addAction( diff --git a/src/applications/phame/controller/post/PhamePostViewController.php b/src/applications/phame/controller/post/PhamePostViewController.php index bac0a370d3..000942d96a 100644 --- a/src/applications/phame/controller/post/PhamePostViewController.php +++ b/src/applications/phame/controller/post/PhamePostViewController.php @@ -141,14 +141,13 @@ final class PhamePostViewController extends PhameController { $blog = $post->getBlog(); $can_view_live = $blog && !$post->isDraft(); - $must_use_form = $blog && $blog->getDomain(); if ($can_view_live) { - $live_uri = 'live/'.$blog->getID().'/post/'.$post->getPhameTitle(); + $live_uri = $blog->getLiveURI($post); } else { $live_uri = 'post/notlive/'.$post->getID().'/'; + $live_uri = $this->getApplicationURI($live_uri); } - $live_uri = $this->getApplicationURI($live_uri); $actions->addAction( id(new PhabricatorActionView()) @@ -156,7 +155,6 @@ final class PhamePostViewController extends PhameController { ->setIcon('world') ->setHref($live_uri) ->setName(pht('View Live')) - ->setRenderAsForm($must_use_form) ->setDisabled(!$can_view_live) ->setWorkflow(!$can_view_live)); diff --git a/src/applications/phame/storage/PhameBlog.php b/src/applications/phame/storage/PhameBlog.php index 2a5ce6832c..39f3f14910 100644 --- a/src/applications/phame/storage/PhameBlog.php +++ b/src/applications/phame/storage/PhameBlog.php @@ -136,6 +136,21 @@ final class PhameBlog extends PhameDAO return self::$requestBlog; } + public function getLiveURI(PhamePost $post = null) { + if ($this->getDomain()) { + $base = new PhutilURI('http://'.$this->getDomain().'/'); + } else { + $base = '/phame/live/'.$this->getID().'/'; + $base = PhabricatorEnv::getURI($base); + } + + if ($post) { + $base .= '/post/'.$post->getPhameTitle(); + } + + return $base; + } + /* -( PhabricatorPolicyInterface Implementation )-------------------------- */