Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

CSRF vulnerability, injecting state in session #25

Merged
merged 1 commit into from

6 participants

@mbleigh
Owner

So you're saying that OmniAuth simply should not support user-passed state at all?

@homakov

@mbleigh hm.. two ways:
1) do not support it at all and use for internal csrf protection
2) support it and append it to state: CSRFTOKEN.USERINPUT and on callback slice it back... it will look really weird to user... but it will not break apps
so i prefer way 1 and commit does so. if anyone wants to go way 2 - go ahead please

@homakov

0day vulnerability, 10 days and not merged? come on...

@mbleigh

@mbleigh mbleigh merged commit 451f7ec into intridea:master
@thesp0nge

Hi there guys. I'm codesake-dawn (a security ruby source code scanner) maintainer. For this CVE I'm marking version 1.1.1 as vulnerable.
If I've understood properly, vulnerability is fixed in the code but a new gem has not been yet released.
Can I ask why?

@guilhermesimoes

No, this patch is part of v1.1.1. You can check it here. Just below the commit comment, you can see the v1.1.1 tag.

As for why a new gem version has not been released after v1.1.1, see #36...

@skorth skorth referenced this pull request in rubysec/ruby-advisory-db
Merged

Fix patched version for omniauth-oauth2 gem #85

@postmodern

@guilhermesimoes but are we correct in that this patch is not present in the v1.1.1 release? Need to make sure for ruby-advisory-db.

@postmodern postmodern referenced this pull request in rubysec/ruby-advisory-db
Merged

Correct patched versions for omniauth-oauth2 #119

@guilhermesimoes

This patch is present in the v1.1.1 release.

If you go to this Pull Request's commit, you can see that it is present in versions over and including v1.1.1.

On this commits page if you search for the author of the patch, homakov, you can see that the commit is included before the version is bumped to 1.1.1. (Be mindful that this commits page will change over time as people commit to the repo.)

@derekprior

@guilhermesimoes, can you please update the CVE?

It very clearly states version 1.1.1 is affected:

Cross-site request forgery (CSRF) vulnerability in the omniauth-oauth2 gem 1.1.1 and earlier for Ruby allows remote attackers to hijack the authentication of users for requests that modify session state.

@derekprior

There appears to be some confusion over affected versions in the ecosystem. See: mkdynamic/omniauth-facebook#162

@guilhermesimoes

There's some confusion, but not on my part. This patch is clearly present in version 1.1.1. So both the CVE and that omniauth-facebook pull request are wrong.

I have no idea of how to edit that CVE though.

@derekprior

I'm pleased you are not confused, though I think you can see how some might be given the CVE. As a library maintainer, you're probably best to submit for an update to eliminate any future confusion. See: https://cve.mitre.org/about/faqs.html#b12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 1 addition and 3 deletions.
  1. +1 −3 lib/omniauth/strategies/oauth2.rb
View
4 lib/omniauth/strategies/oauth2.rb
@@ -49,9 +49,7 @@ def request_phase
end
def authorize_params
- if options.authorize_params[:state].to_s.empty?
- options.authorize_params[:state] = SecureRandom.hex(24)
- end
+ options.authorize_params[:state] = SecureRandom.hex(24)
params = options.authorize_params.merge(options.authorize_options.inject({}){|h,k| h[k.to_sym] = options[k] if options[k]; h})
if OmniAuth.config.test_mode
@env ||= {}
Something went wrong with that request. Please try again.