Bypassing GitHub's OAuth Flow with a Head Request

https://blog.teddykatz.com/2019/11/05/github-oauth-bypass.html

by fanf2 • 7 years ago

470 98 7 years ago

blog.teddykatz.com

purephase• 7 years ago

* 2019-06-19 23:28:56 UTC Issue reported to GitHub on HackerOne

* 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.

zaroth4• 7 years ago

I mean, it’s a total auth bypass in GitHub by simply sending a HEAD request, with a one line fix. The turnaround time shows that they have the ability to evaluate a serious report through HackerOne quickly, they have a dedicated engineer who could get in and make the one line fix, and they have the ability to deploy new code quickly. All good things!

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.

_jal• 7 years ago

Perhaps it does merely reduce to "being competent". But the contrast with nearly every other vendor out there is stark.

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.

Tijdreiziger• 7 years ago

It doesn't say they paid out yet, just that they 'awarded' the bounty, which could just be GitHub telling HackerOne they intend to pay without actually having moved any money yet.

spullara• 7 years ago

How long until someone is coding up a minivan?

https://dilbert.com/strip/1995-11-13

notacoward• 7 years ago

Exactly what I thought too. Just for once, a bug bounty program worked as intended. Bug was fixed in a timely fashion instead of being denied or ignored, bounty was awarded, disclosure was made, life gets better for everyone. Nice to see.

throwaway5752• 7 years ago

"Just for once, a bug bounty program worked as intended" I would just say, you don't generally hear about it when it works as intended (as it usually does).

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.

t34543• 7 years ago

I run a bounty program and I like to think my company does a good job, too. I received a report of a RCE in a desktop app and got a fix released in 72h. We have a bounty committee that reviews payouts and paid in one week.

FactolSarin• 7 years ago

It's good PR too. This really makes me appreciate GitHub that much more

diafygi4• 7 years ago

In the proof of concept[1]:

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...

homakov12• 7 years ago

Only step 1 is important, that's where the bug is. The rest is routine to get a code for your already authorized app.

zaroth4• 7 years ago

Why would you want to ask the user to authorize an app they already authorized? IMO that would be a bug and definitely unexpected behavior.

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.

diafygi4• 7 years ago

Hmmm, as a user, I guess I would expect the opposite behavior, and I'd think the question should be why wouldn't you want to ask the user to authorize and app they already authorized?

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)

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.

couchand6• 7 years ago

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?

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.

hirsin2• 7 years ago

Facebook does this when you sign into an app you've previously authorized, in native application flows. Microsoft just added this recently as well. We do recommend it to other IDPs.

This is only enforced for public clients though that don't have verifiable reply URIs - so web sites are OK.

diafygi4• 7 years ago

Hmmm, is there guidance saying that you should auto-redirect?

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?

[1] https://tools.ietf.org/html/rfc6749#section-10.5

mrgalaxy• 7 years ago

Why does Github automatically redirect back the 2nd time without asking the user to authorize again?

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.

homakov12• 7 years ago

Wow, why would the browser allow HEAD request in the first place w/o explicit confirmation from github server with CORS headers. This is very strange.

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.

lol7685• 7 years ago

Why's it strange? In a properly designed application, it shouldn't do anything any different to the no-cors requests you can already achieve with e.g. an <img> element. HEAD, GET et al are all supposed to be "safe methods" per the spec.

homakov12• 7 years ago

You can't achieve a HEAD request with <img> elements, can you?

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.

Gaelan2• 7 years ago

I think HEAD requests are how the browser gets CORS headers in the first place.

thefreeman3• 7 years ago

I believe you are thinking of the OPTIONS preflight request.

thefreeman3• 7 years ago

The author proxied the request through his own server in order to bypass CORS restrictions.

iancarroll• 7 years ago

That would defeat the point because then the server would need to know your authentication cookie. I can’t see the PoC but I doubt this is how it works.

lol7685• 7 years ago

Presumably a SameSite=Lax cookie wouldn't have helped here, because

"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?

fanf2• 7 years ago

I found a very similar bug in my code, though not in such a dangerous place! So thank you Teddy Katz and GitHub for helping me to fix a mistake in a less stressful way. This is why sharing detailed vulnerability reports is such a good thing.

zaroth4• 7 years ago

What is it about OAuth’s design that provides such an incredibly diverse number of failure modes?

I feel like it’s the most common critical vulnerability that crops up on every major site in one form or another.

lightedman• 7 years ago

"What is it about OAuth’s design that provides such an incredibly diverse number of failure modes?"

A solid lack of understanding at all layers of the OSI model, especially the first three.

SgtBastard• 7 years ago

Did you mean the top 3 (Session-Application) not first 3 (Physical-Network)? This is deliciously ironic if not.

est• 7 years ago

Ohhh, so that's how my github account was stolen.

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.

lol7685• 7 years ago

If the folks at GitHub are half-decent engineers (and everything I've seen suggests they know what they're doing), they'll have request logs that would make it very very obvious someone had done this.

Would advise sending a support request in, if you think this happened to you.

couchand6• 7 years ago

I don't believe it is possible to change your password with the GitHub API, and that action also requires reauthentication.

danso• 7 years ago

So Rails (along with some other web frameworks) implements a clever hack: it tries to route HEAD requests to the same place as it would route GET requests. Then it runs the controller code, and just omits the response body.

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

333c• 7 years ago

Your link doesn't work.

homakov12• 7 years ago

I wonder if Rails will fix the default implementation of handling HEAD requests

It absolutely should. Nobody expected Rails to covertly turn HEAD into GET. It must be explicit "get_or_head" method in routes.rb

michaelmior• 7 years ago

HEAD isn't turned into GET, it's routed the same as a GET request. If it were actually turned into a GET request, then `request.get?` would have been true, and this exploit wouldn't have happened.

But I do agree with your point that routing of HEAD requests should be explicit.

FanaHOVA• 7 years ago

Considering that GitHub is one the main Rails maintainers, probably. That might actually be how they fixed it internally and will push it upstream.

jessaustin• 7 years ago

ISTM it's reasonable not to force everyone not to write a separate HEAD handler everywhere. The main result of that would be lots of 405 errors. This was a really dumb mistake at the application level. Maybe the library needs better documentation, but it shouldn't be made less functional.

latortuga• 7 years ago

Agreed. This is a corner case because Rails encourages you to route different request verbs to different endpoints via the `resource` routing scheme. The situation played out like this because of handling GET and POST via the same action which should not have been done and in general is not something Rails encourages.

kerng• 7 years ago

It took all of 8 minutes from the bug bounty hunter reporting, to Github confirming the finding. That's amazing.

greenie_beans• 7 years ago

very cool, thanks for sharing. i'm a junior dev who recently implemented oauth in a rails app and this is instructional. thanks

oefrha• 7 years ago

This is why I hate it when people think there are N possible cases, and in a cascading if/else construct they only check for N-1 conditions and assume the remaining all fall within the Nth case. Thing is application logic in the real world is often fuzzy, it is entirely possible to overlook certain possibilities (which may or may not arise from bugs elsewhere); and even if you have considered all cases at present, future changes to other parts of the code may inadvertently introduce new possibilities.

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.

femto113• 7 years ago

In general I’d say testing the method in a controller is a smell，and in this particular case I’d suggest GET and POST should never have been handled by the same function. If GET did nothing but render the view sending a HEAD would be harmless.

diminoten• 7 years ago

Yeah the "if GET" code raised my eyebrows a bit, I know rails can handle separate GET and POST funcs.

icebraining• 7 years ago

While you're not wrong, if I explicitly tell the framework to only accept GET and POST, I consider it a bug if any other method "gets through", particularly since the docs don't say that can happen.

bobbyi_settv• 7 years ago

A common reason people don't do it the way you're suggesting is dogma about code coverage.

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.

lysium• 7 years ago

So true! Another reason against dogmas, especially concerning code coverage.

progval• 7 years ago

Some languages have good alternatives to this. Examples:

* 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.

Existenceblinks2• 7 years ago

As an average Ruby developer. The first 100ms I saw:

  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?

wolco• 7 years ago

Could you explain that further.

Why would you avoid a serious statement in an else?

Would this be more acceptable? The negative on the if

  if !request.get?

jacquesm3• 7 years ago

A cascading if/else construct can have up to 2^(N+1) possible combinations where N is the number of if-else constructs. That can become very hard to test. If you have more than two levels of nested if-else's you should probably refactor especially if it is security sensitive code. The mental model you have of your code is as complex as the code itself and there is no way that you will be able to accurately model some more complex than what you can see all at once. Scope reduction is super important if you want reliable code and function extraction is one way to achieve this. It also allows for much cleaner test setups.

nickjj2• 7 years ago

Interesting bug.

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.

griffinmb2• 7 years ago

This class of bug (CSRF bypass via route confusion) is probably more common in Phoenix apps. I’ve found a handful of apps vulnerable to this issue with Sobelow.

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.

nickjj2• 7 years ago

To be safe from this in Phoenix that would look like this right?

    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.

tptacek2• 7 years ago

What a fantastic bug. Rails routes HEAD to GET, but .get? in a HEAD handler is false. Github uses a postback to /authorize to complete OAuth, distinguishing between the original GET and the completing POST with .get?. Rails sees the HEAD request and assumes it's a POST (because it's not .get?) --- and also assumes that, because it's a POST, it's CSRF protected, which it is not, because it is not actually a POST.

eganist• 7 years ago

It's bad assumptions all the way down.

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.

jacquesm3• 7 years ago

It's bad assumptions all the way down.

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.

ljm• 7 years ago

It also looks like a Rails bug, unless Github routed the HEAD requests themselves. In that case, it’s a shortcut that bit them in the ass.

ryanisnan• 7 years ago

Rails is not assuming that it's a POST. The conditional logic in the controller explicitly checks for a get, and then else's otherwise. This is an unfortunate consequence of not having routing go to specific methods, e.g. `def post():`.

tptacek2• 7 years ago

Rails routing does go to specific methods ordinarily.

What's funny here is, apparently, .get? used to return true for HEAD requests.

londons_explore• 7 years ago

And this folks is why you should always use separate sessions for different websites, and remember to log out of sites you aren't using.

kureikain• 7 years ago

This is the reason I don't use else much and use explicitly matching.

`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.

jacquesm3• 7 years ago

Defensive code would have you write 'break if i >= 10'.

Bypassing GitHub's OAuth Flow with a Head Request

Related Stories