Page MenuHomePhabricator

Whitelist controllers which can receive a 'code' parameter
ClosedPublic

Authored by epriestley on Mar 12 2014, 3:20 PM.
Tags
None
Referenced Files
F14058275: D8499.diff
Sun, Nov 17, 10:38 AM
F13993032: D8499.id20166.diff
Tue, Oct 22, 8:14 PM
F13985124: D8499.diff
Sun, Oct 20, 6:07 PM
F13963064: D8499.id20175.diff
Oct 15 2024, 1:48 PM
F13962221: D8499.id.diff
Oct 15 2024, 8:52 AM
Unknown Object (File)
Sep 18 2024, 12:42 PM
Unknown Object (File)
Aug 25 2024, 8:41 PM
Unknown Object (File)
Aug 25 2024, 8:41 PM
Subscribers

Details

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.

Diff Detail

Repository
rP Phabricator
Branch
oauth1
Lint
Lint Passed
Unit
Tests Passed

Event Timeline

epriestley retitled this revision from to Whitelist controllers which can receive a 'code' parameter.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.
  • The old OAuth redirect controller should also be on the whitelist.
btrahan edited edge metadata.
This revision is now accepted and ready to land.Mar 12 2014, 4:17 PM
epriestley updated this revision to Diff 20175.

Closed by commit rP7176240717f2 (authored by @epriestley).