From 6375a356b3729a809cb14f128f61d3c1c58731bb Mon Sep 17 00:00:00 2001 From: Kazuki Yamaguchi Date: Sun, 8 Oct 2017 09:28:15 +0900 Subject: appveyor.yml: remove 'openssl version' line It runs the 'openssl' command line tool that is not of the version used to compile and run the test suite. Thanks to MSP-Greg for pointing this out. Fixes: https://github.com/ruby/openssl/issues/157 --- appveyor.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/appveyor.yml b/appveyor.yml index 9ff363fc..18484fb9 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -14,7 +14,6 @@ install: $Env:openssl_dir = "C:\msys64\mingw64" } - ruby -v - - openssl version - rake install_dependencies build_script: - rake -rdevkit compile -- --with-openssl-dir=%openssl_dir% --enable-debug -- cgit v1.2.3 From d1cbf6d75280d208f352d6ff245be515a75c7933 Mon Sep 17 00:00:00 2001 From: Kazuki Yamaguchi Date: Tue, 10 Oct 2017 14:35:42 +0900 Subject: test/test_ssl_session: skip tests for session_remove_cb In OpenSSL < 1.1.0, the session_remove_cb callback is called inside the global lock for CRYPTO_LOCK_SSL_CTX which is shared across the entire process, not just for the specific SSL_CTX object. It is possible that the callback releases GVL while the lock for CRYPTO_LOCK_SSL_CTX is held, causing another thread calling an OpenSSL function that tries to acquire the same lock stuck forever. Add a note about the possible deadlock to the docs for SSLContext#session_remove_cb=, and skip the relevant test cases unless the OSSL_TEST_ALL environment variable is set to 1. A deadlock due to this issue is observed: http://ci.rvm.jp/results/trunk-test@frontier/104428 --- ext/openssl/ossl_ssl.c | 4 ++++ test/test_ssl_session.rb | 58 +++++++++++++++++++++++++++++++++--------------- 2 files changed, 44 insertions(+), 18 deletions(-) diff --git a/ext/openssl/ossl_ssl.c b/ext/openssl/ossl_ssl.c index aa2dbbc8..736f4924 100644 --- a/ext/openssl/ossl_ssl.c +++ b/ext/openssl/ossl_ssl.c @@ -2446,6 +2446,10 @@ Init_ossl_ssl(void) * A callback invoked when a session is removed from the internal cache. * * The callback is invoked with an SSLContext and a Session. + * + * IMPORTANT NOTE: It is currently not possible to use this safely in a + * multi-threaded application. The callback is called inside a global lock + * and it can randomly cause deadlock on Ruby thread switching. */ rb_attr(cSSLContext, rb_intern("session_remove_cb"), 1, 1, Qfalse); diff --git a/test/test_ssl_session.rb b/test/test_ssl_session.rb index 9d0e5a2d..af8c65b1 100644 --- a/test/test_ssl_session.rb +++ b/test/test_ssl_session.rb @@ -215,6 +215,10 @@ __EOS__ end end + # Skipping tests that use session_remove_cb by default because it may cause + # deadlock. + TEST_SESSION_REMOVE_CB = ENV["OSSL_TEST_ALL"] == "1" + def test_ctx_client_session_cb pend "TLS 1.2 is not supported" unless tls12_supported? @@ -227,11 +231,13 @@ __EOS__ sock, sess = ary called[:new] = [sock, sess] } - ctx.session_remove_cb = lambda { |ary| - ctx, sess = ary - called[:remove] = [ctx, sess] - # any resulting value is OK (ignored) - } + if TEST_SESSION_REMOVE_CB + ctx.session_remove_cb = lambda { |ary| + ctx, sess = ary + called[:remove] = [ctx, sess] + # any resulting value is OK (ignored) + } + end server_connect_with_session(port, ctx, nil) { |ssl| assert_equal(1, ctx.session_cache_stats[:cache_num]) @@ -239,7 +245,9 @@ __EOS__ assert_equal([ssl, ssl.session], called[:new]) assert(ctx.session_remove(ssl.session)) assert(!ctx.session_remove(ssl.session)) - assert_equal([ctx, ssl.session], called[:remove]) + if TEST_SESSION_REMOVE_CB + assert_equal([ctx, ssl.session], called[:remove]) + end } end end @@ -275,10 +283,12 @@ __EOS__ last_server_session = sess } - ctx.session_remove_cb = lambda { |ary| - _ctx, sess = ary - called[:remove] = sess - } + if TEST_SESSION_REMOVE_CB + ctx.session_remove_cb = lambda { |ary| + _ctx, sess = ary + called[:remove] = sess + } + end } start_server(ctx_proc: ctx_proc) do |port| connections = 0 @@ -290,7 +300,9 @@ __EOS__ assert_nil called[:get] assert_not_nil called[:new] assert_equal sess0.id, called[:new].id - assert_nil called[:remove] + if TEST_SESSION_REMOVE_CB + assert_nil called[:remove] + end called.clear # Internal cache hit @@ -302,12 +314,16 @@ __EOS__ } assert_nil called[:get] assert_nil called[:new] - assert_nil called[:remove] + if TEST_SESSION_REMOVE_CB + assert_nil called[:remove] + end called.clear sctx.flush_sessions(Time.now + 10000) - assert_not_nil called[:remove] - assert_equal sess0.id, called[:remove].id + if TEST_SESSION_REMOVE_CB + assert_not_nil called[:remove] + assert_equal sess0.id, called[:remove].id + end called.clear # External cache hit @@ -325,12 +341,16 @@ __EOS__ assert_equal sess0.id, sess2.id assert_equal sess0.id, called[:get] assert_nil called[:new] - assert_nil called[:remove] + if TEST_SESSION_REMOVE_CB + assert_nil called[:remove] + end called.clear sctx.flush_sessions(Time.now + 10000) - assert_not_nil called[:remove] - assert_equal sess0.id, called[:remove].id + if TEST_SESSION_REMOVE_CB + assert_not_nil called[:remove] + assert_equal sess0.id, called[:remove].id + end called.clear # Cache miss @@ -344,7 +364,9 @@ __EOS__ assert_equal sess0.id, called[:get] assert_not_nil called[:new] assert_equal sess3.id, called[:new].id - assert_nil called[:remove] + if TEST_SESSION_REMOVE_CB + assert_nil called[:remove] + end end end -- cgit v1.2.3 From bb10767b0570d44f240632a7399c882764a48649 Mon Sep 17 00:00:00 2001 From: Kazuki Yamaguchi Date: Wed, 18 Oct 2017 23:24:37 +0900 Subject: cipher: disallow setting AAD for non-AEAD ciphers EVP_CipherUpdate() must not be call with the output parameter set to NULL when the cipher does not support AEAD. Check the flag of EVP_CIPHER, and raise an exception as necessary. Reference: http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-core/83337 Reference: https://bugs.ruby-lang.org/issues/14024 --- ext/openssl/ossl_cipher.c | 2 ++ test/test_cipher.rb | 7 +++++++ 2 files changed, 9 insertions(+) diff --git a/ext/openssl/ossl_cipher.c b/ext/openssl/ossl_cipher.c index 36e42ede..740f04b2 100644 --- a/ext/openssl/ossl_cipher.c +++ b/ext/openssl/ossl_cipher.c @@ -580,6 +580,8 @@ ossl_cipher_set_auth_data(VALUE self, VALUE data) in_len = RSTRING_LEN(data); GetCipher(self, ctx); + if (!(EVP_CIPHER_flags(EVP_CIPHER_CTX_cipher(ctx)) & EVP_CIPH_FLAG_AEAD_CIPHER)) + ossl_raise(eCipherError, "AEAD not supported by this cipher"); if (!ossl_cipher_update_long(ctx, NULL, &out_len, in, in_len)) ossl_raise(eCipherError, "couldn't set additional authenticated data"); diff --git a/test/test_cipher.rb b/test/test_cipher.rb index ad0e87b4..216eeded 100644 --- a/test/test_cipher.rb +++ b/test/test_cipher.rb @@ -297,6 +297,13 @@ class OpenSSL::TestCipher < OpenSSL::TestCase assert_equal tag1, tag2 end if has_cipher?("aes-128-gcm") + def test_non_aead_cipher_set_auth_data + assert_raise(OpenSSL::Cipher::CipherError) { + cipher = OpenSSL::Cipher.new("aes-128-cfb").encrypt + cipher.auth_data = "123" + } + end + private def new_encryptor(algo, **kwargs) -- cgit v1.2.3 From 4cf2074148f658ec7c57c70f3e9d447991ae5c32 Mon Sep 17 00:00:00 2001 From: Kazuki Yamaguchi Date: Thu, 19 Oct 2017 11:01:50 +0900 Subject: test/test_cipher: fix test_non_aead_cipher_set_auth_data failure A follow-up to commit bb10767b0570 ("cipher: disallow setting AAD for non-AEAD ciphers", 2017-10-18). Cipher#auth_data= raises NotImplementedError if built with OpenSSL < 1.0.1. --- test/test_cipher.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_cipher.rb b/test/test_cipher.rb index 216eeded..48149d41 100644 --- a/test/test_cipher.rb +++ b/test/test_cipher.rb @@ -302,7 +302,7 @@ class OpenSSL::TestCipher < OpenSSL::TestCase cipher = OpenSSL::Cipher.new("aes-128-cfb").encrypt cipher.auth_data = "123" } - end + end if has_cipher?("aes-128-gcm") private -- cgit v1.2.3 From 1425bf543ddcf7280877624532128cdd311f54ab Mon Sep 17 00:00:00 2001 From: Kazuki Yamaguchi Date: Sat, 11 Nov 2017 00:59:57 +0900 Subject: pkey: make pkey_check_public_key() non-static Also make it take const pointer as it never modifies the pkey. --- ext/openssl/ossl_pkey.c | 9 +++++---- ext/openssl/ossl_pkey.h | 1 + 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/ext/openssl/ossl_pkey.c b/ext/openssl/ossl_pkey.c index 314d1d94..aad3e2e4 100644 --- a/ext/openssl/ossl_pkey.c +++ b/ext/openssl/ossl_pkey.c @@ -163,8 +163,8 @@ ossl_pkey_new_from_data(int argc, VALUE *argv, VALUE self) return ossl_pkey_new(pkey); } -static void -pkey_check_public_key(EVP_PKEY *pkey) +void +ossl_pkey_check_public_key(const EVP_PKEY *pkey) { void *ptr; const BIGNUM *n, *e, *pubkey; @@ -172,7 +172,8 @@ pkey_check_public_key(EVP_PKEY *pkey) if (EVP_PKEY_missing_parameters(pkey)) ossl_raise(ePKeyError, "parameters missing"); - ptr = EVP_PKEY_get0(pkey); + /* OpenSSL < 1.1.0 takes non-const pointer */ + ptr = EVP_PKEY_get0((EVP_PKEY *)pkey); switch (EVP_PKEY_base_id(pkey)) { case EVP_PKEY_RSA: RSA_get0_key(ptr, &n, &e, NULL); @@ -352,7 +353,7 @@ ossl_pkey_verify(VALUE self, VALUE digest, VALUE sig, VALUE data) int siglen, result; GetPKey(self, pkey); - pkey_check_public_key(pkey); + ossl_pkey_check_public_key(pkey); md = GetDigestPtr(digest); StringValue(sig); siglen = RSTRING_LENINT(sig); diff --git a/ext/openssl/ossl_pkey.h b/ext/openssl/ossl_pkey.h index e3b723cd..a0b49744 100644 --- a/ext/openssl/ossl_pkey.h +++ b/ext/openssl/ossl_pkey.h @@ -48,6 +48,7 @@ int ossl_generate_cb_2(int p, int n, BN_GENCB *cb); void ossl_generate_cb_stop(void *ptr); VALUE ossl_pkey_new(EVP_PKEY *); +void ossl_pkey_check_public_key(const EVP_PKEY *); EVP_PKEY *GetPKeyPtr(VALUE); EVP_PKEY *DupPKeyPtr(VALUE); EVP_PKEY *GetPrivPKeyPtr(VALUE); -- cgit v1.2.3 From 363f40fb47bef81a784d4cc6feabef345ada9b0f Mon Sep 17 00:00:00 2001 From: Kazuki Yamaguchi Date: Sat, 11 Nov 2017 01:08:27 +0900 Subject: x509cert, x509crl, x509req, ns_spki: check sanity of public key The pub_encode routine of an EVP_PKEY_ASN1_METHOD seems to assume the parameters and public key component(s) to be set properly. Calling that, for example, through X509_set_pubkey(), with an incomplete object may cause segfault. Use ossl_pkey_check_public_key() to check that. It doesn't look pretty, but unfortunately there isn't a generic way to do that with the EVP API. Something similar applies to the verify routine of an EVP_PKEY_METHOD. Do the same check before calling *_verify(). Reference: http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-core/83688 Reference: https://bugs.ruby-lang.org/issues/14087 --- ext/openssl/ossl_ns_spki.c | 24 ++++++++++++++---------- ext/openssl/ossl_x509cert.c | 15 ++++++++------- ext/openssl/ossl_x509crl.c | 5 ++++- ext/openssl/ossl_x509req.c | 12 ++++++------ 4 files changed, 32 insertions(+), 24 deletions(-) diff --git a/ext/openssl/ossl_ns_spki.c b/ext/openssl/ossl_ns_spki.c index 4d978bd0..5ac8ef55 100644 --- a/ext/openssl/ossl_ns_spki.c +++ b/ext/openssl/ossl_ns_spki.c @@ -208,12 +208,13 @@ static VALUE ossl_spki_set_public_key(VALUE self, VALUE key) { NETSCAPE_SPKI *spki; + EVP_PKEY *pkey; GetSPKI(self, spki); - if (!NETSCAPE_SPKI_set_pubkey(spki, GetPKeyPtr(key))) { /* NO NEED TO DUP */ - ossl_raise(eSPKIError, NULL); - } - + pkey = GetPKeyPtr(key); + ossl_pkey_check_public_key(pkey); + if (!NETSCAPE_SPKI_set_pubkey(spki, pkey)) + ossl_raise(eSPKIError, "NETSCAPE_SPKI_set_pubkey"); return key; } @@ -307,17 +308,20 @@ static VALUE ossl_spki_verify(VALUE self, VALUE key) { NETSCAPE_SPKI *spki; + EVP_PKEY *pkey; GetSPKI(self, spki); - switch (NETSCAPE_SPKI_verify(spki, GetPKeyPtr(key))) { /* NO NEED TO DUP */ - case 0: + pkey = GetPKeyPtr(key); + ossl_pkey_check_public_key(pkey); + switch (NETSCAPE_SPKI_verify(spki, pkey)) { + case 0: + ossl_clear_error(); return Qfalse; - case 1: + case 1: return Qtrue; - default: - ossl_raise(eSPKIError, NULL); + default: + ossl_raise(eSPKIError, "NETSCAPE_SPKI_verify"); } - return Qnil; /* dummy */ } /* Document-class: OpenSSL::Netscape::SPKI diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c index 87086a7c..91c25c4e 100644 --- a/ext/openssl/ossl_x509cert.c +++ b/ext/openssl/ossl_x509cert.c @@ -546,18 +546,19 @@ ossl_x509_get_public_key(VALUE self) /* * call-seq: - * cert.public_key = key => key + * cert.public_key = key */ static VALUE ossl_x509_set_public_key(VALUE self, VALUE key) { X509 *x509; + EVP_PKEY *pkey; GetX509(self, x509); - if (!X509_set_pubkey(x509, GetPKeyPtr(key))) { /* DUPs pkey */ - ossl_raise(eX509CertError, NULL); - } - + pkey = GetPKeyPtr(key); + ossl_pkey_check_public_key(pkey); + if (!X509_set_pubkey(x509, pkey)) + ossl_raise(eX509CertError, "X509_set_pubkey"); return key; } @@ -594,9 +595,9 @@ ossl_x509_verify(VALUE self, VALUE key) X509 *x509; EVP_PKEY *pkey; - pkey = GetPKeyPtr(key); /* NO NEED TO DUP */ GetX509(self, x509); - + pkey = GetPKeyPtr(key); + ossl_pkey_check_public_key(pkey); switch (X509_verify(x509, pkey)) { case 1: return Qtrue; diff --git a/ext/openssl/ossl_x509crl.c b/ext/openssl/ossl_x509crl.c index 035025ab..add72c6c 100644 --- a/ext/openssl/ossl_x509crl.c +++ b/ext/openssl/ossl_x509crl.c @@ -366,9 +366,12 @@ static VALUE ossl_x509crl_verify(VALUE self, VALUE key) { X509_CRL *crl; + EVP_PKEY *pkey; GetX509CRL(self, crl); - switch (X509_CRL_verify(crl, GetPKeyPtr(key))) { + pkey = GetPKeyPtr(key); + ossl_pkey_check_public_key(pkey); + switch (X509_CRL_verify(crl, pkey)) { case 1: return Qtrue; case 0: diff --git a/ext/openssl/ossl_x509req.c b/ext/openssl/ossl_x509req.c index 15bc7052..0f7fecdc 100644 --- a/ext/openssl/ossl_x509req.c +++ b/ext/openssl/ossl_x509req.c @@ -330,11 +330,10 @@ ossl_x509req_set_public_key(VALUE self, VALUE key) EVP_PKEY *pkey; GetX509Req(self, req); - pkey = GetPKeyPtr(key); /* NO NEED TO DUP */ - if (!X509_REQ_set_pubkey(req, pkey)) { - ossl_raise(eX509ReqError, NULL); - } - + pkey = GetPKeyPtr(key); + ossl_pkey_check_public_key(pkey); + if (!X509_REQ_set_pubkey(req, pkey)) + ossl_raise(eX509ReqError, "X509_REQ_set_pubkey"); return key; } @@ -365,7 +364,8 @@ ossl_x509req_verify(VALUE self, VALUE key) EVP_PKEY *pkey; GetX509Req(self, req); - pkey = GetPKeyPtr(key); /* NO NEED TO DUP */ + pkey = GetPKeyPtr(key); + ossl_pkey_check_public_key(pkey); switch (X509_REQ_verify(req, pkey)) { case 1: return Qtrue; -- cgit v1.2.3 From f3b596e858ea1604d0ea5653bffe80672c22f079 Mon Sep 17 00:00:00 2001 From: Kazuki Yamaguchi Date: Sat, 25 Nov 2017 19:17:04 +0900 Subject: History.md: fix a typo --- History.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/History.md b/History.md index d592bc6a..9569b100 100644 --- a/History.md +++ b/History.md @@ -170,7 +170,7 @@ Notable changes - A new option 'verify_hostname' is added to OpenSSL::SSL::SSLContext. When it is enabled, and the SNI hostname is also set, the hostname verification on the server certificate is automatically performed. It is now enabled by - OpenSSL::SSL::Context#set_params. + OpenSSL::SSL::SSLContext#set_params. [[GH ruby/openssl#60]](https://github.com/ruby/openssl/pull/60) Removals -- cgit v1.2.3