Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  qoobaa September 4th, 2009 @ 08:55 AM

  • Tag changed from “activesupport” to “1.9, 1.9.1, 2.3.4, activesupport, patch”

  The patch applies cleanly on the version from repository (tag v2.3.4), but there are some problems with applying it on the gem. The secure_compare now uses bytesize and each_byte methods when available (Ruby 1.8.7, 1.9.x, ...).

  • 0001-ruby-1.9-friendly-secure_compare.patch 1.5 KB

• You flagged this item as spam.
  

  qoobaa September 4th, 2009 @ 09:05 AM

  Checked again and it seems that everything works on gem version as well.

• You flagged this item as spam.
  

  hukl September 4th, 2009 @ 11:29 AM

  I just ran into the same problem on ruby 1.9 and fixed it quite differently. I'm not sure why you chose to make it so complicated to compare those two strings.

  I would be highly interested to hear what you think about my version!

  In the end you could just compare the strings in ruby versions below 1.8.7 (maybe even below 1.9). In 1.9 however it should be sufficient to compare the byte arrays as that would make sure it only returns true if they are of the same length, the same content and the same order.

  • 0001-secure_compare-would-not-work-on-ruby-1.9-because-a-.patch 1.2 KB
• 

  CancelProfileIsBroken September 4th, 2009 @ 12:22 PM

  hukl - Isn't the simple "compare" approach vulnerable to the timing attack that http://github.com/rails/rails/commit/1f07a89c5946910fc28ea5ccd1da6a... was committed to address in the first place?

• 

  CancelProfileIsBroken September 4th, 2009 @ 12:23 PM

  • Assigned user set to “Michael Koziarski”

• You flagged this item as spam.
  

  qoobaa September 4th, 2009 @ 12:53 PM

  irb(main):011:0> puts Benchmark.measure { ([0] + [1] * 100_000_000) == ([1] + [1] * 100_000_000) }
   14.350000   1.590000  15.940000 ( 17.394467)
  => nil
  irb(main):012:0> puts Benchmark.measure { ([1] * 100_000_000 + [0]) == ([1] * 100_000_000 + [1]) }
   16.430000   1.900000  18.330000 ( 21.816415)
  => nil
  

  Huki - your patch is also vulnerable to timing attack. The main difference between your code and the code from Rails is that RoR always checks every single byte, even if the first byte is different.

  Method should use bytesize instead of size (size depends on the string encoding, it may cause some problems).

• You flagged this item as spam.
  

  hukl September 4th, 2009 @ 02:11 PM

  hrm okay, i don't understand that timing issue yet. The main concern is to fix it for ruby 1.9 without breaking this timing issue again. I'd be still interested in a detailed explanation for this issue.

• 

  CancelProfileIsBroken September 4th, 2009 @ 02:20 PM

  See http://codahale.com/a-lesson-in-timing-attacks/ - an article from the guy who reported the issue in Rails. It's a good introduction to the subject.

• You flagged this item as spam.
  

  qoobaa September 4th, 2009 @ 02:23 PM

  Huki: basically, comparing "abbb" and "bbbb" is faster than comparing "bbba" and "bbbb". It means, that if you have some good way to measure the time taken by the comparison, you know how many bytes of your digest (of some password, signature, etc.) are correct. Theoretically you can guess the digest, measuring the time taken by processing request. http://en.wikipedia.org/wiki/Timing_attack

• You flagged this item as spam.
  

  hukl September 4th, 2009 @ 03:02 PM

  Okay I see! ;) Too bad!

  And iterating over it is the only way? Can't you still do something like this:

  result = digest_a.bytes.to_a | digest_b.bytes.to_a

  result.length == digest_a.bytes.to_a.length

  This would also compare the whole array. Well - I guess I can live with the iterating version - just looks so massive for an operation that should be so small ;)

  Thank you for clueing me in ;)

• You flagged this item as spam.
  

  travis (at appoxy) September 4th, 2009 @ 08:47 PM

  This is a pretty severe bug that's been introduced going from 2.3.3 to 2.3.4. It means that anyone who's visited the site won't be able to access it anymore and will get this error message.

  This should be rescued and handled WAY more elegantly, perhaps just trashing the session and starting fresh or something.

• You flagged this item as spam.
  

  travis (at appoxy) September 4th, 2009 @ 08:52 PM

  Actually, this even happens after clearing cookies. I could get to the login page, but then after logging in and going to the next page, same error. Rolling back to 2.3.3.

• You flagged this item as spam.
  

  Adam S September 9th, 2009 @ 06:11 AM

  I know it's only been a couple of days, but can we get this fixed soon?

  I too can't move to 2.3.4 because of this bug.

• You flagged this item as spam.
  

  Adam S September 9th, 2009 @ 07:19 AM

  +1 patch applied cleanly.

• You flagged this item as spam.
  

  Priit Tamboom September 9th, 2009 @ 07:22 AM

  @Adam S: This bug is fixed at github's master branch. Temporarily you can copy the fixed file to your local copy or build gem from source etc.

• You flagged this item as spam.
  

  travis (at appoxy) September 9th, 2009 @ 07:31 AM

  When do you think 2.3.5 will come out with this patch?

• You flagged this item as spam.
  

  Priit Tamboom September 9th, 2009 @ 07:39 AM

  It's out of my scope, but I would suggest to release 2.3.4.1 instead.

• You flagged this item as spam.
  

  Nicolas Blanco September 9th, 2009 @ 09:35 AM

  +1 for 2.3.4.1 !

• You flagged this item as spam.
  

  Berin Loritsch September 9th, 2009 @ 08:56 PM

  +1 for 2.3.4.1

  The core difference is in the return type from Ruby 1.8 hexdigest and Ruby 1.9 hexdigest. Ruby 1.8 returns an array of bytes and Ruby 1.9 returns a string. By forcing generate_digest to always return a string, it makes it easy to compare. Alternatively, it can be done by making generate_digest always return an array of bytes. I fixed it manually by applying the following code:

    def secure_compare(a, b)
      return a == b
    end
  
    def generate_digest(data)
      require 'openssl' unless defined?(OpenSSL)
      OpenSSL::HMAC.hexdigest(OpenSSL::Digest::Digest.new(@digest), @secret, data).to_s
    end
  

• You flagged this item as spam.
  

  bloritsch September 10th, 2009 @ 04:32 PM

  OK, after reading up on the timing attack, and found out that Ruby 1.9 likes to automatically convert bytes to characters, I found an approach that will always work. Not to mention, fix a particular bug in the implementation that violated the reason for the change to begin with:

    def secure_compare(a, b)
      result = a.length ^ b.length
      for i in 0..([a.length, b.length].max - 1)
        result |= a[i].to_i ^ b[i].to_i
      end
      result == 0
    end
  

  The previous version would end early when the two digests were not identical. With this information, an attacker can at least guess the size of the key. This simpler version of the secure_compare() method takes advantage of a few things:

  if you ask for a value outside the range of an array you will get a nil:

  a = [10,20,101]
  val = a[5]
  val.nil? == true

  When you convert nil.to_i you get the number zero (0).

  This allows the loop to force every element of both arrays to be a number, whether it was a string (ruby 1.9) or a nil (arrays are different sizes).

  The only way to make this more cryptographically secure from the sideband timing attack is to identify one of the arguments as the secret and the other argument as the item being compared to the secret. You would always use the length of the item being compared to the secret so as not to reveal the length of the secret. Using an empty array for the secret is protected from by including the length of the secret compared to the length of the array being compared. This requires that the method be used consistently--which would suggest the need better argument names.

• You flagged this item as spam.
  

  Adam S September 11th, 2009 @ 02:56 AM

  I think the suggestion "always use the length of the item being compared to the secret so as not to reveal the length of the secret" is a great idea. This means that small attacks can still return quickly, and the attacker basically has no way of guessing anything.

• 

  Michael Koziarski September 11th, 2009 @ 04:31 AM

  Jakub's original patch looks good, can you rebase it so it applies against master and 2-3-stable please.

  We'll push a 2.3.5 once we know this is fixed properly on 1.9

• You flagged this item as spam.
  

  qoobaa September 11th, 2009 @ 08:20 AM

  Rebased to master and 2-3-stable.

  • 0001-ruby-1.9-friendly-secure_compare.patch 1.5 KB

• You flagged this item as spam.
  

  bloritsch September 11th, 2009 @ 01:39 PM

  I just thought of something last night. Using the length of the requester provided HMAC to control the loop opens the door for a possible Denial of Service attack. If the client provides a very long key it will negatively affect the response of the application. So even better than using the length of the master key or the challenge key, use a constant length that is longer than both yet still reasonable. The method would then look like this:

  def secure_compare(a, b)

  result = a.length ^ b.length
  for i in 0..99 # or some other constant
    result |= a[i].to_i ^ b[i].to_i
  end
  result == 0
  

  end

  Which satisfies both 1.8.6 and 1.9.1 requirements without the complex logic of the last friendly-secure-compare.patch.

• You flagged this item as spam.
  

  bloritsch September 11th, 2009 @ 01:40 PM

    def secure_compare(a, b)
      result = a.length ^ b.length
      for i in 0..([a.length, b.length].max - 1)
        result |= a[i].to_i ^ b[i].to_i
      end
      result == 0
    end
  

• You flagged this item as spam.
  

  bloritsch September 11th, 2009 @ 01:41 PM

    def secure_compare(a, b)
      result = a.length ^ b.length
      for i in 0..99 # or some other constant
        result |= a[i].to_i ^ b[i].to_i
      end
      result == 0
    end
  

  Is there some way to clean up the mistakes? I can't delete the erroneous comment from before, or edit the formatting for one I made earlier.

• 

  Michael Koziarski September 12th, 2009 @ 01:52 AM

  • State changed from “new” to “resolved”

  Applied jakub's patches so I'm wrapping this one up.

  Thanks for the feedback from everyone but I'm happy that this one is good enough now.

• You flagged this item as spam.
  

  Joe Hannon September 21st, 2009 @ 02:20 AM

  • Assigned user cleared.

  Google brought me to this page. I guess I need this fix. I'm an idiot, and I can't figure out how to apply the patch. Can someone help me?

  I've tried

  $ sudo patch /usr/local/lib/ruby/gems/1.9.1/gems/activesupport-2.3.3/lib/active_support/message_verifier.rb ~/Dev/0001-ruby-1.9-friendly-secure_compare.patch

  and also with --force, neither works. Hunk #1 FAILED at 40. Reading the patch man-page and further googling hasn't helped me figure out how to get it patched. Can someone give me a pointer?

  Thanks

• You flagged this item as spam.
  

  Rob Sanheim September 24th, 2009 @ 08:52 AM

  Is this going to be pushed in a new patch level soon?

• You flagged this item as spam.
  

  alberto September 29th, 2009 @ 06:55 PM

  Joe:

  I ran into the same problem. You need to use the original patch file at the beginning of the thread if you are patching rails 2.3.4, that is what worked for me.

• You flagged this item as spam.
  

  The Doctor What November 4th, 2009 @ 07:01 PM

  Just curious. Is there a reason not to just change:
  a[i] ^ b[i]
  to:
  a[i].ord ^ b[i].ord

• You flagged this item as spam.
  

  Julian Burgess November 4th, 2009 @ 08:34 PM

  I don't wish to sound rude, but how did this get through testing? It seems there must have been a gap in the process to let a bug like this through? Has something been done to make sure that future releases will be fully tested in Ruby 1.9.1 before being published?

• You flagged this item as spam.
  

  The Doctor What November 4th, 2009 @ 08:37 PM

  BTW: Was a unittest added when this was fixed?

• You flagged this item as spam.
  

  csnk May 18th, 2011 @ 08:29 AM

  We are the professional clothing manufacturer and clothing supplier, so we manufacture kinds of custom clothing manufacturer. welcome you to come to our china clothing manufacturer and clothing factory.