Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protect request phase against CSRF when Rails is used. #809

Closed
wants to merge 1 commit into from

Conversation

@DouweM
Copy link

@DouweM DouweM commented May 25, 2015

TL;DR, as of 2020-03-07:

Please note: there is currently a CSRF vulnerability which affects OmniAuth (designated CVE-2015-9284) that requires mitigation at the application level. More details on how to do this can be found on the Wiki.

Original issue description:


The request phase in OmniAuth is currently vulnerable to Cross-Site Request Forgery, which allows an attacker to easily gain full access to a user's account on a site that uses OmniAuth, when used in combination with another CSRF vulnerability on the side of a connected OAuth provider.

This vulnerability was reported to us (GitLab) by Mohamed Abdelbaset Elnoby (@SymbianSyMoh), a Senior Information Security Analyst at Seekurity.com, on April 23rd. On April 24th, we reported it to the OmniAuth team via email. On May 14th, we merged our patch into the public GitLab repository.

Mr. Elnoby also gave us an example of an OAuth provider that had the CSRF vulnerability on their side that is needed to make this attack "weaponizable". The vulnerability has been reported to them.

The below description of the issue is written from GitLab’s perspective.

Connecting the GitLab account to an OAuth provider like Google works like this:

  1. User visits https://gitlab.com/users/auth/google
  2. User is redirected to a page on Google
  3. Google checks if the user is logged in
  4. If the user is logged in, Google checks if the user has authorized the GitLab.com application to use their Google account
  5. If the user has authorized the GitLab.com application to use their Google account, Google redirects back to GitLab
  6. GitLab adds the Google account to the GitLab account as an alternate identity, which means the Google account can be used to log into the GitLab account.

This is all very straightforward, and works as expected.

Note that if the Google user has already authorized the GitLab.com application to use their Google account, there is no user action needed to link the two accounts, apart from the initial https://gitlab.com/users/auth/google visit.

This means that if you can get a person who 1) is signed into Google, 2) has authorized the GitLab.com application to use it, and 3) is signed into GitLab.com, to visit https://gitlab.com/users/auth/google, their account will be linked to the Google account without any further interaction, and without any feedback to the user.

This is not a big deal if the signed in Google account belongs to the GitLab user themselves, but it is if the user is somehow signed into Google using an account belonging to an attacker. In this scenario, the GitLab account would be linked to the attacker's Google account, and the GitLab user would be none the wiser.

And let it just so be the case that that iss possible with Redacted: there exists a vulnerability on their side that allows an attacker to log into Redacted using CSRF, which means that if a user visits a specially crafted page, they will end up being logged into Redacted as the attacker.

Combining this with the described GitLab.com flow, forcing the victim to visit that specially crafted page, and directly after that, forcing them to visit https://gitlab.com/users/auth/redacted (by using an iframe, for example), will give the attacker full control over the victim's GitLab account. "Forcing them" can be as simple as sending them a link over email and getting them to click it.

To actually use the POST method rather than GET so the CSRF token is sent along, we have changed our links to the auth path from:

link_to "Log in with #{provider}", omniauth_authorize_path(:user, provider)

to:

link_to "Log in with #{provider}", omniauth_authorize_path(:user, provider), method: :post

Note that the current implementation is very specific to Rails. It needs to be, as far as I can see, since the CSRF protection needs to use the web framework’s authenticity token.

I suggest deprecating OmniAuth.config.allowed_request_methods and only allowing :post from now on, but I haven't made that change yet since I think it warrants more discussion.

People that use OmniAuth with Rails will need to change the link_to helpers and <form> tags in their own code to use POST instead of GET,
so I’m afraid this is a breaking change and can’t just go in a patch release, in SemVer terms.

cc @sferik

@DouweM DouweM force-pushed the csrf-protection branch from eea68f0 to 4e4f65a May 25, 2015
@DouweM DouweM force-pushed the csrf-protection branch from 4e4f65a to 561ef98 May 25, 2015
@sferik
Copy link
Contributor

@sferik sferik commented May 26, 2015

Thanks for this patch. Normally, I’d merge and release a security patch immediately but I’m reluctant to do so in this case for two reasons:

  1. this is a breaking change and
  2. it contains Rails-specific code

I’d propose that we solve this by creating a new gem: omniauth-rails, that depends on omniauth and rails and contains all Rails-specific code. We could release this as version 1.0.0 and notify users of OmniAuth that they should depend on omniauth-rails instead of omniauth if they’re using OmniAuth with Rails.

I’d like to get 👍 from the original author of OmniAuth (@mbleigh) before making this change as well as the Rails security team (@tenderlove, @jeremy, @NZKoz).

If everyone agrees this is a good idea, we can move forward with this plan very quickly.

@DouweM
Copy link
Author

@DouweM DouweM commented May 26, 2015

@sferik Moving to omniauth-rails makes sense. We'd need to get devise to move on this as well.

@sferik
Copy link
Contributor

@sferik sferik commented May 26, 2015

@DouweM That should be doable. Ping: @josevalim and @carlosantoniodasilva.

@sferik
Copy link
Contributor

@sferik sferik commented May 26, 2015

Two more questions:

  1. Has a CVE-ID already been assigned for this vulnerability?
  2. When I click on Seekurity.com, I get a GoDaddy parked page. Is that really the security firm with which @SymbianSyMoh is associated?

@DouweM
Copy link
Author

@DouweM DouweM commented May 26, 2015

  1. No, I haven't done anything on that. From our earlier interaction, I wasn't sure if the issue was as serious as I thought and if it warranted that.
  2. He says so.

@sferik
Copy link
Contributor

@sferik sferik commented May 26, 2015

  1. No, I haven't done anything on that. From our earlier interaction, I wasn't sure if the issue was as serious as I thought and if it warranted that.

@DouweM I think it would be helpful to have a CVE-ID for this vulnerability, so we can reference it unambiguously. Since you are most familiar with this issue, could you please initiate that process? The process to request a CVE-ID is described here.

@DouweM
Copy link
Author

@DouweM DouweM commented May 26, 2015

@sferik Thanks, I'll go through the motions.

@sferik
Copy link
Contributor

@sferik sferik commented May 26, 2015

@sferik Thanks, I'll go through the motions.

@DouweM Thank you.

I’ve contacted the Rails Security Team, informing them of this issue and asking for their help in spreading the word once it is patched.

@DouweM
Copy link
Author

@DouweM DouweM commented May 26, 2015

@tenderlove
Copy link

@tenderlove tenderlove commented May 26, 2015

I guess I don't understand how this is a Rails specific issue. Any framework you use with Omniauth will have this problem if you're using GET requests (though this solution is for Rails apps). This patch seems fine for handling the Rails side (though I thought we'd do everything automatically if you did a link_to with a method: post)

@rafaelfranca
Copy link

@rafaelfranca rafaelfranca commented May 26, 2015

@tenderlove I think this issue is not Rails specific so we don't need to handle it, @sferik only want our review in his plan.

The plan looks good. The Rails team can help to spread the word via our twitter account and the security list.

@tenderlove
Copy link

@tenderlove tenderlove commented May 26, 2015

@tenderlove I think this issue is not Rails specific so we don't need to handle it, @sferik only want our review in his plan.

Gothcha. It seems good to me. The only question I have is since the link is turned in to a form, wouldn't we already take care of CSRF validation? I don't know omniauth internals. I assume it's a middleware? If Omiauth was placed after we do CSRF checking, couldn't we delete most of the code in this patch?

@DouweM
Copy link
Author

@DouweM DouweM commented May 26, 2015

Gothcha. It seems good to me. The only question I have is since the link is turned in to a form, wouldn't we already take care of CSRF validation? I don't know omniauth internals. I assume it's a middleware? If Omiauth was placed after we do CSRF checking, couldn't we delete most of the code in this patch?

OmniAuth intercepts the request at the Rack middleware level, before it gets to the Rails controller and the protect_against_forgery before_action at all.

If Rails had a RequestForgeryProtection middleware, that would be useful here, but that wouldn't work for regular Rails controllers, since it depends on controller-level settings.

This patch basically turns protect_against_forgery into a middleware, albeit an OmniAuth-specific one, using the settings on ApplicationController.

@tenderlove
Copy link

@tenderlove tenderlove commented May 26, 2015

OmniAuth intercepts the request at the Rack middleware level, before it gets to the Rails controller and the protect_against_forgery before_action at all. If Rails had a RequestForgeryProtection middleware, that would be useful here, but that wouldn't work for regular Rails controllers, since it depends on controller-level settings.

Makes sense. 👍 from me on this patch. :)

@DouweM
Copy link
Author

@DouweM DouweM commented May 26, 2015

@sferik So what's the next step? If you create an intridea/omniauth-rails repo, I'm happy to build a gem out of this patch and move it there. I think it's better to have it under intridea than DouweM or gitlabhq, so people will know it's the "official" OmniAuth+Rails gem.

@sferik
Copy link
Contributor

@sferik sferik commented May 26, 2015

@tenderlove @rafaelfranca Thanks for taking the time to review this patch.

@DouweM I just created an empty repo at https://github.com/intridea/omniauth-rails. I’ll initialize that with a gem scaffold now. Feel free to open a pull request against that repo with the same code you’ve placed here. I’ll be busy for the next few hours but will try to push out a release later on this evening.

The README of this repo should also be updated to indicate that Rails users should depend on omniauth-rails instead of directly on omniauth.

I’m going to close this pull request.

@sferik sferik closed this May 26, 2015
@DouweM
Copy link
Author

@DouweM DouweM commented May 26, 2015

@sferik All right. I won't have time tonight, unfortunately, but I'll submit a pull request tomorrow.

@SymbianSyMoh
Copy link

@SymbianSyMoh SymbianSyMoh commented May 26, 2015

Hi Guys,
Sorry for being late involving this thread, so many things happening here.
@sferik Yup, that's exactly the domain but not yet stared and the website is fully under constructions.

@DouweM
Copy link
Author

@DouweM DouweM commented May 26, 2015

Hey @SymbianSyMoh, thanks for finding this vulnerability and reporting it to us!

I hope you don't mind that that I've taken up the task of reporting where relevant and making sure it gets patched. I've credited you here and in the CVE request: http://www.openwall.com/lists/oss-security/2015/05/26/11.

@SymbianSyMoh
Copy link

@SymbianSyMoh SymbianSyMoh commented May 26, 2015

Thanks @DouweM really appreciate that 👍

@SymbianSyMoh
Copy link

@SymbianSyMoh SymbianSyMoh commented May 26, 2015

@DouweM @sferik @tenderlove Should this issue be reported under this responsible disclosure rules:

  1. https://hackerone.com/ruby
  2. https://hackerone.com/rails
    Or not ?!

@rafaelfranca
Copy link

@rafaelfranca rafaelfranca commented May 26, 2015

@SymbianSyMoh for this case no, it is not related to Ruby or Rails projects. But if you find something on one of these projects, those are the recommended channels.

@SymbianSyMoh
Copy link

@SymbianSyMoh SymbianSyMoh commented May 26, 2015

Thanks @rafaelfranca Appreciate it

@DouweM
Copy link
Author

@DouweM DouweM commented May 27, 2015

Pull request agains omniauth-rails submitted: omniauth/omniauth-rails#1

@SymbianSyMoh
Copy link

@SymbianSyMoh SymbianSyMoh commented Nov 19, 2015

Hi Guys, I know this is a long time ago thread, but guess what? I never
received any CVE regarding this issue till now!! So what can i do to get it?

_Mohamed Abdelbaset Elnoby_Guru Programmer, Senior Information Security
Consultant & Web Application Penetration Tester at Seekurity Inc
http://www.Seekurity.com.

Contact me at:
LinkedIn
https://www.linkedin.com/in/symbiansymohFacebook
https://fb.com/symbiansymohTwitter https://twitter.com/symbiansymoh
https://twitter.com/symbiansymoh

On Wed, May 27, 2015 at 3:35 AM, Douwe Maan notifications@github.com
wrote:

Pull request agains omniauth-rails submitted: omniauth/omniauth-rails#1
omniauth/omniauth-rails#1


Reply to this email directly or view it on GitHub
#809 (comment).

@hiroshi
Copy link

@hiroshi hiroshi commented Mar 4, 2020

Hi all, is the gem below the recommended upgrade route for those of us using Rails?
https://rubygems.org/gems/omniauth-rails_csrf_protection

Hi, I found that README is updated recently.
https://github.com/omniauth/omniauth#integrating-omniauth-into-your-application

Please note: there is currently a CSRF vulnerability which affects OmniAuth (designated CVE-2015-9284) that requires mitigation at the application level. More details on how to do this can be found on the Wiki.

In the wiki page, omniauth-rails_csrf_protection is used as a first step for mitigating the vulnerability.

EDIT: @DouweM, update the OP with those information if you please.

BTW, depedabot may urge to update omniauth to 1.9.1, but I don't think the update don't include any fix for the vulnerability.

See v1.9.0...v1.9.1
Security related commits seem to be only this REAME change;
0740ac9

@BobbyMcWho
Copy link
Member

@BobbyMcWho BobbyMcWho commented Mar 4, 2020

@hiroshi this is correct, unfortunately a resolution for that CVE was not included in that version bump. I am not sure of any time-frame in which it will be resolved at this point in time.

@DouweM
Copy link
Author

@DouweM DouweM commented Mar 7, 2020

EDIT: @DouweM, update the OP with those information if you please.

@hiroshi Done; thanks for the ping!

@tibbon
Copy link

@tibbon tibbon commented Apr 2, 2020

What's the status on this one?

patch0 added a commit to RaspberryPiFoundation/omniauth-rpi that referenced this issue Apr 23, 2020
This reverts commit 2dcd499.

The CVE hasn't actually been fixed yet..

omniauth/omniauth#809
SMLuthi pushed a commit to CMSgov/dpc-app that referenced this issue Jun 5, 2020
We need to make sure that we're mitigating the Cross Site Scripting
Forgery vulnerability by using a POST method to initiate the Omniauth
flow. For more info:

https://github.com/omniauth/omniauth/wiki/Resolving-CVE-2015-9284
omniauth/omniauth#809
@josh-m-sharpe
Copy link

@josh-m-sharpe josh-m-sharpe commented Nov 14, 2020

At this point, is there any reason we can't:

  1. munge the learnings from omniauth-rails_csrf_protection into this omniauth
  2. add something like raise 'this version is only compatible with rails, read [this wiki] for more info and submit a PR! :)' unless defined?(Rails) somewhere
  3. release 2.0 with notes about the breaking changes and how to fix link_to
  4. wait for the non-rails patches for <insert your favorite framework> to come a rollin' on in?

This would solve the issue for (presumably) the majority of us, would be a step in the right direction since the rails solution is more or less figured out and would open the door to other frameworks to contribute an acceptable solution for them. There's already good work done above for sinatra and rack apps. They just need to be finalized.

I realize this isn't ideal, but it wouldn't outright break non-rails apps since the raise statement prevents them from upgrading and they 'work' today, so they'll 'work' tomorrow. It might even spur some benefit because it would force the non-rails community to think through what their solution is and get it in place.

We've had some time to let the dust settle, and now I fear that this lingering CVE is conditionalizing everyone to accept that "open CVEs for omniauth" are quite alright.

yaf added a commit to betagouv/rdv-solidarites.fr that referenced this issue Nov 23, 2020
yaf added a commit to betagouv/rdv-solidarites.fr that referenced this issue Nov 23, 2020
yaf added a commit to betagouv/rdv-solidarites.fr that referenced this issue Nov 23, 2020
yaf added a commit to betagouv/rdv-solidarites.fr that referenced this issue Nov 26, 2020
* fix security alert

Résolution en suivant les points https://github.com/omniauth/omniauth/wiki/Resolving-CVE-2015-9284

ajout du test proposé omniauth/omniauth#809 (comment)

* ajoute une page pour la connexion github en post
@janko
Copy link
Contributor

@janko janko commented Dec 2, 2020

I'm trying to take this vulnerability seriously, but I just can't help but think how difficult it would be for an attacker to exploit it in the way described by the OP.

  • the attacker would only be able to target victims that are using a browser session where the attacker is currently logged into a 3rd party service (e.g. Google), which drastically narrows down the victim pool
  • the victim would need to be logged into the particular application the attacker wants to hijack (e.g. GitLab.com)
  • the attacker would need to feed the victim a link to the /auth/:provider page and get them to click on it
  • when the 3rd party redirects back, and the app links attacker's 3rd party to victim's primary account, the app would probably show a message such as "Your Google account has been successfully linked to your primary account", which the victim would have to ignore and not try to unlink it or anything (remember, they didn't intend to sign into GitLab.com or even visit it in the first place, they clicked on a disguised link)

Is there an angle that I'm missing? Or is the main pain of this vulnerability the fact that it has a CVE assigned that is causing noise?

I'm currently building OmniAuth integation for Rodauth, and I thought I should go ahead and disallow GET /auth/:provider. But from reading a bit, requiring POST appears to have its own issues as well, such as being incompatible with authentication flows where something redirects to /auth/:provider (which multiple people mentioned here), and not supporting env["omniauth.params"] out of the box (#975).

@BobbyMcWho
Copy link
Member

@BobbyMcWho BobbyMcWho commented Dec 3, 2020

I want to say the fact that we don't persist query params from the callback phase isn't due to the POST method, but the fact that we set the omniauth.params to try and delete the same key from the session which I think has to do with the fact that callback urls contain sensitive information. If you really wanted to access the callback query params I believe they should always be accessible on a Rack::Request.new(@env).GET since as far as I can tell the GET and POST methods are available on a Rack::Request regardless of the fact that the HTTP_METHOD was POST. You could do this in a before_callback_phase block ~pseudo:

OmniAuth.config.before_callback_phase = ->(env) { env['rack.session']['my.params'] =  Rack::Request.new(env).GET.slice("keys", "you", "want") }

@janko
Copy link
Contributor

@janko janko commented Dec 3, 2020

@BobbyMcWho omniauth.params isn't for retrieving callback phase parameters, but request phase parameters. Supporting POST parameters should be as easy as using request.params instead of request.GET on this line.

@BobbyMcWho
Copy link
Member

@BobbyMcWho BobbyMcWho commented Dec 3, 2020

It seems that was intentional, changed in this PR due to this CVE.

@BobbyMcWho
Copy link
Member

@BobbyMcWho BobbyMcWho commented Dec 3, 2020

I'm open for comments/review on this PR: #1010

@urkle

This comment was marked as off-topic.

@BobbyMcWho
Copy link
Member

@BobbyMcWho BobbyMcWho commented Dec 3, 2020

For the use-case where I have my own controller actions that do work before redirecting to /auth, is there a way to simply "call" the request phase manually from my controller? (vs redirecting).. Then I don't even need the /auth/:provider routes at all.
...

If this is a specific question about the PR I posted, please ask it on the PR #1010 itself. If this is a general OmniAuth question, please refrain from asking in this thread since there are so many people subscribed to it.

CVE-2015-9284 questions/concerns should be posted here: #960

@omniauth omniauth locked and limited conversation to collaborators Dec 3, 2020
@BobbyMcWho BobbyMcWho added this to ToDo in v2.0 via automation Dec 10, 2020
@BobbyMcWho
Copy link
Member

@BobbyMcWho BobbyMcWho commented Dec 11, 2020

I've opened a discussion on a v2.0.0 release candidate: #1017

@BobbyMcWho
Copy link
Member

@BobbyMcWho BobbyMcWho commented Jan 11, 2021

The vulnerable by default configuration has been removed in the v2.0.0 release

@BobbyMcWho BobbyMcWho closed this Jan 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
v2.0
  
ToDo
Linked issues

Successfully merging this pull request may close these issues.

None yet