Don't 302 to an external URI, even after CSRF POST
5854de8c1c47
Actions

Description

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