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)

Author: rhenium

ext/openssl/ossl_pkcs7.c

@@ -330,27 +330,25 @@ 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;
 
     if(rb_scan_args(argc, argv, "01", &arg) == 0)
 	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);
 

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;
-
-	if (RDATA(self)->data)
-		ossl_raise(eSSLSession, "SSL Session already initialized");
-
-	if (rb_obj_is_instance_of(arg1, cSSLSocket)) {
-		SSL *ssl;
-
-		GetSSL(arg1, ssl);
-
-		if ((ctx = SSL_get1_session(ssl)) == NULL)
-			ossl_raise(eSSLSession, "no session available");
-	} else {
-		BIO *in = ossl_obj2bio(&arg1);
+    SSL_SESSION *ctx;
 
-		ctx = PEM_read_bio_SSL_SESSION(in, NULL, NULL, NULL);
+    if (RTYPEDDATA_DATA(self))
+        ossl_raise(eSSLSession, "SSL Session already initialized");
 
-		if (!ctx) {
-		        OSSL_BIO_reset(in);
-			ctx = d2i_SSL_SESSION_bio(in, NULL);
-		}
+    if (rb_obj_is_instance_of(arg1, cSSLSocket)) {
+        SSL *ssl;
 
-		BIO_free(in);
+        GetSSL(arg1, ssl);
 
-		if (!ctx)
-			ossl_raise(rb_eArgError, "unknown type");
-	}
+        if ((ctx = SSL_get1_session(ssl)) == NULL)
+            ossl_raise(eSSLSession, "no session available");
+    }
+    else {
+        BIO *in = ossl_obj2bio(&arg1);
 
-	/* should not happen */
-	if (ctx == NULL)
-		ossl_raise(eSSLSession, "ctx not set - internal error");
+        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");
+    }
 
-	RDATA(self)->data = ctx;
+    RTYPEDDATA_DATA(self) = ctx;
 
-	return self;
+    return self;
 }
 
 static VALUE

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;
 }

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;
 }

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;
 }

test/openssl/test_x509cert.rb

@@ -245,6 +245,18 @@ def test_read_from_file
     }
   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,