[ruby-core:64101] [ruby-trunk - Feature #10098] [Open] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[ruby-core:64101] [ruby-trunk - Feature #10098] [Open] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[arrtchiu]

Issue #10098 has been reported by Matt U.

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098

* Author: Matt U
* Status: Open
* Priority: Normal
* Assignee: 
* Category: ext/openssl
* Target version: next minor
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:64102] [ruby-trunk - Feature #10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[arrtchiu]

Issue #10098 has been updated by Matt U.

File hmac-timing.patch added

Modified misleading error message - new patch attached.

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-48113

* Author: Matt U
* Status: Open
* Priority: Normal
* Assignee: 
* Category: ext/openssl
* Target version: next minor
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:64106] [ruby-trunk - Feature #10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[nobu]

Issue #10098 has been updated by Nobuyoshi Nakada.


* Indent style mismatch
* Should try to convert the argument with `StringValue()`
* Why HMAC only?  Other digests don't need it?
* Probably we should provide timing-safe binary compare function?

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-48117

* Author: Matt U
* Status: Open
* Priority: Normal
* Assignee: 
* Category: ext/openssl
* Target version: next minor
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:64107] [ruby-trunk - Feature #10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[arrtchiu]

Issue #10098 has been updated by Matt U.


Thanks for the feedback!

Nobuyoshi Nakada wrote:
> * Indent style mismatch
> * Should try to convert the argument with `StringValue()`
Will fix - sorry, this is my first contribution!

> * Why HMAC only?  Other digests don't need it?
Good point, I thought since HMAC is for both redundancy and message authentication it made most sense on HMAC rather than all digests - but - I can see there would be use-cases for comparing other digests or other strings in a timing-safe manner.

> * Probably we should provide timing-safe binary compare function?
I think this makes the most sense :)
How about moving this to a method on `String` - e.g. `String#slow_eql?` or `String::timing_safe_eql?`? I'm terrible at naming things :)

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-48118

* Author: Matt U
* Status: Open
* Priority: Normal
* Assignee: 
* Category: ext/openssl
* Target version: next minor
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:64108] [ruby-trunk - Feature #10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[nobu]

Issue #10098 has been updated by Nobuyoshi Nakada.


Matt U wrote:
> How about moving this to a method on `String` - e.g. `String#slow_eql?` or `String::timing_safe_eql?`? I'm terrible at naming things :)

`slow` is not the main concern here, IMHO.
The latter is more descriptive, but seems less pretty a little.

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-48119

* Author: Matt U
* Status: Open
* Priority: Normal
* Assignee: 
* Category: ext/openssl
* Target version: next minor
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:64110] [ruby-trunk - Feature #10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[arrtchiu]

Issue #10098 has been updated by Matt U.

File tsafe_eql.patch added

Nobuyoshi Nakada wrote:
> `slow` is not the main concern here, IMHO.
> The latter is more descriptive, but seems less pretty a little.

Cleaned up (hopefully correctly) and moved to `String#tsafe_eql?`. Any ideas for a better name?



----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-48121

* Author: Matt U
* Status: Open
* Priority: Normal
* Assignee: 
* Category: ext/openssl
* Target version: next minor
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:64112] [ruby-trunk - Feature #10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[nobu]

Issue #10098 has been updated by Nobuyoshi Nakada.


Seems correct.

I made a benchmark for it:

~~~ruby
# bm-tsafe_eql.rb: [Feature #10098]

require 'benchmark'
a = "x"*1024_000
b = a+"y"
c = "y"+a
a << "x"
n = (ARGV.shift || 100000).to_i
Benchmark.bm(15) {|bm|
  bm.report("a==b") {n.times{a==b}}
  bm.report("a==c") {n.times{a==c}}
  bm.report("a.tsafe_eql?(b)") {n.times{a.tsafe_eql?(b)}}
  bm.report("a.tsafe_eql?(c)") {n.times{a.tsafe_eql?(c)}}
}
~~~

It seems quite slow.

               |      user|    system|     total|       real
---------------|----------|----------|----------|-----------
a==b           |  4.660000|  0.000000|  4.660000|(  4.672629)
a==c           |  0.010000|  0.000000|  0.010000|(  0.007154)
a.tsafe_eql?(b)| 34.330000|  0.020000| 34.350000|( 34.366759)
a.tsafe_eql?(c)| 34.450000|  0.020000| 34.470000|( 34.480267)

With the following patch,

               |      user|    system|     total|       real
---------------|----------|----------|----------|-----------
a==b           |  4.660000|  0.000000|  4.660000|(  4.666683)
a==c           |  0.000000|  0.000000|  0.000000|(  0.006657)
a.tsafe_eql?(b)|  7.660000|  0.010000|  7.670000|(  7.662584)
a.tsafe_eql?(c)|  7.660000|  0.000000|  7.660000|(  7.671189)

~~~ruby
diff --git a/string.c b/string.c
index 5c2852f..90865c0 100644
--- a/string.c
+++ b/string.c
@@ -2504,7 +2504,7 @@ static VALUE
 rb_str_tsafe_eql(VALUE str1, VALUE str2)
 {
     long len, idx;
-    char result;
+    VALUE result;
     const char *buf1, *buf2;
 
     str2 = StringValue(str2);
@@ -2516,7 +2516,13 @@ rb_str_tsafe_eql(VALUE str1, VALUE str2)
     buf2 = RSTRING_PTR(str2);
 
     result = 0;
-    for (idx = 0; idx < len; idx++) {
+    idx = 0;
+    if (UNALIGNED_WORD_ACCESS || !((VALUE)buf1 % sizeof(VALUE)) && !((VALUE)buf2 % sizeof(VALUE))) {
+	for (; idx < len; idx += sizeof(VALUE)) {
+	    result |= *(const VALUE *)(buf1+idx) ^ *(const VALUE *)(buf2+idx);
+	}
+    }
+    for (; idx < len; idx++) {
         result |= buf1[idx] ^ buf2[idx];
     }
 
~~~




----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-48123

* Author: Matt U
* Status: Open
* Priority: Normal
* Assignee: 
* Category: ext/openssl
* Target version: next minor
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:64113] [ruby-trunk - Feature #10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[nobu]

Issue #10098 has been updated by Nobuyoshi Nakada.


According to http://www.tedunangst.com/flak/post/notes-on-timingsafe-memcmp,
OpenBSD has `timingsafe_memcmp()`, and NetBSD has `consttime_memequal()`.


----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-48124

* Author: Matt U
* Status: Open
* Priority: Normal
* Assignee: 
* Category: ext/openssl
* Target version: next minor
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:64114] [ruby-trunk - Feature #10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[arrtchiu]

Issue #10098 has been updated by Matt U.


Nobuyoshi Nakada wrote:
> According to [notes on timingsafe_memcmp](http://www.tedunangst.com/flak/post/notes-on-timingsafe-memcmp),
> OpenBSD has [`timingsafe_memcmp()`](http://openbsd.cs.toronto.edu/cgi-bin/cvsweb/src/lib/libc/string/timingsafe_memcmp.c), and NetBSD has [`consttime_memequal()`](http://netbsd.gw.com/cgi-bin/man-cgi?consttime_memequal+3+NetBSD-current).

Wow, thank you for such detailed and valuable feedback (and an awesome patch!)

What do you think about extracting this to an (inline) method like `rb_timingsafe_memcmp(..)` which can then use the system-provided ones if they exist? Since this is moving into distro/platform-specific territory I'm not sure how this fits with Ruby's coding guidelines.

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-48125

* Author: Matt U
* Status: Open
* Priority: Normal
* Assignee: 
* Category: ext/openssl
* Target version: next minor
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:64115] [ruby-trunk - Feature #10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[nobu]

Issue #10098 has been updated by Nobuyoshi Nakada.


Agree to extract a function, I meant it by "provide timing-safe binary compare function".
But the name `*_memcmp` doesn't look nice, as it looks like returning the same result as `memcmp`.
The order on thins comparison won't make sense, so `*_equal` or `*_eql` would be better.

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-48126

* Author: Matt U
* Status: Open
* Priority: Normal
* Assignee: 
* Category: ext/openssl
* Target version: next minor
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:64118] [ruby-trunk - Feature #10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[arrtchiu]

Issue #10098 has been updated by Matt U.

File tsafe_inline.patch added

What's your thoughts on this new patch?

At the moment I'm using OSX and Linux, unable to test `timingsafe_memcmp()` and `consttime_memequal()`.

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-48128

* Author: Matt U
* Status: Open
* Priority: Normal
* Assignee: 
* Category: ext/openssl
* Target version: next minor
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:64120] [ruby-trunk - Feature #10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[nobu]

Issue #10098 has been updated by Nobuyoshi Nakada.


`rb_tsafe_eql()` doesn't need to be `VALUE`, `int` is OK.
Tests for timing-safeness are desirable, but it would be fragile by noise.

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-48129

* Author: Matt U
* Status: Open
* Priority: Normal
* Assignee: 
* Category: ext/openssl
* Target version: next minor
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:64121] [ruby-trunk - Feature #10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[cremno]

Issue #10098 has been updated by cremno phobia.


I don't like the proposed method name.

`tsafe`? It should be `timingsafe`. Saving some keystrokes isn't worth it to have a less obvious name. Even C programmers chose a longer name!

I'm also not happy about `eql?` either. Then people expect it to work like `String#eql?`. Same argument requirements/conversion, same result, and it's even mentioned in the proposed docs (“similarly” is vague)! But it doesn't. For example:

```ruby
a = "\xFF".force_encoding(Encoding::CP437)
b = "\xFF".force_encoding(Encoding::CP850)
p a.eql?(b)  # => false
p a.tsafe_eql?(b)  # => true
```

If something is going to be added to String, then encodings need to be considered. Or:

```ruby
require 'fiddle'
a = "\xFF"
(b = Fiddle::Pointer.malloc(1))[0] = 0xff
p a.eql?(b)  # => false
p a.tsafe_eql?(b)  # => true
```


----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-48130

* Author: Matt U
* Status: Open
* Priority: Normal
* Assignee: 
* Category: ext/openssl
* Target version: next minor
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:64123] [ruby-trunk - Feature #10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[arrtchiu]

Issue #10098 has been updated by Matt U.


Nobuyoshi Nakada wrote:
> `rb_tsafe_eql()` doesn't need to be `VALUE`, `int` is OK.
> Tests for timing-safeness are desirable, but it would be fragile by noise.

I'll get these done. Your benchmark code demonstrated this pretty well so (if it's ok with you) I'll use that as a starting point.

cremno phobia wrote:
> I don't like the proposed method name.
> 
> `tsafe`? It should be `timingsafe`. Saving some keystrokes isn't worth it to have a less obvious name. Even C programmers chose a longer name!
> I'm also not happy about `eql?` either. Then people expect it to work like `String#eql?`. Same argument requirements/conversion, same result, and it's even mentioned in the proposed docs (“similarly” is vague)! But it doesn't. For example:
> ...

Thanks for taking the time to test out this patch!
You have some really good points to consider. My thoughts in response:

- Since Ruby has only the one `String` class for text and data, I think it does make sense to keep this method on the `String` class.
- The behaviour you've demonstrated is intended, we care about the bytes in the buffer; not the encoding.
- Name and documentation is terrible, I agree :)

I think the easiest way to resolve this would be to come up with a better name, and to explain more clearly in the documentation.

For a starting point, how about `timingsafe_bytes_eq?` ? I'll improve the documentation while making the fixes as suggested by Nobu :)

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-48133

* Author: Matt U
* Status: Open
* Priority: Normal
* Assignee: 
* Category: ext/openssl
* Target version: next minor
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:64125] [ruby-trunk - Feature #10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[cremno]

Issue #10098 has been updated by cremno phobia.


Matt U wrote:
> - Since Ruby has only the one `String` class for text and data, I think it does make sense to keep this method on the `String` class.

I agree with you!

> - The behaviour you've demonstrated is intended, we care about the bytes in the buffer; not the encoding.
> - Name and documentation is terrible, I agree :)

Then that encoding will be ignored has to be mentioned in the documentation. What happens if the length of both strings differ, too.

I don't know yet how I feel about (silently) ignoring encoding. Maybe only `ASCII-8BIT` strings should be allowed? But having `bytes` in the name is good anyway! Naming things is hard. I don't have a better idea either (`timingsafe_bytecmp?`, but there's also `casecmp`…).

Are there any gems for this or is such a method part of one?

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-48135

* Author: Matt U
* Status: Open
* Priority: Normal
* Assignee: 
* Category: ext/openssl
* Target version: next minor
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:64508] [ruby-trunk - Feature #10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[arrtchiu]

Issue #10098 has been updated by Matt U.

File 0001-add-timing-safe-string-compare-method.patch added

Changelog:

* Renamed `rb_tsafe_eql` => `rb_consttime_memequal`.
* Renamed `rb_str_tsafe_eql` => `rb_str_consttime_bytes_eq`.
* Renamed `tsafe_eql?` => `consttime_bytes_eq?`.
* `rb_consttime_memequal` now has return type `int`.
* Updated documentation to reflect that encodings are ignored, and removed reference to `eql?`.
* Added tests to ensure timing safety (delta of 0.25 sec allowed to account for GC/system noise).
* Build on Travis passing: https://travis-ci.org/ruby/ruby/builds/33351019

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-48453

* Author: Matt U
* Status: Open
* Priority: Normal
* Assignee: 
* Category: ext/openssl
* Target version: next minor
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:65104] [ruby-trunk - Feature #10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[arrtchiu]

Issue #10098 has been updated by Matt U.


Keen to hear feedback if any. Completely understand there are many more important tickets than this one, but it would be great to see this feature in MRI soon!
 
Devise, one of the most popular frameworks currently implements a timing-safe string compare in Ruby manually: https://github.com/plataformatec/devise/blob/66db52ce31b5d8629f5813a1d7f03a8bc17e5d52/lib/devise.rb#L480-L488

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-48961

* Author: Matt U
* Status: Open
* Priority: Normal
* Assignee: 
* Category: ext/openssl
* Target version: next minor
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:65988] [ruby-trunk - Feature #10098] [Assigned] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[nagachika00]

Issue #10098 has been updated by Tomoyuki Chikanaga.

Category changed from ext/openssl to core
Status changed from Open to Assigned
Assignee set to Yukihiro Matsumoto

The latest patch seems satisfy nobu, doesn't it?
At last we need to get approved from Matz.

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-49720

* Author: Matt U
* Status: Assigned
* Priority: Normal
* Assignee: Yukihiro Matsumoto
* Category: core
* Target version: next minor
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:70792] [Ruby trunk - Feature #10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[zzak]

Issue #10098 has been updated by Zachary Scott.

Assignee changed from Yukihiro Matsumoto to openssl

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-54176

* Author: Matt U
* Status: Assigned
* Priority: Normal
* Assignee: openssl
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:73724] [Ruby trunk Feature#10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[aleksandrs]

Issue #10098 has been updated by Aleksandrs Ļedovskis.


Can someone clarify, what state is this feature in? Do we still need to get Matz's approval of String API change, or in light of Zachary's change "openssl" group gives the final call?




----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-56910

* Author: Matt U
* Status: Assigned
* Priority: Normal
* Assignee: openssl
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:73970] [Ruby trunk Feature#10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[arrtchiu]

Issue #10098 has been updated by Matt U.


Aleksandrs Ļedovskis wrote:
> Can someone clarify, what state is this feature in? Do we still need to get Matz's approval of String API change, or in light of Zachary's change "openssl" group gives the final call?

While still useful with OpenSSL, I'd say that this feature has changed since it was initially reported and no longer relates to OpenSSL. It seems this falls under Ruby's standard String API, which I assume is up to Matz.

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-57124

* Author: Matt U
* Status: Assigned
* Priority: Normal
* Assignee: openssl
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:74087] [Ruby trunk Feature#10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[naruse]

Issue #10098 has been updated by Yui NARUSE.

Assignee changed from openssl to Yukihiro Matsumoto

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-57234

* Author: Matt U
* Status: Assigned
* Priority: Normal
* Assignee: Yukihiro Matsumoto
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:74393] [Ruby trunk Feature#10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[shyouhei]

Issue #10098 has been updated by Shyouhei Urabe.


Anyways, this issue was discussed in this month's developer meeting.  Attendees at there found that OpenSSL already provides a timing-safe binary comparison function in C (namely CRYPTO_memcmp). Just not currently usable from ruby.  Why not export this to ruby-level so that application programmers can use?

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-57513

* Author: Matt U
* Status: Assigned
* Priority: Normal
* Assignee: Yukihiro Matsumoto
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:74968] [Ruby trunk Feature#10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[naruse]

Issue #10098 has been updated by Yui NARUSE.


Following is a patch but I just found there's OPENSSL_memcmp, which is not timing safe...

```diff
diff --git a/ext/openssl/ossl.c b/ext/openssl/ossl.c
index d03dfa7..76333e2 100644
--- a/ext/openssl/ossl.c
+++ b/ext/openssl/ossl.c
@@ -551,6 +551,21 @@ static void Init_ossl_locks(void)
     CRYPTO_set_dynlock_destroy_callback(ossl_dyn_destroy_callback);
 }

+static VALUE
+ossl_memcmp(VALUE dummy, VALUE str1, VALUE str2)
+{
+    const unsigned char *p1 = (const unsigned char *)StringValuePtr(str1);
+    const unsigned char *p2 = (const unsigned char *)StringValuePtr(str2);
+    long len = RSTRING_LEN(str1);
+    long i;
+    unsigned char ret = 0;
+    if (len != RSTRING_LEN(str2)) return Qfalse;
+    for (i=0; i < len; i++) {
+       ret |= p1[i] ^ p2[i];
+    }
+    return ret ? Qfalse : Qtrue;
+}
+
 /*
  * OpenSSL provides SSL, TLS and general purpose cryptography.  It wraps the
  * OpenSSL[http://www.openssl.org/] library.
@@ -1088,6 +1103,7 @@ Init_openssl(void)
      */
     mOSSL = rb_define_module("OpenSSL");
     rb_global_variable(&mOSSL);
+    rb_define_singleton_method(mOSSL, "memcmp", ossl_memcmp, 2);

     /*
      * OpenSSL ruby extension version
```

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-58095

* Author: Matt U
* Status: Assigned
* Priority: Normal
* Assignee: Yukihiro Matsumoto
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:76268] [Ruby trunk Feature#10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[arrtchiu]

Issue #10098 has been updated by Matt U.


Yui, I'm a little confused. The patch you have in your comment looks timing-safe to me. Also I suggest taking a look at Nobu's improvements to my code, I definitely learned a lot more about speed after reading it.

Other things that don't use OpenSSL might benefit from this feature, so my vote is to add this to the String library rather than OpenSSL.

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-59506

* Author: Matt U
* Status: Assigned
* Priority: Normal
* Assignee: Yukihiro Matsumoto
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:77201] [Ruby trunk Feature#10098][Feedback] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[naruse]

Issue #10098 has been updated by Yui NARUSE.

Status changed from Assigned to Feedback

Even though OpenSSL uses the name `memcmp`, I re-considered it is a bad name.
Therefore this is back to naming issue.

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-60418

* Author: Matt U
* Status: Feedback
* Priority: Normal
* Assignee: Yukihiro Matsumoto
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:77423] [Ruby trunk Feature#10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[shyouhei]

Issue #10098 has been updated by Shyouhei Urabe.


(Just to be clear) what is bad about the word "memcmp" is that the "cmp" part implies returning integers, rather than true/false.  One of such example that returns integer is String#casecmp.  But almost nobody wants integers for timing-safe comparisons, meseems at least.  We should provide a method that returns true/false, and in doing so "memcmp" is inappropriate.

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-60680

* Author: Matt U
* Status: Feedback
* Priority: Normal
* Assignee: Yukihiro Matsumoto
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:77426] [Ruby trunk Feature#10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[aleksandrs]

Issue #10098 has been updated by Aleksandrs Ļedovskis.


Ok, here are couple suggestions, to keep the carriage rolling.


~~~
#safe_eql?
#secure_eql?
#timesafe_eql?
#timingsafe_eql?
~~~


----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-60684

* Author: Matt U
* Status: Feedback
* Priority: Normal
* Assignee: Yukihiro Matsumoto
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:77427] [Ruby trunk Feature#10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[zn]

Issue #10098 has been updated by Kazuhiro NISHIYAMA.


Rack and Rails uses `secure_compare`.

- http://www.rubydoc.info/github/rack/rack/Rack/Utils.secure_compare
- http://api.rubyonrails.org/classes/ActiveSupport/SecurityUtils.html#method-c-secure_compare

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-60685

* Author: Matt U
* Status: Feedback
* Priority: Normal
* Assignee: Yukihiro Matsumoto
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:77429] [Ruby trunk Feature#10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[nobu]

Issue #10098 has been updated by Nobuyoshi Nakada.


`secure` sounds vague, and `compare` doesn't address the point by shyouhei, I think.

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-60686

* Author: Matt U
* Status: Feedback
* Priority: Normal
* Assignee: Yukihiro Matsumoto
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:77432] [Ruby trunk Feature#10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[shyouhei]

Issue #10098 has been updated by Shyouhei Urabe.


Are we going to add this method under OpenSSL namespace? If so "secure" describes almost nothing.

My suggestion is to add `String#casecmp?` (proposed in #12786) first, then introduce this one with `OpenSSL.memcmp?` naming.

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-60688

* Author: Matt U
* Status: Feedback
* Priority: Normal
* Assignee: Yukihiro Matsumoto
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:87505] [Ruby trunk Feature#10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[shyouhei]

Issue #10098 has been updated by shyouhei (Shyouhei Urabe).


`String#casecmp?` has already landed.  It seems no blocker are there for implementing this one, except the name.

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-72510

* Author: arrtchiu (Matt U)
* Status: Feedback
* Priority: Normal
* Assignee: matz (Yukihiro Matsumoto)
* Target version: 
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:87633] [Ruby trunk Feature#10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[bartdewater]

Issue #10098 has been updated by bdewater (Bart de Water).


I agree Rails' `secure_compare` name is not great. 'Secure' is a result of the fact it's a constant-time comparison. How about `String#const_time_eq` or something along those lines?

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-72654

* Author: arrtchiu (Matt U)
* Status: Feedback
* Priority: Normal
* Assignee: matz (Yukihiro Matsumoto)
* Target version: 
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:91392] [Ruby trunk Feature#10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[shevegen]

Issue #10098 has been updated by shevegen (Robert A. Heiler).


I think String#const_time_eql? is not an ideal name either.

A problem with "secure" is that it can mean different things in different contexts; on class String this
may be a bit more difficult since Strings in ruby can be so general purpose. Perhaps the OpenSSL
namespace could have somewhat more freedom to also accept names that are not absolutely
perfect? Just as comparison - although I don't think #const_time_eql? is a very good name, I think
it would fit a lot better into OpenSSL than it may fit towards class String as such.

> 'Secure' is a result of the fact it's a constant-time comparison. 

Now I think that will surprise some people since they may wonder what secure has to do with any
constant-time comparison per se. :)

(By the way, I think the word "compare", also as method name, is easier to reason for than e. g.
"secure"; what would a "secure string" mean, for example? Would that be different or the same
to a "tainted" string or is it a separate aspect? But that is just a side comment, I think Bart would
like to see any forward decision towards the issue at hand considering he added it to the next
upcoming developer meeting.)

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-76649

* Author: arrtchiu (Matt U)
* Status: Feedback
* Priority: Normal
* Assignee: matz (Yukihiro Matsumoto)
* Target version: 
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:91459] [Ruby trunk Feature#10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[naruse]

Issue #10098 has been updated by naruse (Yui NARUSE).

Assignee changed from matz (Yukihiro Matsumoto) to openssl
Status changed from Feedback to Assigned

This feature is now considered up to @rhenium.

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-76720

* Author: arrtchiu (Matt U)
* Status: Assigned
* Priority: Normal
* Assignee: openssl
* Target version: 
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:94409] [Ruby master Feature#10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[bartdewater]

Issue #10098 has been updated by bdewater (Bart de Water).


New patch over at https://github.com/ruby/openssl/pull/269

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-80831

* Author: arrtchiu (Matt U)
* Status: Assigned
* Priority: Normal
* Assignee: openssl
* Target version: 
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:95440] [Ruby master Feature#10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[bartdewater]

Issue #10098 has been updated by bdewater (Bart de Water).


The above PR has been merged into the OpenSSL gem 🎉

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-82192

* Author: arrtchiu (Matt U)
* Status: Assigned
* Priority: Normal
* Assignee: openssl
* Target version: 
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:95447] [Ruby master Feature#10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

[shyouhei]

Issue #10098 has been updated by shyouhei (Shyouhei Urabe).

Status changed from Assigned to Closed

Implemented in upstream.  Closing.

----------------------------------------
Feature #10098: [PATCH] Timing-safe string comparison for OpenSSL::HMAC
https://bugs.ruby-lang.org/issues/10098#change-82198

* Author: arrtchiu (Matt U)
* Status: Closed
* Priority: Normal
* Assignee: openssl
* Target version: 
----------------------------------------
I could be totally wrong, but it seems the standard library doesn't provide a reliable way of comparing hashes in constant-time.

* The docs for `OpenSSL::HMAC` encourage the use of `Digest#to_s` (see: http://ruby-doc.org/stdlib-2.1.0/libdoc/openssl/rdoc/OpenSSL/HMAC.html#method-c-new )
* Ruby's string comparison uses memcmp, which isn't timing safe (see: http://rxr.whitequark.org/mri/source/string.c#2382 )

With this patch I propose to add an additional method, `OpenSSL::HMAC#verify`, which takes a binary string with a digest and compares it against the computed hash.


---Files--------------------------------
hmac-timing.patch (2.5 KB)
hmac-timing.patch (2.48 KB)
tsafe_eql.patch (2.48 KB)
tsafe_inline.patch (3.51 KB)
0001-add-timing-safe-string-compare-method.patch (4.31 KB)


-- 
https://bugs.ruby-lang.org/