From: arrtchiu@...
Date: 2014-07-30T02:07:12+00:00
Subject: [ruby-core:64123] [ruby-trunk - Feature #10098] [PATCH] Timing-safe string comparison for OpenSSL::HMAC

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/