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 `<form />` 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
This commit is contained in:
epriestley
2014-03-10 16:21:07 -07:00
parent 0a779b60a2
commit 5854de8c1c
4 changed files with 45 additions and 32 deletions

View File

@@ -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));