From c4069b02d4d02d8b40dc3830509ddaf57dfec630 Mon Sep 17 00:00:00 2001 From: James Coglan Date: Thu, 2 Jul 2015 14:45:43 +0100 Subject: [PATCH] Persist the 'state' parameter until a successful callback. We discovered the following issue while using [omniauth-facebook][1]. We are seeing an unexpectedly high number of CSRF violations (roughly 6% of sign-ins) when users return from facebook.com. Every time the user visits `/auth/facebook` and invokes the `request_phase` method, a new `state` value is generated and put in the session. If a user opens the site in two tabs and navigates to `/auth/facebook` in each, then they have two copies of https://www.facebook.com/dialog/oauth open, each with a different `state` param. But, only one of these state params is in the session under the `omniauth.state` key. One of these pages will thus have a `state` param that does not match the session. It is also possible to trigger this with concurrent or duplicate requests, or by using the back/forward buttons, causing a race condition in updating the session. The page with the `state` value that "lost" the race will authorize Facebook's permissions, but then fail when redirected to `/auth/facebook/callback` due to the token mismatch. I wondered why this gem always assigns a new `state` value on visiting `/auth/:provider`, compared to how [rack-protection][2] and [Rails][3] implement CSRF protection using a guarded assignment. My assumption in this patch is that this is to avoid sending a value to one provider that could be redeemed by another. If I send a state value to alice.com, then someone working for alice.com could take that value and the account of the user that authenticated there, then email the user a phishing callback link for bob.com, including a `state` value that will work. To avoid this, I have introduced a namespace into the session: each `state` value is keyed by the class name of the particular provider, so you can have a different state value per provider, but have them be reusable. The value is still removed from the session on completion to avoid replay attacks. Keeping the `state` constant until the auth process completes means it's safe to open multiple tabs and perform concurrent requests, or anything else that might leave the session in an inconsistent or unpredictable state. [1]: https://github.com/mkdynamic/omniauth-facebook [2]: https://github.com/sinatra/rack-protection/blob/v1.5.3/lib/rack/protection/authenticity_token.rb#L24 [3]: https://github.com/rails/rails/blob/v4.2.3/actionpack/lib/action_controller/metal/request_forgery_protection.rb#L316 --- lib/omniauth/strategies/oauth2.rb | 39 +++++++++++++++------ spec/omniauth/strategies/oauth2_spec.rb | 46 ++++++++++++++++++++++--- 2 files changed, 71 insertions(+), 14 deletions(-) diff --git a/lib/omniauth/strategies/oauth2.rb b/lib/omniauth/strategies/oauth2.rb index 0197452..0abfb95 100644 --- a/lib/omniauth/strategies/oauth2.rb +++ b/lib/omniauth/strategies/oauth2.rb @@ -53,14 +53,10 @@ def request_phase end def authorize_params - options.authorize_params[:state] = SecureRandom.hex(24) - params = options.authorize_params.merge(options_for("authorize")) - if OmniAuth.config.test_mode - @env ||= {} - @env["rack.session"] ||= {} - end - session["omniauth.state"] = params[:state] - params + site = self.class.name + state = state_store.fetch(site) { state_store[site] = SecureRandom.hex(24) } + options.authorize_params[:state] = state + options.authorize_params.merge(options_for("authorize")) end def token_params @@ -69,11 +65,18 @@ def token_params def callback_phase # rubocop:disable AbcSize, CyclomaticComplexity, MethodLength, PerceivedComplexity error = request.params["error_reason"] || request.params["error"] + site = self.class.name + expected_state = state_store[site] + actual_state = request.params["state"].to_s + if error - fail!(error, CallbackError.new(request.params["error"], request.params["error_description"] || request.params["error_reason"], request.params["error_uri"])) - elsif !options.provider_ignores_state && (request.params["state"].to_s.empty? || request.params["state"] != session.delete("omniauth.state")) + description = request.params["error_description"] || request.params["error_reason"] + error_uri = request.params["error_uri"] + fail!(error, CallbackError.new(request.params["error"], description, error_uri)) + elsif !options.provider_ignores_state && (actual_state.empty? || actual_state != expected_state) fail!(:csrf_detected, CallbackError.new(:csrf_detected, "CSRF detected")) else + state_store.delete(site) self.access_token = build_access_token self.access_token = access_token.refresh! if access_token.expired? super @@ -109,6 +112,22 @@ def options_for(option) hash end + private + + def state_store + if OmniAuth.config.test_mode + @env ||= {} + @env["rack.session"] ||= {} + end + + state_key = "omniauth.oauth2.state" + state_store = session[state_key] + unless state_store.is_a?(Hash) + state_store = session[state_key] = {} + end + state_store + end + # An error that is indicated in the OAuth 2.0 callback. # This could be a `redirect_uri_mismatch` or other class CallbackError < StandardError diff --git a/spec/omniauth/strategies/oauth2_spec.rb b/spec/omniauth/strategies/oauth2_spec.rb index 0bffcde..4c9db94 100644 --- a/spec/omniauth/strategies/oauth2_spec.rb +++ b/spec/omniauth/strategies/oauth2_spec.rb @@ -53,11 +53,49 @@ def app expect(instance.authorize_params["scope"]).to eq("bar") expect(instance.authorize_params["foo"]).to eq("baz") end + end - it "includes random state in the authorize params" do - instance = subject.new("abc", "def") - expect(instance.authorize_params.keys).to eq(["state"]) - expect(instance.session["omniauth.state"]).not_to be_empty + describe "state handling" do + SocialNetwork = Class.new(OmniAuth::Strategies::OAuth2) + + let(:client_options) { {:site => "https://graph.example.com"} } + let(:instance) { SocialNetwork.new(-> env {}) } + + before do + allow(SecureRandom).to receive(:hex).with(24).and_return("hex-1", "hex-2") + end + + it "includes a state scoped to the client" do + expect(instance.authorize_params["state"]).to eq("hex-1") + expect(instance.session["omniauth.oauth2.state"]).to eq("SocialNetwork" => "hex-1") + end + + context "once a state value has been generated" do + before do + instance.authorize_params + end + + it "does not replace an existing session value" do + expect(instance.authorize_params["state"]).to eq("hex-1") + expect(instance.session["omniauth.oauth2.state"]).to eq("SocialNetwork" => "hex-1") + end + end + + context "on a successful callback" do + let(:request) { double("Request", :params => {"code" => "auth-code", "state" => "hex-1"}) } + let(:access_token) { double("AccessToken", :expired? => false, :expires? => false, :token => "access-token") } + + before do + allow(instance).to receive(:request).and_return(request) + allow(instance).to receive(:build_access_token).and_return(access_token) + + instance.authorize_params + instance.callback_phase + end + + it "removes the value from the session" do + expect(instance.session["omniauth.oauth2.state"]).to eq({}) + end end end