CVE-2013-4562: Cross-Site Request Forgery (CSRF) (CWE-352)
value: The omniauth-facebook gem 1.4.1 before 1.5.0 does not properly store the session parameter, which allows remote attackers to conduct cross-site request forgery (CSRF) attacks via the state parameter.
CWE description: The web application does not, or can not, sufficiently verify whether a well-formed, valid, consistent request was intentionally provided by the user who submitted the request.
Published: 2014-05-13T15:55Z
Repository: https://github.com/mkdynamic/omniauth-facebook
Language: Ruby
Files changed: README.md, lib/omniauth/facebook/version.rb, lib/omniauth/strategies/facebook.rb, test/support/shared_examples.rb, test/test.rb
Commit date: 2013-11-12 13:15:58 -0800
Commit message: fix CSRF vulnerability. prepare 1.5.0 release

Diff: diff --git a/README.md b/README.md
index 1fc82e4..24ab3da 100644
--- a/README.md
+++ b/README.md
@@ -50,12 +50,10 @@ end
 
 ### Per-Request Options
 
 If you want to set the `display` format or `scope` on a per-request basis, you can just pass it to the OmniAuth request phase URL, for example: `/auth/facebook?display=popup` or `/auth/facebook?scope=email`.
 
-You can also pass through a `state` param which will be passed along to the callback url.
-
 ### Custom Callback URL/Path
 
 You can set a custom `callback_url` or `callback_path` option to override the default value. See [OmniAuth::Strategy#callback_url](https://github.com/intridea/omniauth/blob/master/lib/omniauth/strategy.rb#L411) for more details on the default.
 
 ## Auth Hash
diff --git a/lib/omniauth/facebook/version.rb b/lib/omniauth/facebook/version.rb
index 1efaaf2..f76afd8 100644
--- a/lib/omniauth/facebook/version.rb
+++ b/lib/omniauth/facebook/version.rb
@@ -1,5 +1,5 @@
 module OmniAuth
   module Facebook
-    VERSION = "1.4.1"
+    VERSION = "1.5.0"
   end
 end
diff --git a/lib/omniauth/strategies/facebook.rb b/lib/omniauth/strategies/facebook.rb
index 92da028..9575689 100644
--- a/lib/omniauth/strategies/facebook.rb
+++ b/lib/omniauth/strategies/facebook.rb
@@ -110,24 +110,21 @@ module OmniAuth
       def access_token_options
         options.access_token_options.inject({}) { |h,(k,v)| h[k.to_sym] = v; h }
       end
 
       ##
-      # You can pass +display+, +state+ or +scope+ params to the auth request, if
+      # You can pass +display+ or +scope+ params to the auth request, if
       # you need to set them dynamically. You can also set these options
       # in the OmniAuth config :authorize_params option.
       #
       # /auth/facebook?display=popup&state=ABC
       #
       def authorize_params
         super.tap do |params|
-          %w[display state scope].each do |v|
+          %w[display scope].each do |v|
             if request.params[v]
               params[v.to_sym] = request.params[v]
-
-              # to support omniauth-oauth2's auto csrf protection
-              session['omniauth.state'] = params[:state] if v == 'state'
             end
           end
 
           params[:scope] ||= DEFAULT_SCOPE
         end
diff --git a/test/support/shared_examples.rb b/test/support/shared_examples.rb
index 053b3b8..dfbf55f 100644
--- a/test/support/shared_examples.rb
+++ b/test/support/shared_examples.rb
@@ -48,24 +48,24 @@ module OAuth2StrategyTests
       refute_empty strategy.authorize_params['state']
       refute_empty strategy.session['omniauth.state']
       assert_equal strategy.authorize_params['state'], strategy.session['omniauth.state']
     end
 
-    test 'should store state in the session when present in authorize params vs. a random one' do
+    test 'should not store state in the session when present in authorize params vs. a random one' do
       @options = { :authorize_params => { :state => 'bar' } }
       refute_empty strategy.authorize_params['state']
-      assert_equal 'bar', strategy.authorize_params[:state]
+      refute_equal 'bar', strategy.authorize_params[:state]
       refute_empty strategy.session['omniauth.state']
-      assert_equal 'bar', strategy.session['omniauth.state']
+      refute_equal 'bar', strategy.session['omniauth.state']
     end
 
-    test 'should store state in the session when present in request params vs. a random one' do
+    test 'should not store state in the session when present in request params vs. a random one' do
       @request.stubs(:params).returns({ 'state' => 'foo' })
       refute_empty strategy.authorize_params['state']
-      assert_equal 'foo', strategy.authorize_params[:state]
+      refute_equal 'foo', strategy.authorize_params[:state]
       refute_empty strategy.session['omniauth.state']
-      assert_equal 'foo', strategy.session['omniauth.state']
+      refute_equal 'foo', strategy.session['omniauth.state']
     end
   end
 
   module TokenParamsTests
     extend BlockTestHelper
diff --git a/test/test.rb b/test/test.rb
index d0456d2..21ecdba 100644
--- a/test/test.rb
+++ b/test/test.rb
@@ -54,16 +54,10 @@ class AuthorizeParamsTest < StrategyTestCase
     @request.stubs(:params).returns({ 'display' => 'touch' })
     assert strategy.authorize_params.is_a?(Hash)
     assert_equal 'touch', strategy.authorize_params[:display]
   end
 
-  test 'includes state parameter from request when present' do
-    @request.stubs(:params).returns({ 'state' => 'some_state' })
-    assert strategy.authorize_params.is_a?(Hash)
-    assert_equal 'some_state', strategy.authorize_params[:state]
-  end
-
   test 'overrides default scope with parameter passed from request' do
     @request.stubs(:params).returns({ 'scope' => 'email' })
     assert strategy.authorize_params.is_a?(Hash)
     assert_equal 'email', strategy.authorize_params[:scope]
   end

Vulnerable code: You can also pass through a `state` param which will be passed along to the callback url.

    VERSION = "1.4.1"
      # You can pass +display+, +state+ or +scope+ params to the auth request, if
          %w[display state scope].each do |v|

              # to support omniauth-oauth2's auto csrf protection
              session['omniauth.state'] = params[:state] if v == 'state'
    test 'should store state in the session when present in authorize params vs. a random one' do
      assert_equal 'bar', strategy.authorize_params[:state]
      assert_equal 'bar', strategy.session['omniauth.state']
    test 'should store state in the session when present in request params vs. a random one' do
      assert_equal 'foo', strategy.authorize_params[:state]
      assert_equal 'foo', strategy.session['omniauth.state']
  test 'includes state parameter from request when present' do
    @request.stubs(:params).returns({ 'state' => 'some_state' })
    assert strategy.authorize_params.is_a?(Hash)
    assert_equal 'some_state', strategy.authorize_params[:state]
  end

Fixed code:     VERSION = "1.5.0"
      # You can pass +display+ or +scope+ params to the auth request, if
          %w[display scope].each do |v|
    test 'should not store state in the session when present in authorize params vs. a random one' do
      refute_equal 'bar', strategy.authorize_params[:state]
      refute_equal 'bar', strategy.session['omniauth.state']
    test 'should not store state in the session when present in request params vs. a random one' do
      refute_equal 'foo', strategy.authorize_params[:state]
      refute_equal 'foo', strategy.session['omniauth.state']