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

First attempt at fixing CVE-2015-9284 #2

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

tmilewski
Copy link
Member

No description provided.

@tmilewski tmilewski self-assigned this May 30, 2019
@tmilewski tmilewski changed the title First go at fixing CVE-2015-9284 First attempt at fixing CVE-2015-9284 May 30, 2019
@jcope2013
Copy link

jcope2013 commented May 30, 2019

I noticed this didn't seem to be taking into effect the initializer when trying to install it in my rails app (rails 4.2.11.1)

it should add OmniAuth.config.allowed_request_methods = [:post] but my app was still allowing me to do get requests, adding that line to my initializer in my app directly seemed to work to verify it wasn't getting included correctly

I have an initializer in my app already that uses Rails.application.config.middleware.use OmniAuth::Builder, not sure if that has something to do with it

line I used to install it

gem 'omniauth-rails', :git => "https://github.com/omniauth/omniauth-rails.git", :branch => "CVE-2015-9284" 

@gshutler
Copy link

gshutler commented May 31, 2019

There's a typo in the file path of lib/omiauth-rails.rb (missing the n in omniauth), until that's resolved you can add:

gem 'omniauth-rails', git: "https://github.com/omniauth/omniauth-rails.git", branch: "CVE-2015-9284", require: "omiauth-rails"

Still doing more testing but that registers the Railtie as expected.

@akhil-gautam
Copy link

akhil-gautam commented May 31, 2019

@gshutler Is this gem to be used alongside OmniAuth?

@svoop
Copy link

svoop commented May 31, 2019

@gshutler Shouldn't the typo-workaround rather be require: "omiauth-rails"?

@akhil-gautam omniauth-rails pulls omniauth as a dependency and contains the Rails-specific bits only. Therefore, it should be sufficient to replace the omniauth-Line with omniauth-rails in your Gemfile.

@gshutler
Copy link

@svoop Yes 🤦‍♂ - have updated my comment, thanks.

@jcope2013
Copy link

jcope2013 commented May 31, 2019

@gshutler thanks that includes the railtie but now I get this error when trying to click on one of my login links

NameError (uninitialized constant OmniAuth::Rails::RequestForgeryProtection):

@sbleon
Copy link

sbleon commented May 31, 2019

I worked around the NameError by adding the following to /config/initializers/omniauth.rb:

require 'omniauth-rails/request_forgery_protection'

lib/omiauth-rails.rb Outdated Show resolved Hide resolved
@dankozlowski
Copy link

Thank you so much for putting this together! Any way I can help push this forward?

@tmilewski
Copy link
Member Author

@dankozlowski Thanks! Just been super busy. Should have something out soon. Honestly, the best thing would be any discourse on the original thread. Any suggestions for some plans that I'll post soon are highly encouraged. Trying to limit damage to developers.

@tmilewski
Copy link
Member Author

@jcope2013 I fixed the naming issue. What provider gem are you using? Is there a chance that it's being updated later down the chain?

@jcope2013
Copy link

@jcope2013 I fixed the naming issue. What provider gem are you using? Is there a chance that it's being updated later down the chain?

thanks, I am using https://github.com/zquestz/omniauth-google-oauth2 and have an initializer where I configure it

@tmilewski
Copy link
Member Author

@jcope2013 Hmm, out of curiosity, were you able to try it since the naming changes? If not, would you mind doing so? Thanks a lot for your help!

@jcope2013
Copy link

@tmilewski thanks, yeah was still getting the error after the naming change

@sbleon
Copy link

sbleon commented Jun 25, 2019

@tmilewski your branch is working well for me. Thanks!

@nickgrim is there any way we can get this merged and released? It would have to be a major version bump, and I know that can take some effort.

@sbleon
Copy link

sbleon commented Sep 4, 2019

@nickgrim there's a lot of interest in getting this PR merged and released. Is there anything I can do to help with that?

@nickgrim
Copy link

nickgrim commented Sep 9, 2019

@sbleon: I'm not a maintainer; that was just a drive-by code-review. ;)

@sbleon
Copy link

sbleon commented Sep 9, 2019

Oh, my bad! Thanks for filling me in, @nickgrim .

Now that I'm actually trying to understand what's going on, I'm SUPER confused.

  1. This repo seems to have no actual code it in. The /lib dir contains only version.rb.
  2. The rubygems page tells me that the official omniauth-rails gem comes this other repo: https://github.com/danrabinowitz/omniauth-rails, which looks like a completely different project.
  3. So, it seems like this PR wasn't actually submitted to the official omniauth-rails repo.
  4. My Rails app wasn't even using omniauth-rails before I learned about the vulnerability in May 2019. Therefore, I think I don't need that gem; I just need the vulnerability protection that @tmilewski's work grants.

I'm coming to the conclusion that we need to publish a new gem, called something like omniauth-rails_request_protection, and that we need to make it very clear that it's unrelated to omniauth-rails. Also, this repo should be deleted, because it's presence is very confusing!

Does that all make sense, or am I crazy over here?

@fschwahn
Copy link

fschwahn commented Sep 9, 2019

There is already https://rubygems.org/gems/omniauth-rails_csrf_protection which was released by sikachu (who is on the rails core team, so pretty credible), and I've been using this since June. This repo seems a bit redundant?

@CHTJonas
Copy link

CHTJonas commented Sep 9, 2019

I'm SUPER confused

There's some instructions here that might be of help!
https://github.com/omniauth/omniauth/wiki/Resolving-CVE-2015-9284

@sbleon
Copy link

sbleon commented Sep 9, 2019

Awesome! Thanks, @fschwahn and @CHTJonas ! I've switched over to the omniauth-rails_csrf_protection gem.

I still think this repo should be deleted :-)

@alexventuraio
Copy link

Hey guys, I have a doubt maybe here somebody can suggest something.
Actually I'm using this gem omniauth-github as strategy for OmniAuth. But I got the CVE-2015-9284 warning. So it seems like the omniauth-github is using omniauth gem v1.5 as shown in the .gemspec here. And the fix shown in this PR is actually for the omniauth-rails gem not for omniauth.

So, I'm not sure what could I do.

  • Does the omniauth-github gem should be using omniauth-rails gem instead of omniauth?
  • In case of yes, is it enough to fork the omniauth-github repo and replace omniauth gem and then just use it on my Gemfile?

I hope I could explain myself. And if you have any suggestion, please share it!
Thanks in advance and awesome work in this PR. 👏🏻

dgmstuart added a commit to dgmstuart/swing-out-london that referenced this pull request Nov 17, 2019
It seems like omniauth-rails is not actually the correct gem:

  omniauth/omniauth-rails#2 (comment)

Here's the relevant instructions:

  https://github.com/omniauth/omniauth/wiki/Resolving-CVE-2015-9284
dgmstuart added a commit to dgmstuart/swing-out-london that referenced this pull request Nov 17, 2019
It seems like omniauth-rails is not actually the correct gem:

  omniauth/omniauth-rails#2 (comment)

Here's the relevant instructions:

  https://github.com/omniauth/omniauth/wiki/Resolving-CVE-2015-9284
dgmstuart added a commit to dgmstuart/swing-out-london that referenced this pull request May 11, 2021
It seems like omniauth-rails is not actually the correct gem:

  omniauth/omniauth-rails#2 (comment)

Here's the relevant instructions:

  https://github.com/omniauth/omniauth/wiki/Resolving-CVE-2015-9284
dgmstuart added a commit to dgmstuart/swing-out-london that referenced this pull request May 11, 2021
It seems like omniauth-rails is not actually the correct gem:

  omniauth/omniauth-rails#2 (comment)

Here's the relevant instructions:

  https://github.com/omniauth/omniauth/wiki/Resolving-CVE-2015-9284
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.