This project is archived and is in readonly mode.

#3144 ✓ resolved
Wojciech Wnętrzak

undefined method `^' for String - RoR 2.3.4

Reported by Wojciech Wnętrzak | September 4th, 2009 @ 08:42 AM

In active_support/mesage_verifier.rb method secure_compare is trying to make exclusive or a[i] ^ b[i], where a and b are strings.
In Ruby 1.9 string[index] returns character not code of the character (like it was in 1.8).

Comments and changes to this ticket

  • qoobaa

    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, ...).

  • qoobaa

    qoobaa September 4th, 2009 @ 09:05 AM

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

  • hukl

    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.

  • CancelProfileIsBroken

    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

    CancelProfileIsBroken September 4th, 2009 @ 12:23 PM

    • Assigned user set to “Michael Koziarski”
  • qoobaa

    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).

  • hukl

    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

    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.

  • qoobaa

    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

  • hukl

    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 ;)

  • travis (at appoxy)

    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.

  • travis (at appoxy)

    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.

  • Adam S

    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.

  • Adam S

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

    +1 patch applied cleanly.

  • Priit Tamboom

    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.

  • travis (at appoxy)

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

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

  • Priit Tamboom

    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.

  • Nicolas Blanco
  • Berin Loritsch

    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
    
  • bloritsch

    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.

  • Adam S

    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

    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

  • qoobaa
  • bloritsch

    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.

  • bloritsch

    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
    
  • bloritsch

    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

    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.

  • Joe Hannon

    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

  • Rob Sanheim

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

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

  • alberto

    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.

  • The Doctor What

    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

  • Julian Burgess

    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?

  • The Doctor What

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

    BTW: Was a unittest added when this was fixed?

  • benben
  • csnk

    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.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Tickets have moved to Github

The new ticket tracker is available at https://github.com/rails/rails/issues

Shared Ticket Bins

Referenced by

Pages