Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add secure equals method to prevent timing attacks #56

Closed
wants to merge 1 commit into from

Conversation

leishman
Copy link
Contributor

@mgomes @kjg

I updated the signatures_match? method to prevent timing attacks. The equality operator should not be used to compare two strings for authentication in Ruby (I believe). For example:

'abc' == 'abc' will take longer to evaluate than 'def' == 'abc' because the ruby interpreter will return false after analyzing the first bytes of each string in the second example, but will iterate through each byte of the strings in the first example. This information can be used by a malicious actor in what is known as a timing attack.

There are two solutions I know of to protect against this timing attack:

  • Implement a "constant time" equality method (constant in that it takes the same amount of time for any strings of given lengths, not that it is O(n))
  • Test the equality of the HMAC of the strings (which in this case are HMACs themselves).

I learned about this attack from a Stanford cryptography professor in this lecture and he recommended the solution I propose here. His example is for Python but I assume that it applies to Ruby as well.

All tests pass.

@kjg
Copy link
Collaborator

kjg commented Dec 20, 2014

I had never heard of a timing attack before, that's pretty interesting. Do you think it would be possible to perform a timing attack again api_auth considering the method used to generate the canonical string combined with the fact that the data here is being transmitted over the internet which introduces a lot of variability in time?

@RLovelett
Copy link

@kjg watch the lecture in the link. He addresses the Internet latency/variability thing. Short answer: yes it can still be done over the net.

@dacamp
Copy link

dacamp commented Jan 12, 2015

My understanding is that Benchmark isn't great for testing timing attacks, as it does a fair amount of rounding. I wanted to put the numbers in here anyways in hopes that some solution is implemented to limit risks associated with these types of attacks.

I tested master, @leishman's pull request, and Fast Secure Compare - (see [below](#Fast secure compare)) using the same two randomly generated secrets over 50,000 iterations of ApiAuth.send(:signatures_match?, signed_request, secret_key) where the signed_request remains static and secret_key is either a 'Match' (secret_1) or 'Differ' (secret_2).

Here's the script I used for testing: https://gist.github.com/dacamp/8d970d7065931045d343

Candidate: Local hack using fast_secure_compare

ApiAuth#signatures_match? (50000 iterations) timing attack against:
Secret 1: MVkg7f9VvH4eo1dD74jZX7egmTsLFoWbHeRxBWLpiiSr0UhuuCraEIAM/9k4wk9mcuijPvwTsWlfvBRqHoS78Q==
          Signatures match? true
Secret 2: ZNExKT/H/cHsLLQknAIrXslgFddBhp9T4cPpUCa4061/6Cbyv5mvXBE2ix0Qx3FO600R0oXx532FuhTpEh0jIg==
          Signatures match? false

Rehearsal -------------------------------------------
Match:    2.080000   0.010000   2.090000 (  2.096422)
Differ:   2.030000   0.010000   2.040000 (  2.044381)
---------------------------------- total: 4.130000sec

              user     system      total        real
Match:    1.920000   0.010000   1.930000 (  1.929009)
Differ:   1.970000   0.010000   1.980000 (  1.976619)
Candidate: leishman/api_auth (2fe4df2319)

ApiAuth#signatures_match? (50000 iterations) timing attack against:
Secret 1: MVkg7f9VvH4eo1dD74jZX7egmTsLFoWbHeRxBWLpiiSr0UhuuCraEIAM/9k4wk9mcuijPvwTsWlfvBRqHoS78Q==
          Signatures match? true
Secret 2: ZNExKT/H/cHsLLQknAIrXslgFddBhp9T4cPpUCa4061/6Cbyv5mvXBE2ix0Qx3FO600R0oXx532FuhTpEh0jIg==
          Signatures match? false

Rehearsal -------------------------------------------
Match:    2.250000   0.010000   2.260000 (  2.290130)
Differ:   2.170000   0.020000   2.190000 (  2.206009)
---------------------------------- total: 4.450000sec

              user     system      total        real
Match:    2.070000   0.010000   2.080000 (  2.083388)
Differ:   2.410000   0.010000   2.420000 (  2.441620)
Candidate: mgomes/api_auth (master)

ApiAuth#signatures_match? (50000 iterations) timing attack against:
Secret 1: MVkg7f9VvH4eo1dD74jZX7egmTsLFoWbHeRxBWLpiiSr0UhuuCraEIAM/9k4wk9mcuijPvwTsWlfvBRqHoS78Q==
          Signatures match? true
Secret 2: ZNExKT/H/cHsLLQknAIrXslgFddBhp9T4cPpUCa4061/6Cbyv5mvXBE2ix0Qx3FO600R0oXx532FuhTpEh0jIg==
          Signatures match? false

Rehearsal -------------------------------------------
Match:    2.130000   0.020000   2.150000 (  2.168062)
Differ:   2.230000   0.010000   2.240000 (  2.283192)
---------------------------------- total: 4.390000sec

              user     system      total        real
Match:    2.110000   0.010000   2.120000 (  2.121405)
Differ:   2.020000   0.010000   2.030000 (  2.032040)
Fast secure compare

Url: https://github.com/daxtens/fast_secure_compare

Method Hack:

diff --git a/lib/api_auth/base.rb b/lib/api_auth/base.rb
index 13142be..694fedb 100644
--- a/lib/api_auth/base.rb
+++ b/lib/api_auth/base.rb
@@ -8,6 +8,7 @@
 # Rails ActiveResource, it will integrate with that. It will even generate the
 # secret keys necessary for your clients to sign their requests.
 module ApiAuth
+  require 'fast_secure_compare'

   class << self

@@ -78,7 +79,7 @@ module ApiAuth
       if match_data = parse_auth_header(headers.authorization_header)
         hmac_from_user = match_data[2]
         hmac_of_request = hmac_signature(request, secret_key)
-        return secure_equals?(hmac_from_user, hmac_of_request, secret_key)
+        return FastSecureCompare.compare(hmac_from_user, hmac_of_request)
       end
       false
     end

@johnmcconnell
Copy link

@mgomes @leishman

I'm not seeing how this causes an exploit. Using the timing attack you can find calculated_hmac using a time of calculation, but that is unencrypted in the request headers anyway.

In other words, I'm not seeing how using the attack can cause someone to find the secret_key (or edit the message, or some other vulnerability).

For example, suppose you want to find the secret key. You start with 'a' * length and you find
'd' is the fastest first character. So your secret key becomes 'd' + 'a' * (length - 1).

However, iterating through the next character doesn't help you because changing the next character causes a significant change in the calculated hmac_signature.

More formally signature_hmac(message, 'd' + 'a' * (length - 1)) =~ signature_hmac(message, secret_key) does not guarantee or even correlate with the secret_key being 'd' + more_characters.

@jonahwh jonahwh mentioned this pull request May 23, 2016
@will0
Copy link
Contributor

will0 commented Oct 20, 2016

Howdy! I'm interested in getting this resolved, and I'd be happy to take on resolving the conflicts. There are a couple of other related issues (happy to say if you're comfortable with this channel) in HEAD I'm interested in helping to resolve that collectively make attack easier

@johnmcconnell
Copy link

@will0 I think there is a PR that is already up.

Also, I realized secure comp is needed to prevent forging a request

@leishman
Copy link
Contributor Author

leishman commented Oct 20, 2016

@johnmcconnell Just saw your comment from last year. The attack is not about finding the key, it's about brute-forcing the HMAC itself.

If the HMAC is unencrypted in a request header, then it's vulnerable to a MITM.
Edit: but that's why we use TLS

@will0
Copy link
Contributor

will0 commented Oct 20, 2016

@johnmcconnell do you mean this PR that is up?

@johnmcconnell
Copy link

johnmcconnell commented Oct 20, 2016

@will0 On may 23rd a pull request was created to solve this issue look at: #110

@will0
Copy link
Contributor

will0 commented Oct 20, 2016

@johnmcconnell thank you for pointing that out 👍

@kjg
Copy link
Collaborator

kjg commented Oct 26, 2016

Fixed by #133 Thanks!

@kjg kjg closed this Oct 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants