openssl: improve handling of password for encrypted PEM

* ext/openssl/ossl.c (ossl_pem_passwd_value): Added. Convert the
  argument to String with StringValue() and validate the length is in
  4..PEM_BUFSIZE. PEM_BUFSIZE is a macro defined in OpenSSL headers.
  (ossl_pem_passwd_cb): When reading/writing encrypted PEM format, we
  used to pass the password to PEM_def_callback() directly but it was
  problematic. It is not NUL character safe. And surprisingly, it
  silently truncates the password to 1024 bytes.  [GH ruby/openssl#51]

* ext/openssl/ossl.h: Add function prototype declaration of newly
  added ossl_pem_passwd_value().

* ext/openssl/ossl_pkey.c (ossl_pkey_new_from_data): Use
  ossl_pem_passwd_value() to validate the password String.

* ext/openssl/ossl_pkey_dsa.c (ossl_dsa_initialize, ossl_dsa_export):
  ditto.

* ext/openssl/ossl_pkey_ec.c (ossl_ec_key_initialize,
  ossl_ec_key_to_string): ditto.

* ext/openssl/ossl_pkey_rsa.c (ossl_rsa_initialize, ossl_rsa_export):
  ditto.

* test/openssl/test_pkey_{dsa,ec,rsa}.rb: test this.

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@55087 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

10 files changed, 151 insertions, 70 deletions

diff --git a/ChangeLog b/ChangeLog
index 7d6c88f0c9..26e1e21b7f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,30 @@
+Fri May 20 23:25:42 2016  Kazuki Yamaguchi  <k@rhe.jp>
+
+	* ext/openssl/ossl.c (ossl_pem_passwd_value): Added. Convert the
+	  argument to String with StringValue() and validate the length is in
+	  4..PEM_BUFSIZE. PEM_BUFSIZE is a macro defined in OpenSSL headers.
+	  (ossl_pem_passwd_cb): When reading/writing encrypted PEM format, we
+	  used to pass the password to PEM_def_callback() directly but it was
+	  problematic. It is not NUL character safe. And surprisingly, it
+	  silently truncates the password to 1024 bytes.  [GH ruby/openssl#51]
+
+	* ext/openssl/ossl.h: Add function prototype declaration of newly
+	  added ossl_pem_passwd_value().
+
+	* ext/openssl/ossl_pkey.c (ossl_pkey_new_from_data): Use
+	  ossl_pem_passwd_value() to validate the password String.
+
+	* ext/openssl/ossl_pkey_dsa.c (ossl_dsa_initialize, ossl_dsa_export):
+	  ditto.
+
+	* ext/openssl/ossl_pkey_ec.c (ossl_ec_key_initialize,
+	  ossl_ec_key_to_string): ditto.
+
+	* ext/openssl/ossl_pkey_rsa.c (ossl_rsa_initialize, ossl_rsa_export):
+	  ditto.
+
+	* test/openssl/test_pkey_{dsa,ec,rsa}.rb: test this.
+
 Fri May 20 23:45:53 2016  Naohisa Goto  <ngotogenome@gmail.com>
 
 	* id_table.c (list_id_table_init): When unaligned word access is
diff --git a/ext/openssl/ossl.c b/ext/openssl/ossl.c
index 2b5579e389..a0a7574cc6 100644
--- a/ext/openssl/ossl.c
+++ b/ext/openssl/ossl.c
@@ -147,6 +147,31 @@ ossl_buf2str(char *buf, int len)
 /*
  * our default PEM callback
  */
+
+/*
+ * OpenSSL requires passwords for PEM-encoded files to be at least four
+ * characters long. See crypto/pem/pem_lib.c (as of 1.0.2h)
+ */
+#define OSSL_MIN_PWD_LEN 4
+
+VALUE
+ossl_pem_passwd_value(VALUE pass)
+{
+    if (NIL_P(pass))
+	return Qnil;
+
+    StringValue(pass);
+
+    if (RSTRING_LEN(pass) < OSSL_MIN_PWD_LEN)
+	ossl_raise(eOSSLError, "password must be at least %d bytes", OSSL_MIN_PWD_LEN);
+    /* PEM_BUFSIZE is currently used as the second argument of pem_password_cb,
+     * that is +max_len+ of ossl_pem_passwd_cb() */
+    if (RSTRING_LEN(pass) > PEM_BUFSIZE)
+	ossl_raise(eOSSLError, "password must be shorter than %d bytes", PEM_BUFSIZE);
+
+    return pass;
+}
+
 static VALUE
 ossl_pem_passwd_cb0(VALUE flag)
 {
@@ -159,13 +184,29 @@ ossl_pem_passwd_cb0(VALUE flag)
 }
 
 int
-ossl_pem_passwd_cb(char *buf, int max_len, int flag, void *pwd)
+ossl_pem_passwd_cb(char *buf, int max_len, int flag, void *pwd_)
 {
-    int len, status = 0;
-    VALUE rflag, pass;
+    int len, status;
+    VALUE rflag, pass = (VALUE)pwd_;
+
+    if (RTEST(pass)) {
+	/* PEM_def_callback(buf, max_len, flag, StringValueCStr(pass)) does not
+	 * work because it does not allow NUL characters and truncates to 1024
+	 * bytes silently if the input is over 1024 bytes */
+	if (RB_TYPE_P(pass, T_STRING)) {
+	    len = RSTRING_LEN(pass);
+	    if (len >= OSSL_MIN_PWD_LEN && len <= max_len) {
+		memcpy(buf, RSTRING_PTR(pass), len);
+		return len;
+	    }
+	}
+	OSSL_Debug("passed data is not valid String???");
+	return -1;
+    }
 
-    if (pwd || !rb_block_given_p())
-	return PEM_def_callback(buf, max_len, flag, pwd);
+    if (!rb_block_given_p()) {
+	return PEM_def_callback(buf, max_len, flag, NULL);
+    }
 
     while (1) {
 	/*
@@ -181,12 +222,12 @@ ossl_pem_passwd_cb(char *buf, int max_len, int flag, void *pwd)
 	    return -1;
 	}
 	len = RSTRING_LENINT(pass);
-	if (len < 4) { /* 4 is OpenSSL hardcoded limit */
-	    rb_warning("password must be longer than 4 bytes");
+	if (len < OSSL_MIN_PWD_LEN) {
+	    rb_warning("password must be at least %d bytes", OSSL_MIN_PWD_LEN);
 	    continue;
 	}
 	if (len > max_len) {
-	    rb_warning("password must be shorter then %d bytes", max_len-1);
+	    rb_warning("password must be shorter than %d bytes", max_len);
 	    continue;
 	}
 	memcpy(buf, RSTRING_PTR(pass), len);
diff --git a/ext/openssl/ossl.h b/ext/openssl/ossl.h
index 5b2f6e11b9..25f2c857a8 100644
--- a/ext/openssl/ossl.h
+++ b/ext/openssl/ossl.h
@@ -77,11 +77,6 @@ extern "C" {
 #  include <openssl/ocsp.h>
 #endif
 
-/* OpenSSL requires passwords for PEM-encoded files to be at least four
- * characters long
- */
-#define OSSL_MIN_PWD_LEN 4
-
 /*
  * Common Module
  */
@@ -146,8 +141,14 @@ do{\
 }while(0)
 
 /*
- * our default PEM callback
+ * Our default PEM callback
  */
+/* Convert the argument to String and validate the length. Note this may raise. */
+VALUE ossl_pem_passwd_value(VALUE);
+/* Can be casted to pem_password_cb. If a password (String) is passed as the
+ * "arbitrary data" (typically the last parameter of PEM_{read,write}_
+ * functions), uses the value. If not, but a block is given, yields to it.
+ * If not either, fallbacks to PEM_def_callback() which reads from stdin. */
 int ossl_pem_passwd_cb(char *, int, int, void *);
 
 /*
diff --git a/ext/openssl/ossl_pkey.c b/ext/openssl/ossl_pkey.c
index 7e3154afd9..7240de82ab 100644
--- a/ext/openssl/ossl_pkey.c
+++ b/ext/openssl/ossl_pkey.c
@@ -152,27 +152,21 @@ ossl_pkey_new_from_file(VALUE filename)
 static VALUE
 ossl_pkey_new_from_data(int argc, VALUE *argv, VALUE self)
 {
-     EVP_PKEY *pkey;
-     BIO *bio;
-     VALUE data, pass;
-     char *passwd = NULL;
+    EVP_PKEY *pkey;
+    BIO *bio;
+    VALUE data, pass;
 
-     rb_scan_args(argc, argv, "11", &data, &pass);
+    rb_scan_args(argc, argv, "11", &data, &pass);
+    pass = ossl_pem_passwd_value(pass);
 
-     bio = ossl_obj2bio(data);
-     if (!(pkey = d2i_PrivateKey_bio(bio, NULL))) {
+    bio = ossl_obj2bio(data);
+    if (!(pkey = d2i_PrivateKey_bio(bio, NULL))) {
 	OSSL_BIO_reset(bio);
-	if (!NIL_P(pass)) {
-	    passwd = StringValuePtr(pass);
-	}
-	if (!(pkey = PEM_read_bio_PrivateKey(bio, NULL, ossl_pem_passwd_cb, passwd))) {
+	if (!(pkey = PEM_read_bio_PrivateKey(bio, NULL, ossl_pem_passwd_cb, (void *)pass))) {
 	    OSSL_BIO_reset(bio);
 	    if (!(pkey = d2i_PUBKEY_bio(bio, NULL))) {
 		OSSL_BIO_reset(bio);
-		if (!NIL_P(pass)) {
-		    passwd = StringValuePtr(pass);
-		}
-		pkey = PEM_read_bio_PUBKEY(bio, NULL, ossl_pem_passwd_cb, passwd);
+		pkey = PEM_read_bio_PUBKEY(bio, NULL, ossl_pem_passwd_cb, (void *)pass);
 	    }
 	}
     }
diff --git a/ext/openssl/ossl_pkey_dsa.c b/ext/openssl/ossl_pkey_dsa.c
index 4c0c3f1bd7..281d3a00c9 100644
--- a/ext/openssl/ossl_pkey_dsa.c
+++ b/ext/openssl/ossl_pkey_dsa.c
@@ -216,7 +216,6 @@ ossl_dsa_initialize(int argc, VALUE *argv, VALUE self)
     EVP_PKEY *pkey;
     DSA *dsa;
     BIO *in;
-    char *passwd = NULL;
     VALUE arg, pass;
 
     GetPKey(self, pkey);
@@ -229,10 +228,10 @@ ossl_dsa_initialize(int argc, VALUE *argv, VALUE self)
 	}
     }
     else {
-	if (!NIL_P(pass)) passwd = StringValuePtr(pass);
+	pass = ossl_pem_passwd_value(pass);
 	arg = ossl_to_der_if_possible(arg);
 	in = ossl_obj2bio(arg);
-	dsa = PEM_read_bio_DSAPrivateKey(in, NULL, ossl_pem_passwd_cb, passwd);
+	dsa = PEM_read_bio_DSAPrivateKey(in, NULL, ossl_pem_passwd_cb, (void *)pass);
 	if (!dsa) {
 	    OSSL_BIO_reset(in);
 	    dsa = PEM_read_bio_DSA_PUBKEY(in, NULL, NULL, NULL);
@@ -320,26 +319,20 @@ ossl_dsa_export(int argc, VALUE *argv, VALUE self)
     EVP_PKEY *pkey;
     BIO *out;
     const EVP_CIPHER *ciph = NULL;
-    char *passwd = NULL;
     VALUE cipher, pass, str;
 
     GetPKeyDSA(self, pkey);
     rb_scan_args(argc, argv, "02", &cipher, &pass);
     if (!NIL_P(cipher)) {
 	ciph = GetCipherPtr(cipher);
-	if (!NIL_P(pass)) {
-	    StringValue(pass);
-	    if (RSTRING_LENINT(pass) < OSSL_MIN_PWD_LEN)
-		ossl_raise(eOSSLError, "OpenSSL requires passwords to be at least four characters long");
-	    passwd = RSTRING_PTR(pass);
-	}
+	pass = ossl_pem_passwd_value(pass);
     }
     if (!(out = BIO_new(BIO_s_mem()))) {
 	ossl_raise(eDSAError, NULL);
     }
     if (DSA_HAS_PRIVATE(pkey->pkey.dsa)) {
 	if (!PEM_write_bio_DSAPrivateKey(out, pkey->pkey.dsa, ciph,
-					 NULL, 0, ossl_pem_passwd_cb, passwd)){
+					 NULL, 0, ossl_pem_passwd_cb, (void *)pass)){
 	    BIO_free(out);
 	    ossl_raise(eDSAError, NULL);
 	}
diff --git a/ext/openssl/ossl_pkey_ec.c b/ext/openssl/ossl_pkey_ec.c
index 26eb5e8acf..d4153db68c 100644
--- a/ext/openssl/ossl_pkey_ec.c
+++ b/ext/openssl/ossl_pkey_ec.c
@@ -168,7 +168,6 @@ static VALUE ossl_ec_key_initialize(int argc, VALUE *argv, VALUE self)
     EC_KEY *ec = NULL;
     VALUE arg, pass;
     VALUE group = Qnil;
-    char *passwd = NULL;
 
     GetPKey(self, pkey);
     if (pkey->pkey.ec)
@@ -188,15 +187,15 @@ static VALUE ossl_ec_key_initialize(int argc, VALUE *argv, VALUE self)
         	ec = EC_KEY_new();
         	group = arg;
         } else {
-            BIO *in = ossl_obj2bio(arg);
+            BIO *in;
 
-            if (!NIL_P(pass)) {
-		passwd = StringValuePtr(pass);
-	    }
-	    ec = PEM_read_bio_ECPrivateKey(in, NULL, ossl_pem_passwd_cb, passwd);
+	    pass = ossl_pem_passwd_value(pass);
+	    in = ossl_obj2bio(arg);
+
+	    ec = PEM_read_bio_ECPrivateKey(in, NULL, ossl_pem_passwd_cb, (void *)pass);
             if (!ec) {
 		OSSL_BIO_reset(in);
-		ec = PEM_read_bio_EC_PUBKEY(in, NULL, ossl_pem_passwd_cb, passwd);
+		ec = PEM_read_bio_EC_PUBKEY(in, NULL, ossl_pem_passwd_cb, (void *)pass);
             }
             if (!ec) {
 		OSSL_BIO_reset(in);
@@ -473,7 +472,6 @@ static VALUE ossl_ec_key_to_string(VALUE self, VALUE ciph, VALUE pass, int forma
     BIO *out;
     int i = -1;
     int private = 0;
-    char *password = NULL;
     VALUE str;
 
     Require_EC_KEY(self, ec);
@@ -493,20 +491,12 @@ static VALUE ossl_ec_key_to_string(VALUE self, VALUE ciph, VALUE pass, int forma
     switch(format) {
     case EXPORT_PEM:
     	if (private) {
-	    const EVP_CIPHER *cipher;
+	    const EVP_CIPHER *cipher = NULL;
 	    if (!NIL_P(ciph)) {
 		cipher = GetCipherPtr(ciph);
-		if (!NIL_P(pass)) {
-		    StringValue(pass);
-		    if (RSTRING_LENINT(pass) < OSSL_MIN_PWD_LEN)
-			ossl_raise(eOSSLError, "OpenSSL requires passwords to be at least four characters long");
-		    password = RSTRING_PTR(pass);
-		}
-	    }
-	    else {
-		cipher = NULL;
+		pass = ossl_pem_passwd_value(pass);
 	    }
-            i = PEM_write_bio_ECPrivateKey(out, ec, cipher, NULL, 0, NULL, password);
+            i = PEM_write_bio_ECPrivateKey(out, ec, cipher, NULL, 0, ossl_pem_passwd_cb, (void *)pass);
     	} else {
             i = PEM_write_bio_EC_PUBKEY(out, ec);
         }
diff --git a/ext/openssl/ossl_pkey_rsa.c b/ext/openssl/ossl_pkey_rsa.c
index 6ad9f3eda5..5219dbedff 100644
--- a/ext/openssl/ossl_pkey_rsa.c
+++ b/ext/openssl/ossl_pkey_rsa.c
@@ -210,7 +210,6 @@ ossl_rsa_initialize(int argc, VALUE *argv, VALUE self)
     EVP_PKEY *pkey;
     RSA *rsa;
     BIO *in;
-    char *passwd = NULL;
     VALUE arg, pass;
 
     GetPKey(self, pkey);
@@ -222,10 +221,10 @@ ossl_rsa_initialize(int argc, VALUE *argv, VALUE self)
 	if (!rsa) ossl_raise(eRSAError, NULL);
     }
     else {
-	if (!NIL_P(pass)) passwd = StringValuePtr(pass);
+	pass = ossl_pem_passwd_value(pass);
 	arg = ossl_to_der_if_possible(arg);
 	in = ossl_obj2bio(arg);
-	rsa = PEM_read_bio_RSAPrivateKey(in, NULL, ossl_pem_passwd_cb, passwd);
+	rsa = PEM_read_bio_RSAPrivateKey(in, NULL, ossl_pem_passwd_cb, (void *)pass);
 	if (!rsa) {
 	    OSSL_BIO_reset(in);
 	    rsa = PEM_read_bio_RSA_PUBKEY(in, NULL, NULL, NULL);
@@ -310,7 +309,6 @@ ossl_rsa_export(int argc, VALUE *argv, VALUE self)
     EVP_PKEY *pkey;
     BIO *out;
     const EVP_CIPHER *ciph = NULL;
-    char *passwd = NULL;
     VALUE cipher, pass, str;
 
     GetPKeyRSA(self, pkey);
@@ -319,19 +317,14 @@ ossl_rsa_export(int argc, VALUE *argv, VALUE self)
 
     if (!NIL_P(cipher)) {
 	ciph = GetCipherPtr(cipher);
-	if (!NIL_P(pass)) {
-	    StringValue(pass);
-	    if (RSTRING_LENINT(pass) < OSSL_MIN_PWD_LEN)
-		ossl_raise(eOSSLError, "OpenSSL requires passwords to be at least four characters long");
-	    passwd = RSTRING_PTR(pass);
-	}
+	pass = ossl_pem_passwd_value(pass);
     }
     if (!(out = BIO_new(BIO_s_mem()))) {
 	ossl_raise(eRSAError, NULL);
     }
     if (RSA_HAS_PRIVATE(pkey->pkey.rsa)) {
 	if (!PEM_write_bio_RSAPrivateKey(out, pkey->pkey.rsa, ciph,
-					 NULL, 0, ossl_pem_passwd_cb, passwd)) {
+					 NULL, 0, ossl_pem_passwd_cb, (void *)pass)) {
 	    BIO_free(out);
 	    ossl_raise(eRSAError, NULL);
 	}
diff --git a/test/openssl/test_pkey_dsa.rb b/test/openssl/test_pkey_dsa.rb
index 2c0e1fc24f..680a123997 100644
--- a/test/openssl/test_pkey_dsa.rb
+++ b/test/openssl/test_pkey_dsa.rb
@@ -218,6 +218,18 @@ YNMbNw==
     assert(pem)
   end
 
+  def test_export_password_funny
+    key = OpenSSL::TestUtils::TEST_KEY_DSA256
+    pem = key.export(OpenSSL::Cipher.new('AES-128-CBC'), "pass\0wd")
+    assert_raise(ArgumentError) do
+      OpenSSL::PKey.read(pem, "pass")
+    end
+    key2 = OpenSSL::PKey.read(pem, "pass\0wd")
+    assert(key2.private?)
+    key3 = OpenSSL::PKey::DSA.new(pem, "pass\0wd")
+    assert(key3.private?)
+  end
+
   private
 
   def check_sign_verify(digest)
diff --git a/test/openssl/test_pkey_ec.rb b/test/openssl/test_pkey_ec.rb
index 673b0d6f43..23afed672a 100644
--- a/test/openssl/test_pkey_ec.rb
+++ b/test/openssl/test_pkey_ec.rb
@@ -184,6 +184,18 @@ class OpenSSL::TestEC < OpenSSL::TestCase
     assert(pem)
   end
 
+  def test_export_password_funny
+    key = OpenSSL::TestUtils::TEST_KEY_EC_P256V1
+    pem = key.export(OpenSSL::Cipher.new('AES-128-CBC'), "pass\0wd")
+    assert_raise(ArgumentError) do
+      OpenSSL::PKey.read(pem, "pass")
+    end
+    key2 = OpenSSL::PKey.read(pem, "pass\0wd")
+    assert(key2.private_key?)
+    key3 = OpenSSL::PKey::EC.new(pem, "pass\0wd")
+    assert(key3.private_key?)
+  end
+
   def test_ec_point_mul
     begin
       # y^2 = x^3 + 2x + 2 over F_17
diff --git a/test/openssl/test_pkey_rsa.rb b/test/openssl/test_pkey_rsa.rb
index 54fce2f59e..c3532f637f 100644
--- a/test/openssl/test_pkey_rsa.rb
+++ b/test/openssl/test_pkey_rsa.rb
@@ -276,6 +276,24 @@ AwEAAQ==
     assert(pem)
   end
 
+  def test_export_password_funny
+    key = OpenSSL::TestUtils::TEST_KEY_RSA1024
+    # this assertion may fail in the future because of OpenSSL change.
+    # the current upper limit is 1024
+    assert_raise(OpenSSL::OpenSSLError) do
+      key.export(OpenSSL::Cipher.new('AES-128-CBC'), 'secr' * 1024)
+    end
+    # password containing NUL byte
+    pem = key.export(OpenSSL::Cipher.new('AES-128-CBC'), "pass\0wd")
+    assert_raise(ArgumentError) do
+      OpenSSL::PKey.read(pem, "pass")
+    end
+    key2 = OpenSSL::PKey.read(pem, "pass\0wd")
+    assert(key2.private?)
+    key3 = OpenSSL::PKey::RSA.new(pem, "pass\0wd")
+    assert(key3.private?)
+  end
+
   private
 
   def check_PUBKEY(asn1, key)