From a01daab656a3d32b52bd236503e3d9aebaf39483 Mon Sep 17 00:00:00 2001 From: Kazuki Yamaguchi Date: Wed, 19 May 2021 17:58:18 +0900 Subject: [PATCH] [ruby/openssl] x509, ssl, pkcs7: try to parse as DER-encoding first Methods that take both PEM-encoding and DER-encoding have not been consistent in the order in which encoding to attempt to parse. A DER-encoding may contain a valid PEM block ("\n-----BEGIN ..-----" to "-----END ...-----") embedded within it. Also, the PEM-encoding parser allows arbitrary data around the PEM block and silently skips it. As a result, attempting to parse data in DER-encoding as PEM-encoding first can incorrectly finds the embedded PEM block instead. This commit ensures that DER encoding will always be attempted before PEM encoding. OpenSSL::X509::Certificate is one of the updated classes. With this, the following will always be true: # obj is an OpenSSL::X509::Certificate obj == OpenSSL::X509::Certificate.new(obj.to_der) obj == OpenSSL::X509::Certificate.new(obj.to_pem) https://github.com/ruby/openssl/commit/b280eb1fd0 --- ext/openssl/ossl_pkcs7.c | 20 ++++++------- ext/openssl/ossl_ssl_session.c | 53 +++++++++++++++------------------- ext/openssl/ossl_x509cert.c | 17 ++++++----- ext/openssl/ossl_x509crl.c | 17 ++++++----- ext/openssl/ossl_x509req.c | 17 ++++++----- test/openssl/test_x509cert.rb | 12 ++++++++ 6 files changed, 75 insertions(+), 61 deletions(-) diff --git a/ext/openssl/ossl_pkcs7.c b/ext/openssl/ossl_pkcs7.c index 0bcc76a9fd..dbe5347639 100644 --- a/ext/openssl/ossl_pkcs7.c +++ b/ext/openssl/ossl_pkcs7.c @@ -330,7 +330,7 @@ ossl_pkcs7_alloc(VALUE klass) static VALUE ossl_pkcs7_initialize(int argc, VALUE *argv, VALUE self) { - PKCS7 *p7, *pkcs = DATA_PTR(self); + PKCS7 *p7, *p7_orig = RTYPEDDATA_DATA(self); BIO *in; VALUE arg; @@ -338,19 +338,17 @@ ossl_pkcs7_initialize(int argc, VALUE *argv, VALUE self) return self; arg = ossl_to_der_if_possible(arg); in = ossl_obj2bio(&arg); - p7 = PEM_read_bio_PKCS7(in, &pkcs, NULL, NULL); + p7 = d2i_PKCS7_bio(in, NULL); if (!p7) { - OSSL_BIO_reset(in); - p7 = d2i_PKCS7_bio(in, &pkcs); - if (!p7) { - BIO_free(in); - PKCS7_free(pkcs); - DATA_PTR(self) = NULL; - ossl_raise(rb_eArgError, "Could not parse the PKCS7"); - } + OSSL_BIO_reset(in); + p7 = PEM_read_bio_PKCS7(in, NULL, NULL, NULL); } - DATA_PTR(self) = pkcs; BIO_free(in); + if (!p7) + ossl_raise(rb_eArgError, "Could not parse the PKCS7"); + + RTYPEDDATA_DATA(self) = p7; + PKCS7_free(p7_orig); ossl_pkcs7_set_data(self, Qnil); ossl_pkcs7_set_err_string(self, Qnil); diff --git a/ext/openssl/ossl_ssl_session.c b/ext/openssl/ossl_ssl_session.c index 5514087387..92eb1365fe 100644 --- a/ext/openssl/ossl_ssl_session.c +++ b/ext/openssl/ossl_ssl_session.c @@ -34,43 +34,38 @@ static VALUE ossl_ssl_session_alloc(VALUE klass) * Creates a new Session object from an instance of SSLSocket or DER/PEM encoded * String. */ -static VALUE ossl_ssl_session_initialize(VALUE self, VALUE arg1) +static VALUE +ossl_ssl_session_initialize(VALUE self, VALUE arg1) { - SSL_SESSION *ctx = NULL; + SSL_SESSION *ctx; - if (RDATA(self)->data) - ossl_raise(eSSLSession, "SSL Session already initialized"); + if (RTYPEDDATA_DATA(self)) + ossl_raise(eSSLSession, "SSL Session already initialized"); - if (rb_obj_is_instance_of(arg1, cSSLSocket)) { - SSL *ssl; + if (rb_obj_is_instance_of(arg1, cSSLSocket)) { + SSL *ssl; - GetSSL(arg1, ssl); + GetSSL(arg1, ssl); - if ((ctx = SSL_get1_session(ssl)) == NULL) - ossl_raise(eSSLSession, "no session available"); - } else { - BIO *in = ossl_obj2bio(&arg1); + if ((ctx = SSL_get1_session(ssl)) == NULL) + ossl_raise(eSSLSession, "no session available"); + } + else { + BIO *in = ossl_obj2bio(&arg1); - ctx = PEM_read_bio_SSL_SESSION(in, NULL, NULL, NULL); + ctx = d2i_SSL_SESSION_bio(in, NULL); + if (!ctx) { + OSSL_BIO_reset(in); + ctx = PEM_read_bio_SSL_SESSION(in, NULL, NULL, NULL); + } + BIO_free(in); + if (!ctx) + ossl_raise(rb_eArgError, "unknown type"); + } - if (!ctx) { - OSSL_BIO_reset(in); - ctx = d2i_SSL_SESSION_bio(in, NULL); - } + RTYPEDDATA_DATA(self) = ctx; - BIO_free(in); - - if (!ctx) - ossl_raise(rb_eArgError, "unknown type"); - } - - /* should not happen */ - if (ctx == NULL) - ossl_raise(eSSLSession, "ctx not set - internal error"); - - RDATA(self)->data = ctx; - - return self; + return self; } static VALUE diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c index 5376bff08d..1385411069 100644 --- a/ext/openssl/ossl_x509cert.c +++ b/ext/openssl/ossl_x509cert.c @@ -115,24 +115,27 @@ static VALUE ossl_x509_initialize(int argc, VALUE *argv, VALUE self) { BIO *in; - X509 *x509, *x = DATA_PTR(self); + X509 *x509, *x509_orig = RTYPEDDATA_DATA(self); VALUE arg; + rb_check_frozen(self); if (rb_scan_args(argc, argv, "01", &arg) == 0) { /* create just empty X509Cert */ return self; } arg = ossl_to_der_if_possible(arg); in = ossl_obj2bio(&arg); - x509 = PEM_read_bio_X509(in, &x, NULL, NULL); - DATA_PTR(self) = x; + x509 = d2i_X509_bio(in, NULL); if (!x509) { - OSSL_BIO_reset(in); - x509 = d2i_X509_bio(in, &x); - DATA_PTR(self) = x; + OSSL_BIO_reset(in); + x509 = PEM_read_bio_X509(in, NULL, NULL, NULL); } BIO_free(in); - if (!x509) ossl_raise(eX509CertError, NULL); + if (!x509) + ossl_raise(eX509CertError, "PEM_read_bio_X509"); + + RTYPEDDATA_DATA(self) = x509; + X509_free(x509_orig); return self; } diff --git a/ext/openssl/ossl_x509crl.c b/ext/openssl/ossl_x509crl.c index b0badf45c4..863f0286c0 100644 --- a/ext/openssl/ossl_x509crl.c +++ b/ext/openssl/ossl_x509crl.c @@ -93,23 +93,26 @@ static VALUE ossl_x509crl_initialize(int argc, VALUE *argv, VALUE self) { BIO *in; - X509_CRL *crl, *x = DATA_PTR(self); + X509_CRL *crl, *crl_orig = RTYPEDDATA_DATA(self); VALUE arg; + rb_check_frozen(self); if (rb_scan_args(argc, argv, "01", &arg) == 0) { return self; } arg = ossl_to_der_if_possible(arg); in = ossl_obj2bio(&arg); - crl = PEM_read_bio_X509_CRL(in, &x, NULL, NULL); - DATA_PTR(self) = x; + crl = d2i_X509_CRL_bio(in, NULL); if (!crl) { - OSSL_BIO_reset(in); - crl = d2i_X509_CRL_bio(in, &x); - DATA_PTR(self) = x; + OSSL_BIO_reset(in); + crl = PEM_read_bio_X509_CRL(in, NULL, NULL, NULL); } BIO_free(in); - if (!crl) ossl_raise(eX509CRLError, NULL); + if (!crl) + ossl_raise(eX509CRLError, "PEM_read_bio_X509_CRL"); + + RTYPEDDATA_DATA(self) = crl; + X509_CRL_free(crl_orig); return self; } diff --git a/ext/openssl/ossl_x509req.c b/ext/openssl/ossl_x509req.c index 2c20042a92..6eb91e9c2f 100644 --- a/ext/openssl/ossl_x509req.c +++ b/ext/openssl/ossl_x509req.c @@ -79,23 +79,26 @@ static VALUE ossl_x509req_initialize(int argc, VALUE *argv, VALUE self) { BIO *in; - X509_REQ *req, *x = DATA_PTR(self); + X509_REQ *req, *req_orig = RTYPEDDATA_DATA(self); VALUE arg; + rb_check_frozen(self); if (rb_scan_args(argc, argv, "01", &arg) == 0) { return self; } arg = ossl_to_der_if_possible(arg); in = ossl_obj2bio(&arg); - req = PEM_read_bio_X509_REQ(in, &x, NULL, NULL); - DATA_PTR(self) = x; + req = d2i_X509_REQ_bio(in, NULL); if (!req) { - OSSL_BIO_reset(in); - req = d2i_X509_REQ_bio(in, &x); - DATA_PTR(self) = x; + OSSL_BIO_reset(in); + req = PEM_read_bio_X509_REQ(in, NULL, NULL, NULL); } BIO_free(in); - if (!req) ossl_raise(eX509ReqError, NULL); + if (!req) + ossl_raise(eX509ReqError, "PEM_read_bio_X509_REQ"); + + RTYPEDDATA_DATA(self) = req; + X509_REQ_free(req_orig); return self; } diff --git a/test/openssl/test_x509cert.rb b/test/openssl/test_x509cert.rb index 848a314c9f..70fe9d4419 100644 --- a/test/openssl/test_x509cert.rb +++ b/test/openssl/test_x509cert.rb @@ -245,6 +245,18 @@ class OpenSSL::TestX509Certificate < OpenSSL::TestCase } end + def test_read_der_then_pem + cert1 = issue_cert(@ca, @rsa2048, 1, [], nil, nil) + exts = [ + # A new line before PEM block + ["nsComment", "Another certificate:\n" + cert1.to_pem], + ] + cert2 = issue_cert(@ca, @rsa2048, 2, exts, nil, nil) + + assert_equal cert2, OpenSSL::X509::Certificate.new(cert2.to_der) + assert_equal cert2, OpenSSL::X509::Certificate.new(cert2.to_pem) + end + def test_eq now = Time.now cacert = issue_cert(@ca, @rsa1024, 1, [], nil, nil,