Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Fix timing attack vulnerability in ActiveSupport::MessageVerifier.

Use a constant-time comparison algorithm to compare the candidate HMAC with the calculated HMAC to prevent leaking information about the calculated HMAC.

Signed-off-by: Michael Koziarski <michael@koziarski.com>
  • Loading branch information...
commit 1f07a89c5946910fc28ea5ccd1da6af8a0f972a0 1 parent 2b82708
@codahale codahale authored NZKoz committed
Showing with 16 additions and 3 deletions.
  1. +16 −3 activesupport/lib/active_support/message_verifier.rb
View
19 activesupport/lib/active_support/message_verifier.rb
@@ -25,10 +25,10 @@ def initialize(secret, digest = 'SHA1')
def verify(signed_message)
data, digest = signed_message.split("--")
- if digest != generate_digest(data)
- raise InvalidSignature
- else
+ if secure_compare(digest, generate_digest(data))
Marshal.load(ActiveSupport::Base64.decode64(data))
+ else
+ raise InvalidSignature
end
end
@@ -38,6 +38,19 @@ def generate(value)
end
private
+ # constant-time comparison algorithm to prevent timing attacks
+ def secure_compare(a, b)
+ if a.length == b.length
+ result = 0
+ for i in 0..(a.length - 1)
+ result |= a[i] ^ b[i]
+ end
+ result == 0
+ else
+ false
+ end
+ end
+
def generate_digest(data)
require 'openssl' unless defined?(OpenSSL)
OpenSSL::HMAC.hexdigest(OpenSSL::Digest::Digest.new(@digest), @secret, data)

3 comments on commit 1f07a89

@julik

I know it seems very cool and such but ain't implementing your own strcmp sort of overkill?

@lastobelus

@julik:

wha? do you mean <=> ?

it's not constant time. why would you make such a comment without checking?

irb(main):005:0> basestring = "abcdefghijklmnopqrstuvwxyz123456789abcdef"
=> "abcdefghijklmnopqrstuvwxyz123456789abcdef"
irb(main):006:0> sameaaaaaa = "abcdefghijklmnopqrstuvwxyz123456789abcdef"
=> "abcdefghijklmnopqrstuvwxyz123456789abcdef"
irb(main):007:0> difearlyaa = "aacdefghijklmnopqrstuvwxyz123456789abcdef"
=> "aacdefghijklmnopqrstuvwxyz123456789abcdef"
irb(main):008:0> differlate = "abcdefghijklmnopqrstuvwxyz123456789abcdee"
=> "abcdefghijklmnopqrstuvwxyz123456789abcdee"
irb(main):009:0> require 'benchmark'
=> true
irb(main):042:0> n=15000000
=> 15000000
irb(main):043:0> Benchmark.bm { |x|
irb(main):044:1* x.report { n.times { sameaaaaaa <=> basestring }}
irb(main):045:1> x.report { n.times { difearlyaa <=> basestring }}
irb(main):046:1> x.report { n.times { differlate <=> basestring }}
irb(main):047:1> }
user system total real
2.650000 0.000000 2.650000 ( 2.648783)
2.500000 0.000000 2.500000 ( 2.497226)
2.630000 0.000000 2.630000 ( 2.640446)
=> true

@lastobelus

bah. try again with markdown:

  irb(main):005:0> basestring = "abcdefghijklmnopqrstuvwxyz123456789abcdef"
  => "abcdefghijklmnopqrstuvwxyz123456789abcdef"
  irb(main):006:0> sameaaaaaa = "abcdefghijklmnopqrstuvwxyz123456789abcdef"
  => "abcdefghijklmnopqrstuvwxyz123456789abcdef"
  irb(main):007:0> difearlyaa = "aacdefghijklmnopqrstuvwxyz123456789abcdef"
  => "aacdefghijklmnopqrstuvwxyz123456789abcdef"
  irb(main):008:0> differlate = "abcdefghijklmnopqrstuvwxyz123456789abcdee"
  => "abcdefghijklmnopqrstuvwxyz123456789abcdee"
  irb(main):009:0>  require 'benchmark'
  => true
  irb(main):042:0> n=15000000
  => 15000000
  irb(main):043:0>  Benchmark.bm { |x|
  irb(main):044:1* x.report {  n.times { sameaaaaaa <=> basestring }}
  irb(main):045:1> x.report {  n.times { difearlyaa <=> basestring }}
  irb(main):046:1> x.report {  n.times { differlate <=> basestring }}
  irb(main):047:1> }
        user     system      total        real
    2.650000   0.000000   2.650000 (  2.648783)
    2.500000   0.000000   2.500000 (  2.497226)
    2.630000   0.000000   2.630000 (  2.640446)
  => true
Please sign in to comment.
Something went wrong with that request. Please try again.