https://bugs.ruby-lang.org/https://bugs.ruby-lang.org/favicon.ico?17113305112017-06-27T06:47:51ZRuby Issue Tracking SystemRuby master - Feature #13681: Ruby digest init fails in FIPS mode when built against OpenSSL ~> 1.0.1https://bugs.ruby-lang.org/issues/13681?journal_id=654772017-06-27T06:47:51Zrhenium (Kazuki Yamaguchi)k@rhe.jp
<ul></ul><blockquote>
<pre><code class="diff syntaxhl" data-language="diff"><span class="gh">diff -Nurp old/ext/digest/digest.h new/ext/digest/digest.h
</span><span class="gd">--- old/ext/digest/digest.h 2017-06-21 12:56:09.007011362 -0400
</span><span class="gi">+++ new/ext/digest/digest.h 2017-06-21 12:56:32.458975554 -0400
</span><span class="p">@@ -31,6 +31,26 @@</span> typedef struct {
rb_digest_hash_finish_func_t finish_func;
} rb_digest_metadata_t;
<span class="gi">+#define DEFINE_INIT_FUNC_FOR_EVP(upper_name, lower_name) \
+int \
+rb_digest_##upper_name##_evp_init(upper_name##_CTX* ctx) \
+{ \
+ SSL_load_error_strings(); \
+ EVP_MD_CTX *md_ctx = EVP_MD_CTX_create(); \
+ \
+ if(!EVP_DigestInit_ex(md_ctx, EVP_##lower_name(), NULL)) { \
+ const char *error_message; \
+ EVP_MD_CTX_destroy(md_ctx); \
+ \
+ error_message = ERR_reason_error_string(ERR_peek_last_error()); \
</span></code></pre>
</blockquote>
<p>The OpenSSL error queue must be cleared by <code>ERR_clear_error()</code>.</p>
<blockquote>
<pre><code class="diff syntaxhl" data-language="diff"><span class="gi">+ rb_raise(rb_eRuntimeError, error_message); \
+ } \
+ *ctx = *(upper_name##_CTX*)md_ctx->md_data; \
</span></code></pre>
</blockquote>
<p>This won't compile with OpenSSL 1.1.x since <code>EVP_MD_CTX</code> was made opaque.</p>
<p>Also I suspect this approach breaks if an external OpenSSL engine<br>
replaces the default implementation for the algorithm. I think we have<br>
to completely rewrite to use the EVP API only.</p>
<blockquote>
<pre><code class="diff syntaxhl" data-language="diff"><span class="gi">+ EVP_MD_CTX_destroy(md_ctx); \
+ \
+ return 1; \
+}
+
</span> #define DEFINE_UPDATE_FUNC_FOR_UINT(name) \
void \
rb_digest_##name##_update(void *ctx, unsigned char *ptr, size_t size) \
</code></pre>
</blockquote>
<p>Another idea: I wonder if it would be possible or desirable to rip out<br>
the OpenSSL backend entirely instead. There would be some performance<br>
degradation, though, people could switch to OpenSSL::Digest of the<br>
'openssl' extension if they really care about. It uses the EVP API and<br>
works as a drop-in replacement (in fact it inherits from <code>Digest::Base</code>).</p> Ruby master - Feature #13681: Ruby digest init fails in FIPS mode when built against OpenSSL ~> 1.0.1https://bugs.ruby-lang.org/issues/13681?journal_id=654792017-06-27T07:15:52Znobu (Nobuyoshi Nakada)nobu@ruby-lang.org
<ul></ul><p>rhenium (Kazuki Yamaguchi) wrote:</p>
<blockquote>
<blockquote>
<pre><code class="diff syntaxhl" data-language="diff"><span class="gi">+ error_message = ERR_reason_error_string(ERR_peek_last_error()); \
+ rb_raise(rb_eRuntimeError, error_message); \
</span></code></pre>
</blockquote>
</blockquote>
<p>Just a note, this causes -Wformat-security "format string is not a string literal (potentially insecure)" warnings.<br>
And probably <code>EVP_MD_CTX_</code>* functions need to be checked in <code>ext/digest/digest_conf.rb</code>.</p> Ruby master - Feature #13681: Ruby digest init fails in FIPS mode when built against OpenSSL ~> 1.0.1https://bugs.ruby-lang.org/issues/13681?journal_id=654802017-06-27T08:37:40Zrinzler (Colton Jenkins)
<ul></ul><p>rhenium (Kazuki Yamaguchi) wrote:</p>
<blockquote>
<blockquote>
<pre><code class="diff syntaxhl" data-language="diff"><span class="gh">diff -Nurp old/ext/digest/digest.h new/ext/digest/digest.h
</span><span class="gd">--- old/ext/digest/digest.h 2017-06-21 12:56:09.007011362 -0400
</span><span class="gi">+++ new/ext/digest/digest.h 2017-06-21 12:56:32.458975554 -0400
</span><span class="p">@@ -31,6 +31,26 @@</span> typedef struct {
rb_digest_hash_finish_func_t finish_func;
} rb_digest_metadata_t;
<span class="gi">+#define DEFINE_INIT_FUNC_FOR_EVP(upper_name, lower_name) \
+int \
+rb_digest_##upper_name##_evp_init(upper_name##_CTX* ctx) \
+{ \
+ SSL_load_error_strings(); \
+ EVP_MD_CTX *md_ctx = EVP_MD_CTX_create(); \
+ \
+ if(!EVP_DigestInit_ex(md_ctx, EVP_##lower_name(), NULL)) { \
+ const char *error_message; \
+ EVP_MD_CTX_destroy(md_ctx); \
+ \
+ error_message = ERR_reason_error_string(ERR_peek_last_error()); \
</span></code></pre>
</blockquote>
<p>The OpenSSL error queue must be cleared by <code>ERR_clear_error()</code>.</p>
</blockquote>
<p>Ah, good catch.</p>
<blockquote>
<blockquote>
<pre><code class="diff syntaxhl" data-language="diff"><span class="gi">+ rb_raise(rb_eRuntimeError, error_message); \
+ } \
+ *ctx = *(upper_name##_CTX*)md_ctx->md_data; \
</span></code></pre>
</blockquote>
<p>This won't compile with OpenSSL 1.1.x since <code>EVP_MD_CTX</code> was made opaque.</p>
</blockquote>
<p>Hasn't it always been opaque? They just moved the struct def from evp.h -> evp_locl.h, but the typedef is still in ossl_typ.h</p>
<blockquote>
<p>Also I suspect this approach breaks if an external OpenSSL engine<br>
replaces the default implementation for the algorithm. I think we have<br>
to completely rewrite to use the EVP API only.</p>
</blockquote>
<p>Not sure I follow. Passing NULL for engine skips it? But using EVP api would be ideal.</p>
<blockquote>
<blockquote>
<pre><code class="diff syntaxhl" data-language="diff"><span class="gi">+ EVP_MD_CTX_destroy(md_ctx); \
+ \
+ return 1; \
+}
+
</span> #define DEFINE_UPDATE_FUNC_FOR_UINT(name) \
void \
rb_digest_##name##_update(void *ctx, unsigned char *ptr, size_t size) \
</code></pre>
</blockquote>
<p>Another idea: I wonder if it would be possible or desirable to rip out<br>
the OpenSSL backend entirely instead. There would be some performance<br>
degradation, though, people could switch to OpenSSL::Digest of the<br>
'openssl' extension if they really care about. It uses the EVP API and<br>
works as a drop-in replacement (in fact it inherits from <code>Digest::Base</code>).</p>
</blockquote>
<p>That's an option. OpenSSL 1.1.x 'currently' doesn't support FIPS so it would<br>
be up to the app devs (me) to ensure all code doesn't use non fips compliant algos (ouch).<br>
From what I've seen the majority of gems use digest over openssl::digest<br>
given it isn't guaranteed the system it deploys too will have it.<br>
What is nice about it's current implementation is if the system does have OpenSSL<br>
with fips enabled then digest will obtain that functionality with this.<br>
I doubt many rubyist have to deal with FIPS, but this does make life a bit easier.<br>
<a href="https://github.com/bundler/bundler/issues/4989" class="external">https://github.com/bundler/bundler/issues/4989</a></p> Ruby master - Feature #13681: Ruby digest init fails in FIPS mode when built against OpenSSL ~> 1.0.1https://bugs.ruby-lang.org/issues/13681?journal_id=654812017-06-27T08:38:22Zrinzler (Colton Jenkins)
<ul></ul><p>nobu (Nobuyoshi Nakada) wrote:</p>
<blockquote>
<p>rhenium (Kazuki Yamaguchi) wrote:</p>
<blockquote>
<blockquote>
<pre><code class="diff syntaxhl" data-language="diff"><span class="gi">+ error_message = ERR_reason_error_string(ERR_peek_last_error()); \
+ rb_raise(rb_eRuntimeError, error_message); \
</span></code></pre>
</blockquote>
</blockquote>
<p>Just a note, this causes -Wformat-security "format string is not a string literal (potentially insecure)" warnings.<br>
And probably <code>EVP_MD_CTX_</code>* functions need to be checked in <code>ext/digest/digest_conf.rb</code>.</p>
</blockquote>
<p>K, I'll check that out. Haven't coded in C in quite some time.<br>
Will do.</p> Ruby master - Feature #13681: Ruby digest init fails in FIPS mode when built against OpenSSL ~> 1.0.1https://bugs.ruby-lang.org/issues/13681?journal_id=654832017-06-27T09:26:51Zrhenium (Kazuki Yamaguchi)k@rhe.jp
<ul></ul><p>rinzler (Colton Jenkins) wrote:</p>
<blockquote>
<blockquote>
<blockquote>
<pre><code class="diff syntaxhl" data-language="diff"><span class="gi">+ rb_raise(rb_eRuntimeError, error_message); \
+ } \
+ *ctx = *(upper_name##_CTX*)md_ctx->md_data; \
</span></code></pre>
</blockquote>
<p>This won't compile with OpenSSL 1.1.x since <code>EVP_MD_CTX</code> was made opaque.</p>
</blockquote>
<p>Hasn't it always been opaque? They just moved the struct def from evp.h -> evp_locl.h, but the typedef is still in ossl_typ.h</p>
</blockquote>
<p>crypto/evp/evp_locl.h is not a public header file. -> operator can't be<br>
used against md_ctx.</p>
<blockquote>
<blockquote>
<p>Also I suspect this approach breaks if an external OpenSSL engine<br>
replaces the default implementation for the algorithm. I think we have<br>
to completely rewrite to use the EVP API only.</p>
</blockquote>
<p>Not sure I follow. Passing NULL for engine skips it? But using EVP api would be ideal.</p>
</blockquote>
<p>Passing NULL as the third argument tells OpenSSL to use the 'default<br>
implementation', which can be changed at runtime by an<br>
ENGINE_set_default*() function call by another C extension (such as<br>
openssl).</p> Ruby master - Feature #13681: Ruby digest init fails in FIPS mode when built against OpenSSL ~> 1.0.1https://bugs.ruby-lang.org/issues/13681?journal_id=655052017-06-28T14:35:09Zrinzler (Colton Jenkins)
<ul></ul><p>rhenium (Kazuki Yamaguchi) wrote:</p>
<blockquote>
<p>rinzler (Colton Jenkins) wrote:</p>
<blockquote>
<blockquote>
<blockquote>
<pre><code class="diff syntaxhl" data-language="diff"><span class="gi">+ rb_raise(rb_eRuntimeError, error_message); \
+ } \
+ *ctx = *(upper_name##_CTX*)md_ctx->md_data; \
</span></code></pre>
</blockquote>
<p>This won't compile with OpenSSL 1.1.x since <code>EVP_MD_CTX</code> was made opaque.</p>
</blockquote>
<p>Hasn't it always been opaque? They just moved the struct def from evp.h -> evp_locl.h, but the typedef is still in ossl_typ.h</p>
</blockquote>
<p>crypto/evp/evp_locl.h is not a public header file. -> operator can't be<br>
used against md_ctx.</p>
</blockquote>
<p>Ah ok, do you know what will happen to openssl::digest then given it uses the same? Curious if that plans to be refactored upon 1.1.x I could do the same with this. If not then this doesn't make much sense.</p>
<blockquote>
<blockquote>
<blockquote>
<p>Also I suspect this approach breaks if an external OpenSSL engine<br>
replaces the default implementation for the algorithm. I think we have<br>
to completely rewrite to use the EVP API only.</p>
</blockquote>
<p>Not sure I follow. Passing NULL for engine skips it? But using EVP api would be ideal.</p>
</blockquote>
<p>Passing NULL as the third argument tells OpenSSL to use the 'default<br>
implementation', which can be changed at runtime by an<br>
ENGINE_set_default*() function call by another C extension (such as<br>
openssl).</p>
</blockquote>
<p>It is beginning to sound like this shouldn't be used and decoupling digest from openssl is a better way forward?<br>
I'll probably continue to use this patch locally given our customers are desiring FIPS and rewriting all gems is a very large undertaking, but curious of your thoughts?</p> Ruby master - Feature #13681: Ruby digest init fails in FIPS mode when built against OpenSSL ~> 1.0.1https://bugs.ruby-lang.org/issues/13681?journal_id=656052017-06-30T15:43:52Zrinzler (Colton Jenkins)
<ul></ul><p>rhenium (Kazuki Yamaguchi) wrote:</p>
<blockquote>
<p>rinzler (Colton Jenkins) wrote:</p>
<blockquote>
<blockquote>
<blockquote>
<pre><code class="diff syntaxhl" data-language="diff"><span class="gi">+ rb_raise(rb_eRuntimeError, error_message); \
+ } \
+ *ctx = *(upper_name##_CTX*)md_ctx->md_data; \
</span></code></pre>
</blockquote>
<p>This won't compile with OpenSSL 1.1.x since <code>EVP_MD_CTX</code> was made opaque.</p>
</blockquote>
<p>Hasn't it always been opaque? They just moved the struct def from evp.h -> evp_locl.h, but the typedef is still in ossl_typ.h</p>
</blockquote>
<p>crypto/evp/evp_locl.h is not a public header file. -> operator can't be<br>
used against md_ctx.</p>
<blockquote>
<blockquote>
<p>Also I suspect this approach breaks if an external OpenSSL engine<br>
replaces the default implementation for the algorithm. I think we have<br>
to completely rewrite to use the EVP API only.</p>
</blockquote>
<p>Not sure I follow. Passing NULL for engine skips it? But using EVP api would be ideal.</p>
</blockquote>
<p>Passing NULL as the third argument tells OpenSSL to use the 'default<br>
implementation', which can be changed at runtime by an<br>
ENGINE_set_default*() function call by another C extension (such as<br>
openssl).</p>
</blockquote>
<p>I think I'll move forward with adding in the above recommendations and place a condition for OPENSSL_VERSION < 1.1.<br>
Given FOM 3.0 is still up in the air (<a href="https://www.safelogic.com/openssl-1-1-update/" class="external">https://www.safelogic.com/openssl-1-1-update/</a>) this would only be needed with 1.0.x anyhow.<br>
I'll update the patch.</p> Ruby master - Feature #13681: Ruby digest init fails in FIPS mode when built against OpenSSL ~> 1.0.1https://bugs.ruby-lang.org/issues/13681?journal_id=656302017-07-04T16:18:33Zrhenium (Kazuki Yamaguchi)k@rhe.jp
<ul></ul><p>rinzler (Colton Jenkins) wrote:</p>
<blockquote>
<p>Ah ok, do you know what will happen to openssl::digest then given it uses the same? Curious if that plans to be refactored upon 1.1.x I could do the same with this. If not then this doesn't make much sense.</p>
</blockquote>
<p>See ext/openssl/ossl_digest.c -- it doesn't use the low level API like<br>
SHA1_Update() at all and uses the EVP API consistently.</p>
<blockquote>
<p>It is beginning to sound like this shouldn't be used and decoupling digest from openssl is a better way forward?<br>
I'll probably continue to use this patch locally given our customers are desiring FIPS and rewriting all gems is a very large undertaking, but curious of your thoughts?</p>
</blockquote>
<p>OK, I've just learnt about FIPS 140. So what is important for you is<br>
that the implementation of those algorithms is FIPS-certified (i.e.,<br>
OpenSSL); I didn't think of that case. Although I'm pretty sure it<br>
wasn't originally for that purpose that ext/digest prefers to use<br>
OpenSSL but purely for the performance. Refactoring ext/digest to use<br>
the EVP API seems the best direction, then.</p>
<blockquote>
<p>I think I'll move forward with adding in the above recommendations and place a condition for OPENSSL_VERSION < 1.1.</p>
</blockquote>
<p>No such a condition would be needed, you shouldn't have to extract the md_data<br>
field from an EVP_MD_CTX.</p> Ruby master - Feature #13681: Ruby digest init fails in FIPS mode when built against OpenSSL ~> 1.0.1https://bugs.ruby-lang.org/issues/13681?journal_id=656312017-07-04T17:17:29Zrinzler (Colton Jenkins)
<ul></ul><p>rhenium (Kazuki Yamaguchi) wrote:</p>
<blockquote>
<p>rinzler (Colton Jenkins) wrote:</p>
<blockquote>
<p>Ah ok, do you know what will happen to openssl::digest then given it uses the same? Curious if that plans to be refactored upon 1.1.x I could do the same with this. If not then this doesn't make much sense.</p>
</blockquote>
<p>See ext/openssl/ossl_digest.c -- it doesn't use the low level API like<br>
SHA1_Update() at all and uses the EVP API consistently.</p>
</blockquote>
<p>Yep, I reviewed and noticed it doesn't attempt to access EVP_MD_CTX directly at all. Just passes it around.</p>
<blockquote>
<blockquote>
<p>It is beginning to sound like this shouldn't be used and decoupling digest from openssl is a better way forward?<br>
I'll probably continue to use this patch locally given our customers are desiring FIPS and rewriting all gems is a very large undertaking, but curious of your thoughts?</p>
</blockquote>
<p>OK, I've just learnt about FIPS 140. So what is important for you is<br>
that the implementation of those algorithms is FIPS-certified (i.e.,<br>
OpenSSL); I didn't think of that case. Although I'm pretty sure it<br>
wasn't originally for that purpose that ext/digest prefers to use<br>
OpenSSL but purely for the performance. Refactoring ext/digest to use<br>
the EVP API seems the best direction, then.</p>
</blockquote>
<p>Yes, if openssl with FOM is present then utilizing EVP interface would make life much easier.<br>
Rewriting ext/digest to use EVP would be a larger task to accomplish given what is currently in place works off of alg##_CTX directly.<br>
How is larger work typically done here? Just one large patch or? I'd like to help if possible.</p>
<blockquote>
<blockquote>
<p>I think I'll move forward with adding in the above recommendations and place a condition for OPENSSL_VERSION < 1.1.</p>
</blockquote>
<p>No such a condition would be needed, you shouldn't have to extract the md_data<br>
field from an EVP_MD_CTX.</p>
</blockquote>
<p>Right, if alg##_CTX wasn't passed in then no need to interact with EVP_MD_CTX.<br>
We would have to change how digest init/update/final signatures are called.</p>