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

Authored by epriestley on Mar 10 2014, 3:17 PM.

Details

Reviewers

btrahan

Commits

Restricted Diffusion Commit
rP5854de8c1c47: 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.

Diff Detail

Repository

rP Phabricator

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

btrahan accepted this revision.Mar 10 2014, 6:55 PM

btrahan edited edge metadata.

This revision is now accepted and ready to land.Mar 10 2014, 6:55 PM

Adding to the list of potential mitigations, I do think it's worthwhile to ask installs to explicitly whitelist the full path. It's tricky to identify all situations where these params leak through referrers or other means and using a single path is a healthy precaution. This would mean that any changes to the auth URI would be a breaking change to existing installs, so you'll need to decide if the URIs are sufficiently stable at this point.

https://github.com/facebook/phabricator/blob/master/src/applications/auth/provider/PhabricatorAuthProviderOAuthFacebook.php#L21

This would entail updating the instructions to set "Site URL" to the domain + /auth/login/facebook:facebook.com/ and removing mention of "Site Domain" (a deprecated setting).

Just double checking, but there's no way we can test that they configured things correctly, right (like some sort of application metadata API)? We could intentionally perform a request with a bogus URI, I suppose, but that seems very complicated / error prone.

It's slightly convoluted, but once Phabricator has the app secret, it's able to check the configuration by obtaining an app access token.

https://developers.facebook.com/docs/graph-api/reference/app

You'd want to enforce that:

website_url is the full path to the install's Facebook OAuth endpoint
app_domains, mobile_web_url, [secure_]canvas_url, [secure_]page_tab_url are null

It's also possible to only ask for the App ID & App Secret and then update the configuration automatically, but that might warrant messaging warning of the potentially destructive action.