ruby-changes:43013
From: rhe <ko1@a...>
Date: Sat, 21 May 2016 00:05:29 +0900 (JST)
Subject: [ruby-changes:43013] rhe:r55087 (trunk): openssl: improve handling of password for encrypted PEM
rhe 2016-05-21 00:05:25 +0900 (Sat, 21 May 2016) New Revision: 55087 https://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=revision&revision=55087 Log: 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. Modified files: trunk/ChangeLog trunk/ext/openssl/ossl.c trunk/ext/openssl/ossl.h trunk/ext/openssl/ossl_pkey.c trunk/ext/openssl/ossl_pkey_dsa.c trunk/ext/openssl/ossl_pkey_ec.c trunk/ext/openssl/ossl_pkey_rsa.c trunk/test/openssl/test_pkey_dsa.rb trunk/test/openssl/test_pkey_ec.rb trunk/test/openssl/test_pkey_rsa.rb Index: ext/openssl/ossl_pkey_rsa.c =================================================================== --- ext/openssl/ossl_pkey_rsa.c (revision 55086) +++ ext/openssl/ossl_pkey_rsa.c (revision 55087) @@ -210,7 +210,6 @@ ossl_rsa_initialize(int argc, VALUE *arg https://github.com/ruby/ruby/blob/trunk/ext/openssl/ossl_pkey_rsa.c#L210 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 *arg https://github.com/ruby/ruby/blob/trunk/ext/openssl/ossl_pkey_rsa.c#L221 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, V https://github.com/ruby/ruby/blob/trunk/ext/openssl/ossl_pkey_rsa.c#L309 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, V https://github.com/ruby/ruby/blob/trunk/ext/openssl/ossl_pkey_rsa.c#L317 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); } Index: ext/openssl/ossl_pkey.c =================================================================== --- ext/openssl/ossl_pkey.c (revision 55086) +++ ext/openssl/ossl_pkey.c (revision 55087) @@ -152,27 +152,21 @@ ossl_pkey_new_from_file(VALUE filename) https://github.com/ruby/ruby/blob/trunk/ext/openssl/ossl_pkey.c#L152 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); } } } Index: ext/openssl/ossl_pkey_ec.c =================================================================== --- ext/openssl/ossl_pkey_ec.c (revision 55086) +++ ext/openssl/ossl_pkey_ec.c (revision 55087) @@ -168,7 +168,6 @@ static VALUE ossl_ec_key_initialize(int https://github.com/ruby/ruby/blob/trunk/ext/openssl/ossl_pkey_ec.c#L168 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 https://github.com/ruby/ruby/blob/trunk/ext/openssl/ossl_pkey_ec.c#L187 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 https://github.com/ruby/ruby/blob/trunk/ext/openssl/ossl_pkey_ec.c#L472 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 https://github.com/ruby/ruby/blob/trunk/ext/openssl/ossl_pkey_ec.c#L491 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); } Index: ext/openssl/ossl.c =================================================================== --- ext/openssl/ossl.c (revision 55086) +++ ext/openssl/ossl.c (revision 55087) @@ -147,6 +147,31 @@ ossl_buf2str(char *buf, int len) https://github.com/ruby/ruby/blob/trunk/ext/openssl/ossl.c#L147 /* * 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) https://github.com/ruby/ruby/blob/trunk/ext/openssl/ossl.c#L184 } 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_le https://github.com/ruby/ruby/blob/trunk/ext/openssl/ossl.c#L222 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); Index: ext/openssl/ossl.h =================================================================== --- ext/openssl/ossl.h (revision 55086) +++ ext/openssl/ossl.h (revision 55087) @@ -77,11 +77,6 @@ extern "C" { https://github.com/ruby/ruby/blob/trunk/ext/openssl/ossl.h#L77 # 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{\ https://github.com/ruby/ruby/blob/trunk/ext/openssl/ossl.h#L141 }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 *); /* Index: ext/openssl/ossl_pkey_dsa.c =================================================================== --- ext/openssl/ossl_pkey_dsa.c (revision 55086) +++ ext/openssl/ossl_pkey_dsa.c (revision 55087) @@ -216,7 +216,6 @@ ossl_dsa_initialize(int argc, VALUE *arg https://github.com/ruby/ruby/blob/trunk/ext/openssl/ossl_pkey_dsa.c#L216 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 *arg https://github.com/ruby/ruby/blob/trunk/ext/openssl/ossl_pkey_dsa.c#L228 } } 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, V https://github.com/ruby/ruby/blob/trunk/ext/openssl/ossl_pkey_dsa.c#L319 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); } Index: test/openssl/test_pkey_rsa.rb =================================================================== --- test/openssl/test_pkey_rsa.rb (revision 55086) +++ test/openssl/test_pkey_rsa.rb (revision 55087) @@ -276,6 +276,24 @@ AwEAAQ== https://github.com/ruby/ruby/blob/trunk/test/openssl/test_pkey_rsa.rb#L276 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) Index: test/openssl/test_pkey_ec.rb =================================================================== --- test/openssl/test_pkey_ec.rb (revision 55086) +++ test/openssl/test_pkey_ec.rb (revision 55087) @@ -184,6 +184,18 @@ class OpenSSL::TestEC < OpenSSL::TestCas https://github.com/ruby/ruby/blob/trunk/test/openssl/test_pkey_ec.rb#L184 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 Index: test/openssl/test_pkey_dsa.rb =================================================================== --- test/openssl/test_pkey_dsa.rb (revision 55086) +++ test/openssl/test_pkey_dsa.rb (revision 55087) @@ -218,6 +218,18 @@ YNMbNw== https://github.com/ruby/ruby/blob/trunk/test/openssl/test_pkey_dsa.rb#L218 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) Index: ChangeLog =================================================================== --- ChangeLog (revision 55086) +++ ChangeLog (revision 55087) @@ -1,3 +1,30 @@ https://github.com/ruby/ruby/blob/trunk/ChangeLog#L1 +Fri May 20 23:25:42 2016 Kazuki Yamaguchi <k@r...> + + * 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@g...> * id_table.c (list_id_table_init): When unaligned word access is -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/