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

Github security vulnerability alert CVE-2015-9284 #960

Closed
bgwilson87 opened this issue May 29, 2019 · 30 comments
Closed

Github security vulnerability alert CVE-2015-9284 #960

bgwilson87 opened this issue May 29, 2019 · 30 comments
Projects

Comments

@bgwilson87
Copy link

bgwilson87 commented May 29, 2019

I received a security vulnerability alert from GitHub for omniauth, I am using 1.9.0

"The request phase of the OmniAuth Ruby gem is vulnerable to Cross-Site Request Forgery when used as part of the Ruby on Rails framework, allowing accounts to be connected without user intent, user interaction, or feedback to the user. This permits a secondary account to be able to sign into the web application as the primary account."

https://nvd.nist.gov/vuln/detail/CVE-2015-9284

Screen Shot 2019-05-29 at 4 27 11 PM

@bgwilson87
Copy link
Author

Looks like some related discussion here #809

@gencer
Copy link

gencer commented May 29, 2019

If this is considered as too risky and gets created in CVE why it isn't fixed since 2015? I hope this will be get fixed (or PR merged). Otherwise, alternatives...? Saying, What is our alternatives, by the way?

@cjheath
Copy link

cjheath commented May 29, 2019

Github is flagging "vulnerable dependency" in any repo that has omniauth in its Gemfile.lock. That's a lot of maintainers getting hassled over this, so it's important that someone expedites a fix.

@arye-eidelman
Copy link

arye-eidelman commented May 30, 2019

Is this still an issue if I only allow linking OmniAuth accounts that have the same email address as the logged in user?

For example:

if @current_user == User.find_by(email: omniauth_info['email'])
  @current_user.link_omniauth_account(omniauth_info)

@tmilewski
Copy link
Member

Let's please continue the conversation over at #809 so that we don't wind up with diverging discourses.

Thanks!

lunohodov added a commit to lunohodov/eventical that referenced this issue May 30, 2019
Github issued a security vulnerability alert related to OmniAuth.

A Cross-Site Request Forgery vulnerability in the request phase in
OmniAuth, allows an attacker to gain full access to a user's account
on a site that uses OmniAuth.

Not sure how this affects EVE Online's SSO but the fix is
straight-forward so let's stay on the safe side.

This commit adds CSRF verification during OmniAuth's request phase.

See https://nvd.nist.gov/vuln/detail/CVE-2015-9284
See omniauth/omniauth#960

Closes #128
@aniketstiwari
Copy link

aniketstiwari commented Jun 24, 2019

As I can see through the comments the PR(lunohodov/eventical@b017fcf) is not yet merged with the master so what we need to do in this case. I am also getting the same issue and below are my configuration

Rails - 5.2.2
Ruby - 2.5.3
omniauth (1.4.3)
omniauth-linkedin-oauth2 (1.0.0)
omniauth-oauth2 (1.5.0)
omniauth-google-oauth2 (0.6.1)

@mhamzajutt96
Copy link

mhamzajutt96 commented Jul 2, 2019

This issue is not yet resolved, even I update all the gems and my Gemfile.lock also updated which is greater than 1.9
My Repository is: https://github.com/mhamzajutt96/sportscenter

@mathieujobin
Copy link

From the discussion above, it sounds like ...

  • omniauth by itself isn't vulnerable
  • rails by itself isn't vulnerable

some apps that do something very specific can be vulnerable.
like any apps that directly sends http query params to the database would be vulnerable to sql injection, would you blame MySQL for it?

it sounds like the CVE should be clarified and all that omniauth can do, is offer some documentation about it and a nice warning this using authentication gems isn't a guaranty of preventing security vulnerability within our applications

my two cents

@hakusaro
Copy link

@mathieujobin it is entirely unclear to me how anyone is supposed to contact NIST or GitHub to silence the alert on a site-wide scale or otherwise change the status of the alert to be more of an advisory warning against that behavior.

Plain and simple, the alert is too broad and does not adequately provide meaningful steps that can be taken to mitigate the problem. Instead, it scares everyone into a frenzy and causes people to disregard security alerts.

@aspiers
Copy link

aspiers commented Nov 10, 2019

The resolution is nicely documented here:

https://github.com/omniauth/omniauth/wiki/Resolving-CVE-2015-9284

A few days ago I contacted CVE requesting they add that URL as a reference, and they have obliged. So I think this issue can be closed now.

@ngoral
Copy link

ngoral commented Nov 19, 2019

In #809 it was said that for rails we should just use https://github.com/omniauth/omniauth

What's wrong with this solution over the one you suggest?

@aspiers
Copy link

aspiers commented Nov 19, 2019

The solution I have linked does use omniauth/omniauth. The solution is to use it in the right way.

@ngoral
Copy link

ngoral commented Nov 20, 2019

I'm sorry, I definitely meant omniauth-rails, not simply omniauth. However, it appears I missed that the thread in #809 is far longer than I noticed (GitHub had just cut the rest of the messages). I've read till the end now and see omniauth-rails hasn't been ready yet.

@CHTJonas
Copy link
Contributor

CHTJonas commented Nov 20, 2019

It seems like omniauth-rails is a bit of a nascent repo. I think the idea was to incorporate Rails-specific CSRF stuff in there but that hasn't happened yet (and there'd be a lot of inertia to get everyone to change their Gemfiles).

@pebneter
Copy link

I think you should be looking here (kind of the final solution): https://github.com/cookpad/omniauth-rails_csrf_protection

@aspiers
Copy link

aspiers commented Nov 20, 2019

I think you should be looking here (kind of the final solution): cookpad/omniauth-rails_csrf_protection

Installing https://github.com/cookpad/omniauth-rails_csrf_protection is great for introducing protection to your app, but it is not sufficient by itself; as its own README says:

You will then need to verify that all links in your application that would initiate OAuth request phase are being converted to a HTTP POST form that contains authenticity_token value.

The Resolving-CVE-2015-9284 wiki page I previously posted already suggests installing the above gem as the first of several steps, plus it goes into much greater detail explaining why this is all necessary and how to perform the other steps too. Therefore the wiki page is the best link to recommend to people looking for solutions, not the gem which is only one part of the solution.

@zhengxiangyue
Copy link

The resolution is nicely documented here:

https://github.com/omniauth/omniauth/wiki/Resolving-CVE-2015-9284

A few days ago I contacted CVE requesting they add that URL as a reference, and they have obliged. So I think this issue can be closed now.

This doesn't work for me because my provider only supports GET method to redirect to their login page.

@aspiers
Copy link

aspiers commented Mar 17, 2020

@zhengxiangyue commented on March 16, 2020 9:52 PM:

The resolution is nicely documented here:

omniauth/omniauth/wiki/Resolving-CVE-2015-9284

This doesn't work for me because my provider only supports GET method to redirect to their login page.

That sounds like an issue with your provider, not with omniauth itself.

@Papipo
Copy link

Papipo commented Aug 13, 2020

Is omniauth abandoned by any chance? There are lots of pending issues and no activity in the last month.

@BobbyMcWho
Copy link
Member

@Papipo there's a lot of work to be done and not enough folks to do the work unfortunately. I've only recently become a co-maintainer of the project and I do not personally have enough time to weed through stale issues and PRs, and also not enough time to code the solutions.

That being said, if someone makes a new PR or issue, I'll do my best to respond to it in a timely manner. I know this CVE is longstanding but because it would require breaking changes to resolve, it's not something that I feel comfortable rushing out.

@schlenks
Copy link

schlenks commented Sep 1, 2020

Given that this has been a long standing issue, and has it's own dedicated wiki page, has there been any consideration to releasing a v2.0.0 that has the breaking change of only supporting POST requests? That'd allow an upgrade path that doesn't involve instructing bundler-audit to ignore a CVE.

@josh-m-sharpe
Copy link

@BobbyMcWho There's been more than a few PRs, new gems and ideas. That part has been done. As maintainer, it's your responsibility to lead and direct the masses. I don't see that being done.

The wiki page describing the CVE issue and its mitigation is a good step. How about you create a page with a proposed solution so that someone can pick up the reigns and build/test the PR for your review?

@BobbyMcWho
Copy link
Member

@josh-m-sharpe there has been nothing new submitted that I haven't reviewed and/or merged since I've started to help maintain this. I'll reiterate again that I don't personally have the time to make this fix.

I started helping out to maintain this library because new PRs and issues were being ignored, and I figured some maintenance was better than none. I've been pretty good at responding, reviewing, and merging new items coming in a timely manner.

It is not my responsibility to "lead and direct the masses". I don't have the community clout nor the reach to try and do that. I'm happy with my scope of involvement in this project, and again, if you submit a PR that fixes this in a way that makes sense for the library, I'll work to get it reviewed, merged, and released under an appropriate version number for the change.

Have a great weekend 🙂

@josh-m-sharpe
Copy link

I apologize if my first message wasn't clear. I'm absolutely not suggesting that you (or any other maintainer) is responsible to build the fix for this. But, as the maintainers, I would think that you know better than others what is in the best interest of the library.

Set that stage and and the PR will follow :)

@Papipo
Copy link

Papipo commented Nov 16, 2020

I apologize if my first message wasn't clear.

I am afraid that your first message was pretty clear.

@BobbyMcWho
Copy link
Member

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

@BobbyMcWho
Copy link
Member

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

@BobbyMcWho
Copy link
Member

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

@jeremiemv
Copy link

Thank you for all your work @BobbyMcWho

@robotarmy
Copy link

robotarmy commented Jun 8, 2022

creating new issue

filbranden added a commit to filbranden/vimgolf that referenced this issue Aug 29, 2022
OmniAuth was warning about this issue in the logs:

WARN -- omniauth: (twitter)
  You are using GET as an allowed request method for OmniAuth. This may leave
  you open to CSRF attacks. As of v2.0.0, OmniAuth by default allows only POST
  to its own routes. You should review the following resources to guide your
  mitigation:
  https://github.com/omniauth/omniauth/wiki/Resolving-CVE-2015-9284
  omniauth/omniauth#960
  https://nvd.nist.gov/vuln/detail/CVE-2015-9284
  omniauth/omniauth#809

The trouble is that the fix requires using POST instead of GET in the
"Sign in with Twitter" link, but doing so with a link requires using
JavaScript. This can be done using Rails' UJS (Unobtrusive JavaScript)
library, but that would require us to actually adopt it (since it's
currently not used anywhere), it also makes the code harder and more
expensive to test, since the testing environment starts needing a
JavaScript engine.

The alternative to JavaScript is switching to using a form with a submit
button to perform the action with a POST method. I ended up taking this
latter approach of using a form with a submit button, and then using CSS
to make it render the same as a hyperlink.

The CSS I used to render a button as a link was based on the following
answer from StackOverflow:
https://stackoverflow.com/a/12642009/9447571

With some extra modifications to adapt to the link colors and the
underline on hover of our current links. The effect looked good enough
to me on my local testing with a couple of browsers.

There are some links to "signing in" from the box with instructions of
how to play one of the challenges, or how to unlock more answers.
I ended up handling those using JavaScript, to locate the "signin"
form by HTML element id and then submit that form.

I also had to update the RSpec tests to use `click_button` instead of
`click_link`, since the "Sign in with Twitter" is now a button and
not a link anymore.

Additionally to the changes described above, I also updated the
"Sign Out" link to use a form and a button and the /signout URL
to only accept requests via POST.

Tested locally, also confirmed that all RSpec tests passed as expected.
filbranden added a commit to filbranden/vimgolf that referenced this issue Aug 29, 2022
OmniAuth was warning about this issue in the logs:

WARN -- omniauth: (twitter)
  You are using GET as an allowed request method for OmniAuth. This may leave
  you open to CSRF attacks. As of v2.0.0, OmniAuth by default allows only POST
  to its own routes. You should review the following resources to guide your
  mitigation:
  https://github.com/omniauth/omniauth/wiki/Resolving-CVE-2015-9284
  omniauth/omniauth#960
  https://nvd.nist.gov/vuln/detail/CVE-2015-9284
  omniauth/omniauth#809

The trouble is that the fix requires using POST instead of GET in the
"Sign in with Twitter" link, but doing so with a link requires using
JavaScript. This can be done using Rails' UJS (Unobtrusive JavaScript)
library, but that would require us to actually adopt it (since it's
currently not used anywhere), it also makes the code harder and more
expensive to test, since the testing environment starts needing a
JavaScript engine.

The alternative to JavaScript is switching to using a form with a submit
button to perform the action with a POST method. I ended up taking this
latter approach of using a form with a submit button, and then using CSS
to make it render the same as a hyperlink.

The CSS I used to render a button as a link was based on the following
answer from StackOverflow:
https://stackoverflow.com/a/12642009/9447571

With some extra modifications to adapt to the link colors and the
underline on hover of our current links. The effect looked good enough
to me on my local testing with a couple of browsers.

There are some links to "signing in" from the box with instructions of
how to play one of the challenges, or how to unlock more answers.
I ended up handling those using JavaScript, to locate the "signin"
form by HTML element id and then submit that form.

I also had to update the RSpec tests to use `click_button` instead of
`click_link`, since the "Sign in with Twitter" is now a button and
not a link anymore.

Additionally to the changes described above, I also updated the
"Sign Out" link to use a form and a button and the /signout URL
to only accept requests via POST.

Tested locally, also confirmed that all RSpec tests passed as expected.
filbranden added a commit to filbranden/vimgolf that referenced this issue Sep 2, 2022
OmniAuth was warning about this issue in the logs:

WARN -- omniauth: (twitter)
  You are using GET as an allowed request method for OmniAuth. This may leave
  you open to CSRF attacks. As of v2.0.0, OmniAuth by default allows only POST
  to its own routes. You should review the following resources to guide your
  mitigation:
  https://github.com/omniauth/omniauth/wiki/Resolving-CVE-2015-9284
  omniauth/omniauth#960
  https://nvd.nist.gov/vuln/detail/CVE-2015-9284
  omniauth/omniauth#809

The trouble is that the fix requires using POST instead of GET in the
"Sign in with Twitter" link, but doing so with a link requires using
JavaScript. This can be done using Rails' UJS (Unobtrusive JavaScript)
library, but that would require us to actually adopt it (since it's
currently not used anywhere), it also makes the code harder and more
expensive to test, since the testing environment starts needing a
JavaScript engine.

The alternative to JavaScript is switching to using a form with a submit
button to perform the action with a POST method. I ended up taking this
latter approach of using a form with a submit button, and then using CSS
to make it render the same as a hyperlink.

The CSS I used to render a button as a link was based on the following
answer from StackOverflow:
https://stackoverflow.com/a/12642009/9447571

With some extra modifications to adapt to the link colors and the
underline on hover of our current links. The effect looked good enough
to me on my local testing with a couple of browsers.

There are some links to "signing in" from the box with instructions of
how to play one of the challenges, or how to unlock more answers.
I ended up handling those using JavaScript, to locate the "signin"
form by HTML element id and then submit that form.

I also had to update the RSpec tests to use `click_button` instead of
`click_link`, since the "Sign in with Twitter" is now a button and
not a link anymore.

Additionally to the changes described above, I also updated the
"Sign Out" link to use a form and a button and the /signout URL
to only accept requests via POST.

Tested locally, also confirmed that all RSpec tests passed as expected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
v2.0
  
ToDo
Development

No branches or pull requests