Merge branch 'maint'

* maint:
  History.md: fix a typo
  x509cert, x509crl, x509req, ns_spki: check sanity of public key
  pkey: make pkey_check_public_key() non-static
  test/test_cipher: fix test_non_aead_cipher_set_auth_data failure
  cipher: disallow setting AAD for non-AEAD ciphers
  test/test_ssl_session: skip tests for session_remove_cb
  appveyor.yml: remove 'openssl version' line

12 files changed, 92 insertions, 48 deletions

diff --git a/History.md b/History.md
index 4e12682c..1f9a4fba 100644
--- a/History.md
+++ b/History.md
@@ -201,7 +201,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
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
diff --git a/ext/openssl/ossl_cipher.c b/ext/openssl/ossl_cipher.c
index e6179733..3038a766 100644
--- a/ext/openssl/ossl_cipher.c
+++ b/ext/openssl/ossl_cipher.c
@@ -569,6 +569,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/ext/openssl/ossl_ns_spki.c b/ext/openssl/ossl_ns_spki.c
index f17b9509..6f61e61b 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_pkey.c b/ext/openssl/ossl_pkey.c
index 23e21154..2b96ece5 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 = ossl_evp_get_digestbyname(digest);
     StringValue(sig);
     siglen = RSTRING_LENINT(sig);
diff --git a/ext/openssl/ossl_pkey.h b/ext/openssl/ossl_pkey.h
index a87472ad..2b17bf53 100644
--- a/ext/openssl/ossl_pkey.h
+++ b/ext/openssl/ossl_pkey.h
@@ -44,6 +44,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);
diff --git a/ext/openssl/ossl_ssl.c b/ext/openssl/ossl_ssl.c
index 63bdc9ab..8cdb9737 100644
--- a/ext/openssl/ossl_ssl.c
+++ b/ext/openssl/ossl_ssl.c
@@ -2595,6 +2595,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/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c
index 8d16b9b7..40542c4a 100644
--- a/ext/openssl/ossl_x509cert.c
+++ b/ext/openssl/ossl_x509cert.c
@@ -508,18 +508,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;
 }
 
@@ -557,9 +558,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 45cf7fb4..b0badf45 100644
--- a/ext/openssl/ossl_x509crl.c
+++ b/ext/openssl/ossl_x509crl.c
@@ -359,9 +359,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 9f20dba3..2c20042a 100644
--- a/ext/openssl/ossl_x509req.c
+++ b/ext/openssl/ossl_x509req.c
@@ -293,11 +293,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;
 }
 
@@ -328,7 +327,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;
diff --git a/test/test_cipher.rb b/test/test_cipher.rb
index d87dedf7..56061741 100644
--- a/test/test_cipher.rb
+++ b/test/test_cipher.rb
@@ -295,6 +295,13 @@ class OpenSSL::TestCipher < OpenSSL::TestCase
     assert_equal tag1, tag2
   end
 
+  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)
diff --git a/test/test_ssl_session.rb b/test/test_ssl_session.rb
index 199f722e..2cb46cd2 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