Skip to content

Commit

Permalink
Merge pull request #174 from jhartzler/master
Browse files Browse the repository at this point in the history
Prevent timing attack on CSRF, completing wonderful pr by @eutopian
  • Loading branch information
BobbyMcWho authored Oct 12, 2023
2 parents 3e7ee11 + cec0655 commit c830138
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 1 deletion.
13 changes: 12 additions & 1 deletion lib/omniauth/strategies/oauth2.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
if !options.provider_ignores_state && (request.params["state"].to_s.empty? || 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"]))
Expand Down Expand Up @@ -144,6 +144,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
Expand Down
10 changes: 10 additions & 0 deletions spec/omniauth/strategies/oauth2_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit c830138

Please sign in to comment.