Permalink
Browse files

1.9 compatible secure_compare

  • Loading branch information...
NZKoz committed Sep 8, 2009
1 parent 896475c commit 26306f9292f59a6b7fbacbb4369583944e36701d
Showing with 28 additions and 9 deletions.
  1. +28 −9 actionpack/lib/action_controller/session/cookie_store.rb
@@ -165,16 +165,35 @@ def clear_old_cookie_value
@session.cgi.cookies[@cookie_options['name']].clear
end
- # 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]
+ if "foo".respond_to?(:force_encoding)
+ # constant-time comparison algorithm to prevent timing attacks
+ def secure_compare(a, b)
+ a = a.force_encoding(Encoding::BINARY)

This comment has been minimized.

Show comment Hide comment
@ncr

ncr Sep 10, 2009

Contributor

Shouldn't it .dup args before .force_encoding?

@ncr

ncr Sep 10, 2009

Contributor

Shouldn't it .dup args before .force_encoding?

+ b = b.force_encoding(Encoding::BINARY)
+
+ if a.length == b.length
+ result = 0
+ for i in 0..(a.length - 1)
+ result |= a[i].ord ^ b[i].ord
+ end
+ result == 0
+ else
+ false
+ end
+ end
+ else
+ # For 1.8
+ 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
- result == 0
- else
- false
end
end
+
end

7 comments on commit 26306f9

@qoobaa

This comment has been minimized.

Show comment Hide comment
@qoobaa

qoobaa Sep 9, 2009

Contributor

Why do you modify method arguments? What if somebody wants to use the string in some other places?

Contributor

qoobaa replied Sep 9, 2009

Why do you modify method arguments? What if somebody wants to use the string in some other places?

@NZKoz

This comment has been minimized.

Show comment Hide comment
@NZKoz

NZKoz Sep 10, 2009

Member

Sorry if I'm a bit dense, but ... what modification?

Member

NZKoz replied Sep 10, 2009

Sorry if I'm a bit dense, but ... what modification?

@qoobaa

This comment has been minimized.

Show comment Hide comment
@qoobaa

qoobaa Sep 10, 2009

Contributor

a = "123"
b = "123"
a.encoding
#=> #Encoding:UTF-8
b.encoding
#=> #Encoding:UTF-8
secure_compare(a, b)
#=> true
a.encoding
#=> #Encoding:ASCII-8BIT
b.encoding
#=> #Encoding:ASCII-8BIT

I don't think that enforcing encoding is the best way to solve problems like this. If you want to access bytes, use "each_byte" or "bytes" as god intended.

Contributor

qoobaa replied Sep 10, 2009

a = "123"
b = "123"
a.encoding
#=> #Encoding:UTF-8
b.encoding
#=> #Encoding:UTF-8
secure_compare(a, b)
#=> true
a.encoding
#=> #Encoding:ASCII-8BIT
b.encoding
#=> #Encoding:ASCII-8BIT

I don't think that enforcing encoding is the best way to solve problems like this. If you want to access bytes, use "each_byte" or "bytes" as god intended.

@qoobaa

This comment has been minimized.

Show comment Hide comment
@qoobaa

qoobaa Sep 10, 2009

Contributor

Once again:

a = "123"
b = "123"
a.encoding
#=> #<Encoding:UTF-8>
b.encoding #=>
#<Encoding:UTF-8>
secure_compare(a, b)
#=> true
a.encoding
#=> #<Encoding:ASCII-8BIT>
b.encoding
#=> #<Encoding:ASCII-8BIT>
Contributor

qoobaa replied Sep 10, 2009

Once again:

a = "123"
b = "123"
a.encoding
#=> #<Encoding:UTF-8>
b.encoding #=>
#<Encoding:UTF-8>
secure_compare(a, b)
#=> true
a.encoding
#=> #<Encoding:ASCII-8BIT>
b.encoding
#=> #<Encoding:ASCII-8BIT>
@qoobaa

This comment has been minimized.

Show comment Hide comment
@qoobaa

qoobaa Sep 10, 2009

Contributor

force_encoding is not a good solution, it causes problems like this. I think force_encoding should be used only when nothing else works. Could you please just take a look at my patch submitted on lighthouse: https://rails.lighthouseapp.com/projects/8994/tickets/3144/a/261015/0001-ruby-1.9-friendly-secure_compare.patch .

Contributor

qoobaa replied Sep 10, 2009

force_encoding is not a good solution, it causes problems like this. I think force_encoding should be used only when nothing else works. Could you please just take a look at my patch submitted on lighthouse: https://rails.lighthouseapp.com/projects/8994/tickets/3144/a/261015/0001-ruby-1.9-friendly-secure_compare.patch .

@yaroslav

This comment has been minimized.

Show comment Hide comment
@yaroslav

yaroslav Sep 10, 2009

Contributor

+1 ncr

Contributor

yaroslav replied Sep 10, 2009

+1 ncr

@NZKoz

This comment has been minimized.

Show comment Hide comment
@NZKoz

NZKoz Sep 11, 2009

Member

@qoobaa: Thanks for that, can you rebase your patch against current master so I can go about backporting it as needed? Looks good to go.

Member

NZKoz replied Sep 11, 2009

@qoobaa: Thanks for that, can you rebase your patch against current master so I can go about backporting it as needed? Looks good to go.

Please sign in to comment.