From 08ae7cf35ac8ffad38200691c3a40813ea855432 Mon Sep 17 00:00:00 2001 From: eutopian Date: Tue, 13 Nov 2018 18:30:50 -0500 Subject: [PATCH 1/2] use secure compare instead of equal compare to prevent timing attacks --- lib/omniauth/strategies/oauth2.rb | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/omniauth/strategies/oauth2.rb b/lib/omniauth/strategies/oauth2.rb index 28b3ae1..f7f7e2a 100644 --- a/lib/omniauth/strategies/oauth2.rb +++ b/lib/omniauth/strategies/oauth2.rb @@ -67,7 +67,7 @@ def callback_phase # rubocop:disable AbcSize, CyclomaticComplexity, MethodLength 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? || request.params["state"] != session.delete("omniauth.state")) + 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 @@ -105,6 +105,17 @@ def options_for(option) hash end + # constant-time comparison algorithm to prevent timing attacks + def secure_compare(a, b) + return false unless a.bytesize == b.bytesize + + l = a.unpack "C#{a.bytesize}" + + res = 0 + b.each_byte { |byte| res |= byte ^ l.shift } + res == 0 + end + # An error that is indicated in the OAuth 2.0 callback. # This could be a `redirect_uri_mismatch` or other class CallbackError < StandardError From c65a01563dee036f5a665af42544437f946f937b Mon Sep 17 00:00:00 2001 From: eutopian Date: Wed, 14 Nov 2018 18:27:40 -0500 Subject: [PATCH 2/2] added test --- lib/omniauth/strategies/oauth2.rb | 10 +++++----- spec/omniauth/strategies/oauth2_spec.rb | 10 ++++++++++ 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/lib/omniauth/strategies/oauth2.rb b/lib/omniauth/strategies/oauth2.rb index f7f7e2a..ff6b4d5 100644 --- a/lib/omniauth/strategies/oauth2.rb +++ b/lib/omniauth/strategies/oauth2.rb @@ -106,14 +106,14 @@ def options_for(option) end # constant-time comparison algorithm to prevent timing attacks - def secure_compare(a, b) - return false unless a.bytesize == b.bytesize + def secure_compare(string_a, string_b) + return false unless string_a.bytesize == string_b.bytesize - l = a.unpack "C#{a.bytesize}" + l = string_a.unpack "C#{string_a.bytesize}" res = 0 - b.each_byte { |byte| res |= byte ^ l.shift } - 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. diff --git a/spec/omniauth/strategies/oauth2_spec.rb b/spec/omniauth/strategies/oauth2_spec.rb index 2008cc9..33d69b6 100644 --- a/spec/omniauth/strategies/oauth2_spec.rb +++ b/spec/omniauth/strategies/oauth2_spec.rb @@ -87,6 +87,16 @@ def app instance.callback_phase end end + + describe "#secure_params" 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