Feature #13681: Ruby digest init fails in FIPS mode when built against OpenSSL ~> 1.0.1

• Status: Open
• Assignee: -
• Target version: -

[rinzler (Colton Jenkins)]

When FIPS (https://en.wikipedia.org/wiki/FIPS_140-2) is enabled attempting to initialize any digest will kill the process due to https://github.com/openssl/openssl/commit/65300dcfb04bae643ea7b8f42ff8c8f1b1210a9e

Example,

> require 'digest'
> Digest::MD5.new
md5_dgst.c(75): OpenSSL internal error, assertion failed: Low level API call to digest MD5 forbidden in FIPS mode!

> require 'digest'
> Digest::SHA1.new
sha_locl.h(128): OpenSSL internal error, assertion failed: Low level API call to digest SHA1 forbidden in FIPS mode!

This patch will redefine alg##_Init to use the EVP interface. This allows the digest initialization to never die, but will fail when using a non FIPS algorithm (MD5).

Example,

irb(main):002:0> Digest::MD5.new
RuntimeError: disabled for fips
	from (irb):2:in `new'
	from (irb):2
	from /usr/local/bin/irb:11:in `<main>'
irb(main):003:0> Digest::SHA1.new
=> #<Digest::SHA1: da39a3ee5e6b4b0d3255bfef95601890afd80709>

[rhenium (Kazuki Yamaguchi)]

diff -Nurp old/ext/digest/digest.h new/ext/digest/digest.h
--- old/ext/digest/digest.h	2017-06-21 12:56:09.007011362 -0400
+++ new/ext/digest/digest.h	2017-06-21 12:56:32.458975554 -0400
@@ -31,6 +31,26 @@ typedef struct {
     rb_digest_hash_finish_func_t finish_func;
 } rb_digest_metadata_t;
 
+#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()); \

The OpenSSL error queue must be cleared by ERR_clear_error().

+    rb_raise(rb_eRuntimeError, error_message); \
+  } \
+  *ctx = *(upper_name##_CTX*)md_ctx->md_data; \

This won't compile with OpenSSL 1.1.x since EVP_MD_CTX was made opaque.

Also I suspect this approach breaks if an external OpenSSL engine
replaces the default implementation for the algorithm. I think we have
to completely rewrite to use the EVP API only.

+  EVP_MD_CTX_destroy(md_ctx); \
+ \
+  return 1; \
+}
+
 #define DEFINE_UPDATE_FUNC_FOR_UINT(name) \
 void \
 rb_digest_##name##_update(void *ctx, unsigned char *ptr, size_t size) \

Another idea: I wonder if it would be possible or desirable to rip out
the OpenSSL backend entirely instead. There would be some performance
degradation, though, people could switch to OpenSSL::Digest of the
'openssl' extension if they really care about. It uses the EVP API and
works as a drop-in replacement (in fact it inherits from Digest::Base).

[nobu (Nobuyoshi Nakada)]

rhenium (Kazuki Yamaguchi) wrote:

+    error_message = ERR_reason_error_string(ERR_peek_last_error()); \
+    rb_raise(rb_eRuntimeError, error_message); \

Just a note, this causes -Wformat-security "format string is not a string literal (potentially insecure)" warnings.
And probably EVP_MD_CTX_* functions need to be checked in ext/digest/digest_conf.rb.

[rinzler (Colton Jenkins)]

rhenium (Kazuki Yamaguchi) wrote:

diff -Nurp old/ext/digest/digest.h new/ext/digest/digest.h
--- old/ext/digest/digest.h	2017-06-21 12:56:09.007011362 -0400
+++ new/ext/digest/digest.h	2017-06-21 12:56:32.458975554 -0400
@@ -31,6 +31,26 @@ typedef struct {
     rb_digest_hash_finish_func_t finish_func;
 } rb_digest_metadata_t;
 
+#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()); \

The OpenSSL error queue must be cleared by ERR_clear_error().

Ah, good catch.

+    rb_raise(rb_eRuntimeError, error_message); \
+  } \
+  *ctx = *(upper_name##_CTX*)md_ctx->md_data; \

This won't compile with OpenSSL 1.1.x since EVP_MD_CTX was made opaque.

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

Also I suspect this approach breaks if an external OpenSSL engine
replaces the default implementation for the algorithm. I think we have
to completely rewrite to use the EVP API only.

Not sure I follow. Passing NULL for engine skips it? But using EVP api would be ideal.

+  EVP_MD_CTX_destroy(md_ctx); \
+ \
+  return 1; \
+}
+
 #define DEFINE_UPDATE_FUNC_FOR_UINT(name) \
 void \
 rb_digest_##name##_update(void *ctx, unsigned char *ptr, size_t size) \

Another idea: I wonder if it would be possible or desirable to rip out
the OpenSSL backend entirely instead. There would be some performance
degradation, though, people could switch to OpenSSL::Digest of the
'openssl' extension if they really care about. It uses the EVP API and
works as a drop-in replacement (in fact it inherits from Digest::Base).

That's an option. OpenSSL 1.1.x 'currently' doesn't support FIPS so it would
be up to the app devs (me) to ensure all code doesn't use non fips compliant algos (ouch).
From what I've seen the majority of gems use digest over openssl::digest
given it isn't guaranteed the system it deploys too will have it.
What is nice about it's current implementation is if the system does have OpenSSL
with fips enabled then digest will obtain that functionality with this.
I doubt many rubyist have to deal with FIPS, but this does make life a bit easier.
https://github.com/bundler/bundler/issues/4989

[rinzler (Colton Jenkins)]

nobu (Nobuyoshi Nakada) wrote:

rhenium (Kazuki Yamaguchi) wrote:

+    error_message = ERR_reason_error_string(ERR_peek_last_error()); \
+    rb_raise(rb_eRuntimeError, error_message); \

Just a note, this causes -Wformat-security "format string is not a string literal (potentially insecure)" warnings.
And probably EVP_MD_CTX_* functions need to be checked in ext/digest/digest_conf.rb.

K, I'll check that out. Haven't coded in C in quite some time.
Will do.

[rhenium (Kazuki Yamaguchi)]

rinzler (Colton Jenkins) wrote:

+    rb_raise(rb_eRuntimeError, error_message); \
+  } \
+  *ctx = *(upper_name##_CTX*)md_ctx->md_data; \

This won't compile with OpenSSL 1.1.x since EVP_MD_CTX was made opaque.

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

crypto/evp/evp_locl.h is not a public header file. -> operator can't be
used against md_ctx.

Also I suspect this approach breaks if an external OpenSSL engine
replaces the default implementation for the algorithm. I think we have
to completely rewrite to use the EVP API only.

Not sure I follow. Passing NULL for engine skips it? But using EVP api would be ideal.

Passing NULL as the third argument tells OpenSSL to use the 'default
implementation', which can be changed at runtime by an
ENGINE_set_default*() function call by another C extension (such as
openssl).

[rinzler (Colton Jenkins)]

rhenium (Kazuki Yamaguchi) wrote:

rinzler (Colton Jenkins) wrote:

+    rb_raise(rb_eRuntimeError, error_message); \
+  } \
+  *ctx = *(upper_name##_CTX*)md_ctx->md_data; \

This won't compile with OpenSSL 1.1.x since EVP_MD_CTX was made opaque.

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

crypto/evp/evp_locl.h is not a public header file. -> operator can't be
used against md_ctx.

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.

Also I suspect this approach breaks if an external OpenSSL engine
replaces the default implementation for the algorithm. I think we have
to completely rewrite to use the EVP API only.

Not sure I follow. Passing NULL for engine skips it? But using EVP api would be ideal.

Passing NULL as the third argument tells OpenSSL to use the 'default
implementation', which can be changed at runtime by an
ENGINE_set_default*() function call by another C extension (such as
openssl).

It is beginning to sound like this shouldn't be used and decoupling digest from openssl is a better way forward?
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?

[rinzler (Colton Jenkins)]

rhenium (Kazuki Yamaguchi) wrote:

rinzler (Colton Jenkins) wrote:

+    rb_raise(rb_eRuntimeError, error_message); \
+  } \
+  *ctx = *(upper_name##_CTX*)md_ctx->md_data; \

This won't compile with OpenSSL 1.1.x since EVP_MD_CTX was made opaque.

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

crypto/evp/evp_locl.h is not a public header file. -> operator can't be
used against md_ctx.

Also I suspect this approach breaks if an external OpenSSL engine
replaces the default implementation for the algorithm. I think we have
to completely rewrite to use the EVP API only.

Not sure I follow. Passing NULL for engine skips it? But using EVP api would be ideal.

Passing NULL as the third argument tells OpenSSL to use the 'default
implementation', which can be changed at runtime by an
ENGINE_set_default*() function call by another C extension (such as
openssl).

I think I'll move forward with adding in the above recommendations and place a condition for OPENSSL_VERSION < 1.1.
Given FOM 3.0 is still up in the air (https://www.safelogic.com/openssl-1-1-update/) this would only be needed with 1.0.x anyhow.
I'll update the patch.

[rhenium (Kazuki Yamaguchi)]

rinzler (Colton Jenkins) wrote:

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.

See ext/openssl/ossl_digest.c -- it doesn't use the low level API like
SHA1_Update() at all and uses the EVP API consistently.

It is beginning to sound like this shouldn't be used and decoupling digest from openssl is a better way forward?
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?

OK, I've just learnt about FIPS 140. So what is important for you is
that the implementation of those algorithms is FIPS-certified (i.e.,
OpenSSL); I didn't think of that case. Although I'm pretty sure it
wasn't originally for that purpose that ext/digest prefers to use
OpenSSL but purely for the performance. Refactoring ext/digest to use
the EVP API seems the best direction, then.

I think I'll move forward with adding in the above recommendations and place a condition for OPENSSL_VERSION < 1.1.

No such a condition would be needed, you shouldn't have to extract the md_data
field from an EVP_MD_CTX.

[rinzler (Colton Jenkins)]

rhenium (Kazuki Yamaguchi) wrote:

rinzler (Colton Jenkins) wrote:

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.

See ext/openssl/ossl_digest.c -- it doesn't use the low level API like
SHA1_Update() at all and uses the EVP API consistently.

Yep, I reviewed and noticed it doesn't attempt to access EVP_MD_CTX directly at all. Just passes it around.

It is beginning to sound like this shouldn't be used and decoupling digest from openssl is a better way forward?
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?

OK, I've just learnt about FIPS 140. So what is important for you is
that the implementation of those algorithms is FIPS-certified (i.e.,
OpenSSL); I didn't think of that case. Although I'm pretty sure it
wasn't originally for that purpose that ext/digest prefers to use
OpenSSL but purely for the performance. Refactoring ext/digest to use
the EVP API seems the best direction, then.

Yes, if openssl with FOM is present then utilizing EVP interface would make life much easier.
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.
How is larger work typically done here? Just one large patch or? I'd like to help if possible.

I think I'll move forward with adding in the above recommendations and place a condition for OPENSSL_VERSION < 1.1.

No such a condition would be needed, you shouldn't have to extract the md_data
field from an EVP_MD_CTX.

Right, if alg##_CTX wasn't passed in then no need to interact with EVP_MD_CTX.
We would have to change how digest init/update/final signatures are called.