From d9f32433183fd07ec66eb81690ce8dc565a7846c Mon Sep 17 00:00:00 2001 From: Jack Hartzler Date: Wed, 27 Sep 2023 16:25:30 -0400 Subject: [PATCH 1/4] Prevent timing attack on CSRF, completing wonderful pr by @eutopian --- lib/omniauth/strategies/oauth2.rb | 14 ++++++++++++-- spec/omniauth/strategies/oauth2_spec.rb | 10 ++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/lib/omniauth/strategies/oauth2.rb b/lib/omniauth/strategies/oauth2.rb index 468f2aa..4a0daa4 100644 --- a/lib/omniauth/strategies/oauth2.rb +++ b/lib/omniauth/strategies/oauth2.rb @@ -83,8 +83,7 @@ def token_params def callback_phase # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/MethodLength, Metrics/PerceivedComplexity error = request.params["error_reason"] || request.params["error"] - if !options.provider_ignores_state && (request.params["state"].to_s.empty? || request.params["state"] != session.delete("omniauth.state")) - fail!(:csrf_detected, CallbackError.new(:csrf_detected, "CSRF detected")) + elsif !options.provider_ignores_state && (request.params["state"].to_s.empty? || !secure_compare(request.params["state"], session.delete("omniauth.state"))) elsif error fail!(error, CallbackError.new(request.params["error"], request.params["error_description"] || request.params["error_reason"], request.params["error_uri"])) else @@ -144,6 +143,17 @@ def options_for(option) hash end + # constant-time comparison algorithm to prevent timing attacks + def secure_compare(string_a, string_b) + return false unless string_a.bytesize == string_b.bytesize + + l = string_a.unpack "C#{string_a.bytesize}" + + res = 0 + string_b.each_byte { |byte| res |= byte ^ l.shift } + res.zero? + 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 ff6176c..a267773 100644 --- a/spec/omniauth/strategies/oauth2_spec.rb +++ b/spec/omniauth/strategies/oauth2_spec.rb @@ -168,6 +168,16 @@ def app end end end + + describe "#secure_compare" do + subject { fresh_strategy } + + it "returns true when the two inputs are the same and false otherwise" do + instance = subject.new("abc", "def") + expect(instance.send(:secure_compare, "a", "a")).to be true + expect(instance.send(:secure_compare, "b", "a")).to be false + end + end end describe OmniAuth::Strategies::OAuth2::CallbackError do From e962e86996dd2ed1ac3fcb037c281e5367677a6f Mon Sep 17 00:00:00 2001 From: Jack Hartzler Date: Wed, 27 Sep 2023 16:49:14 -0400 Subject: [PATCH 2/4] Fix elsif block --- lib/omniauth/strategies/oauth2.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/omniauth/strategies/oauth2.rb b/lib/omniauth/strategies/oauth2.rb index 4a0daa4..2ff9e0f 100644 --- a/lib/omniauth/strategies/oauth2.rb +++ b/lib/omniauth/strategies/oauth2.rb @@ -83,9 +83,10 @@ def token_params def callback_phase # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/MethodLength, Metrics/PerceivedComplexity error = request.params["error_reason"] || request.params["error"] - elsif !options.provider_ignores_state && (request.params["state"].to_s.empty? || !secure_compare(request.params["state"], session.delete("omniauth.state"))) elsif 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? || !secure_compare(request.params["state"], session.delete("omniauth.state"))) + fail!(:csrf_detected, CallbackError.new(:csrf_detected, "CSRF detected")) else self.access_token = build_access_token self.access_token = access_token.refresh! if access_token.expired? From 3af5f9476bfd3d6a5f72b9b31f8fc2d588eb7317 Mon Sep 17 00:00:00 2001 From: Jack Hartzler Date: Wed, 27 Sep 2023 16:50:31 -0400 Subject: [PATCH 3/4] elsif -> if --- lib/omniauth/strategies/oauth2.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/omniauth/strategies/oauth2.rb b/lib/omniauth/strategies/oauth2.rb index 2ff9e0f..66e1530 100644 --- a/lib/omniauth/strategies/oauth2.rb +++ b/lib/omniauth/strategies/oauth2.rb @@ -83,7 +83,7 @@ def token_params def callback_phase # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/MethodLength, Metrics/PerceivedComplexity error = request.params["error_reason"] || request.params["error"] - elsif error + 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? || !secure_compare(request.params["state"], session.delete("omniauth.state"))) fail!(:csrf_detected, CallbackError.new(:csrf_detected, "CSRF detected")) From cec0655f8465d14a27ebdf573a219462ff2272ec Mon Sep 17 00:00:00 2001 From: Jack Hartzler Date: Wed, 27 Sep 2023 17:02:18 -0400 Subject: [PATCH 4/4] Had the logic flipped --- lib/omniauth/strategies/oauth2.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/omniauth/strategies/oauth2.rb b/lib/omniauth/strategies/oauth2.rb index 66e1530..1588926 100644 --- a/lib/omniauth/strategies/oauth2.rb +++ b/lib/omniauth/strategies/oauth2.rb @@ -83,10 +83,10 @@ def token_params def callback_phase # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/MethodLength, Metrics/PerceivedComplexity error = request.params["error_reason"] || request.params["error"] - 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? || !secure_compare(request.params["state"], session.delete("omniauth.state"))) + if !options.provider_ignores_state && (request.params["state"].to_s.empty? || !secure_compare(request.params["state"], session.delete("omniauth.state"))) fail!(:csrf_detected, CallbackError.new(:csrf_detected, "CSRF detected")) + elsif error + fail!(error, CallbackError.new(request.params["error"], request.params["error_description"] || request.params["error_reason"], request.params["error_uri"])) else self.access_token = build_access_token self.access_token = access_token.refresh! if access_token.expired?