[ruby/openssl] asn1: refactor converting ASN1_OBJECT to string

ruby/openssl exposes OIDs to Ruby as strings in many places, but the
conversion logic has been duplicated and the behavior is inconsistent.
There are mainly two patterns:

 - Returns the short name associated with the OID/NID, or the dotted
   decimal notation if it is unknown to OpenSSL.
 - Returns the long name, or the dotted decimal notation.

These patterns are implemented using different OpenSSL APIs and that
caused subtle differences. Add helper functions ossl_asn1obj_to_string()
and ossl_asn1obj_to_string_long_name() to unify the logic.

Also, document the current behaviors where it is not yet done. The
inconsistency was likely unintentional, but since it dates back to the
original implementations, standardizing it now would cause more issues
than it resolves.

https://github.com/ruby/openssl/commit/2ea36c21a4

15 files changed, 139 insertions, 160 deletions

diff --git a/ext/openssl/ossl_asn1.c b/ext/openssl/ossl_asn1.c
index 58efe96f2a..1309811c74 100644
--- a/ext/openssl/ossl_asn1.c
+++ b/ext/openssl/ossl_asn1.c
@@ -147,6 +147,48 @@ asn1integer_to_num_i(VALUE arg)
     return asn1integer_to_num((ASN1_INTEGER *)arg);
 }
 
+/*
+ * ASN1_OBJECT conversions
+ */
+VALUE
+ossl_asn1obj_to_string_oid(const ASN1_OBJECT *a1obj)
+{
+    VALUE str;
+    int len;
+
+    str = rb_usascii_str_new(NULL, 127);
+    len = OBJ_obj2txt(RSTRING_PTR(str), RSTRING_LENINT(str), a1obj, 1);
+    if (len <= 0 || len == INT_MAX)
+	ossl_raise(eOSSLError, "OBJ_obj2txt");
+    if (len > RSTRING_LEN(str)) {
+	/* +1 is for the \0 terminator added by OBJ_obj2txt() */
+	rb_str_resize(str, len + 1);
+	len = OBJ_obj2txt(RSTRING_PTR(str), len + 1, a1obj, 1);
+	if (len <= 0)
+	    ossl_raise(eOSSLError, "OBJ_obj2txt");
+    }
+    rb_str_set_len(str, len);
+    return str;
+}
+
+VALUE
+ossl_asn1obj_to_string(const ASN1_OBJECT *obj)
+{
+    int nid = OBJ_obj2nid(obj);
+    if (nid != NID_undef)
+        return rb_str_new_cstr(OBJ_nid2sn(nid));
+    return ossl_asn1obj_to_string_oid(obj);
+}
+
+VALUE
+ossl_asn1obj_to_string_long_name(const ASN1_OBJECT *obj)
+{
+    int nid = OBJ_obj2nid(obj);
+    if (nid != NID_undef)
+        return rb_str_new_cstr(OBJ_nid2ln(nid));
+    return ossl_asn1obj_to_string_oid(obj);
+}
+
 /********/
 /*
  * ASN1 module
@@ -393,32 +435,27 @@ decode_null(unsigned char* der, long length)
     return Qnil;
 }
 
+VALUE
+asn1obj_to_string_i(VALUE arg)
+{
+    return ossl_asn1obj_to_string((const ASN1_OBJECT *)arg);
+}
+
 static VALUE
 decode_obj(unsigned char* der, long length)
 {
     ASN1_OBJECT *obj;
     const unsigned char *p;
     VALUE ret;
-    int nid;
-    BIO *bio;
+    int state;
 
     p = der;
-    if(!(obj = d2i_ASN1_OBJECT(NULL, &p, length)))
-	ossl_raise(eASN1Error, NULL);
-    if((nid = OBJ_obj2nid(obj)) != NID_undef){
-	ASN1_OBJECT_free(obj);
-	ret = rb_str_new2(OBJ_nid2sn(nid));
-    }
-    else{
-	if(!(bio = BIO_new(BIO_s_mem()))){
-	    ASN1_OBJECT_free(obj);
-	    ossl_raise(eASN1Error, NULL);
-	}
-	i2a_ASN1_OBJECT(bio, obj);
-	ASN1_OBJECT_free(obj);
-	ret = ossl_membio2str(bio);
-    }
-
+    if (!(obj = d2i_ASN1_OBJECT(NULL, &p, length)))
+        ossl_raise(eASN1Error, "d2i_ASN1_OBJECT");
+    ret = rb_protect(asn1obj_to_string_i, (VALUE)obj, &state);
+    ASN1_OBJECT_free(obj);
+    if (state)
+        rb_jump_tag(state);
     return ret;
 }
 
@@ -1172,23 +1209,7 @@ ossl_asn1obj_get_ln(VALUE self)
 static VALUE
 asn1obj_get_oid_i(VALUE vobj)
 {
-    ASN1_OBJECT *a1obj = (void *)vobj;
-    VALUE str;
-    int len;
-
-    str = rb_usascii_str_new(NULL, 127);
-    len = OBJ_obj2txt(RSTRING_PTR(str), RSTRING_LENINT(str), a1obj, 1);
-    if (len <= 0 || len == INT_MAX)
-	ossl_raise(eASN1Error, "OBJ_obj2txt");
-    if (len > RSTRING_LEN(str)) {
-	/* +1 is for the \0 terminator added by OBJ_obj2txt() */
-	rb_str_resize(str, len + 1);
-	len = OBJ_obj2txt(RSTRING_PTR(str), len + 1, a1obj, 1);
-	if (len <= 0)
-	    ossl_raise(eASN1Error, "OBJ_obj2txt");
-    }
-    rb_str_set_len(str, len);
-    return str;
+    return ossl_asn1obj_to_string_oid((const ASN1_OBJECT *)vobj);
 }
 
 /*
diff --git a/ext/openssl/ossl_asn1.h b/ext/openssl/ossl_asn1.h
index a723b06f60..b605df8f3f 100644
--- a/ext/openssl/ossl_asn1.h
+++ b/ext/openssl/ossl_asn1.h
@@ -35,6 +35,16 @@ ASN1_INTEGER *num_to_asn1integer(VALUE, ASN1_INTEGER *);
  * ASN1_OBJECT conversions
  */
 ASN1_OBJECT *ossl_to_asn1obj(VALUE obj);
+/*
+ * Returns the short name if available, the dotted decimal notation otherwise.
+ * This is the most common way to return ASN1_OBJECT to Ruby.
+ */
+VALUE ossl_asn1obj_to_string(const ASN1_OBJECT *a1obj);
+/*
+ * However, some places use long names instead. This is likely unintentional,
+ * but we keep the current behavior in existing methods.
+ */
+VALUE ossl_asn1obj_to_string_long_name(const ASN1_OBJECT *a1obj);
 
 /*
  * ASN1 module
diff --git a/ext/openssl/ossl_ocsp.c b/ext/openssl/ossl_ocsp.c
index 84d38760e5..390215a8e3 100644
--- a/ext/openssl/ossl_ocsp.c
+++ b/ext/openssl/ossl_ocsp.c
@@ -1591,19 +1591,10 @@ ossl_ocspcid_get_hash_algorithm(VALUE self)
 {
     OCSP_CERTID *id;
     ASN1_OBJECT *oid;
-    BIO *out;
 
     GetOCSPCertId(self, id);
     OCSP_id_get0_info(NULL, &oid, NULL, NULL, id);
-
-    if (!(out = BIO_new(BIO_s_mem())))
-	ossl_raise(eOCSPError, "BIO_new");
-
-    if (!i2a_ASN1_OBJECT(out, oid)) {
-	BIO_free(out);
-	ossl_raise(eOCSPError, "i2a_ASN1_OBJECT");
-    }
-    return ossl_membio2str(out);
+    return ossl_asn1obj_to_string_long_name(oid);
 }
 
 /*
diff --git a/ext/openssl/ossl_ts.c b/ext/openssl/ossl_ts.c
index 57d4b2a70e..575418ccc6 100644
--- a/ext/openssl/ossl_ts.c
+++ b/ext/openssl/ossl_ts.c
@@ -139,27 +139,6 @@ obj_to_asn1obj_i(VALUE obj)
 }
 
 static VALUE
-get_asn1obj(const ASN1_OBJECT *obj)
-{
-    BIO *out;
-    VALUE ret;
-    int nid;
-    if ((nid = OBJ_obj2nid(obj)) != NID_undef)
-        ret = rb_str_new2(OBJ_nid2sn(nid));
-    else{
-        if (!(out = BIO_new(BIO_s_mem())))
-            ossl_raise(eTimestampError, "BIO_new(BIO_s_mem())");
-        if (i2a_ASN1_OBJECT(out, obj) <= 0) {
-            BIO_free(out);
-            ossl_raise(eTimestampError, "i2a_ASN1_OBJECT");
-        }
-        ret = ossl_membio2str(out);
-    }
-
-    return ret;
-}
-
-static VALUE
 ossl_ts_req_alloc(VALUE klass)
 {
     TS_REQ *req;
@@ -229,7 +208,7 @@ ossl_ts_req_get_algorithm(VALUE self)
     mi = TS_REQ_get_msg_imprint(req);
     algor = TS_MSG_IMPRINT_get_algo(mi);
     X509_ALGOR_get0(&obj, NULL, NULL, algor);
-    return get_asn1obj(obj);
+    return ossl_asn1obj_to_string(obj);
 }
 
 /*
@@ -358,7 +337,7 @@ ossl_ts_req_get_policy_id(VALUE self)
     GetTSRequest(self, req);
     if (!TS_REQ_get_policy_id(req))
         return Qnil;
-    return get_asn1obj(TS_REQ_get_policy_id(req));
+    return ossl_asn1obj_to_string(TS_REQ_get_policy_id(req));
 }
 
 /*
@@ -948,7 +927,7 @@ ossl_ts_token_info_get_policy_id(VALUE self)
     TS_TST_INFO *info;
 
     GetTSTokenInfo(self, info);
-    return get_asn1obj(TS_TST_INFO_get_policy_id(info));
+    return ossl_asn1obj_to_string(TS_TST_INFO_get_policy_id(info));
 }
 
 /*
@@ -976,7 +955,7 @@ ossl_ts_token_info_get_algorithm(VALUE self)
     mi = TS_TST_INFO_get_msg_imprint(info);
     algo = TS_MSG_IMPRINT_get_algo(mi);
     X509_ALGOR_get0(&obj, NULL, NULL, algo);
-    return get_asn1obj(obj);
+    return ossl_asn1obj_to_string(obj);
 }
 
 /*
diff --git a/ext/openssl/ossl_x509attr.c b/ext/openssl/ossl_x509attr.c
index d983af5968..9beb15f4a0 100644
--- a/ext/openssl/ossl_x509attr.c
+++ b/ext/openssl/ossl_x509attr.c
@@ -164,29 +164,18 @@ ossl_x509attr_set_oid(VALUE self, VALUE oid)
 
 /*
  * call-seq:
- *    attr.oid => string
+ *    attr.oid -> string
+ *
+ * Returns the OID of the attribute. Returns the short name or the dotted
+ * decimal notation.
  */
 static VALUE
 ossl_x509attr_get_oid(VALUE self)
 {
     X509_ATTRIBUTE *attr;
-    ASN1_OBJECT *oid;
-    BIO *out;
-    VALUE ret;
-    int nid;
 
     GetX509Attr(self, attr);
-    oid = X509_ATTRIBUTE_get0_object(attr);
-    if ((nid = OBJ_obj2nid(oid)) != NID_undef)
-	ret = rb_str_new2(OBJ_nid2sn(nid));
-    else{
-	if (!(out = BIO_new(BIO_s_mem())))
-	    ossl_raise(eX509AttrError, NULL);
-	i2a_ASN1_OBJECT(out, oid);
-	ret = ossl_membio2str(out);
-    }
-
-    return ret;
+    return ossl_asn1obj_to_string(X509_ATTRIBUTE_get0_object(attr));
 }
 
 /*
diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c
index c7653031b4..cf00eb78bf 100644
--- a/ext/openssl/ossl_x509cert.c
+++ b/ext/openssl/ossl_x509cert.c
@@ -318,27 +318,23 @@ ossl_x509_set_serial(VALUE self, VALUE num)
 /*
  * call-seq:
  *    cert.signature_algorithm => string
+ *
+ * Returns the signature algorithm used to sign this certificate. This returns
+ * the algorithm name found in the TBSCertificate structure, not the outer
+ * \Certificate structure.
+ *
+ * Returns the long name of the signature algorithm, or the dotted decimal
+ * notation if \OpenSSL does not define a long name for it.
  */
 static VALUE
 ossl_x509_get_signature_algorithm(VALUE self)
 {
     X509 *x509;
-    BIO *out;
     const ASN1_OBJECT *obj;
-    VALUE str;
 
     GetX509(self, x509);
-    out = BIO_new(BIO_s_mem());
-    if (!out) ossl_raise(eX509CertError, NULL);
-
     X509_ALGOR_get0(&obj, NULL, NULL, X509_get0_tbs_sigalg(x509));
-    if (!i2a_ASN1_OBJECT(out, obj)) {
-	BIO_free(out);
-	ossl_raise(eX509CertError, NULL);
-    }
-    str = ossl_membio2str(out);
-
-    return str;
+    return ossl_asn1obj_to_string_long_name(obj);
 }
 
 /*
diff --git a/ext/openssl/ossl_x509crl.c b/ext/openssl/ossl_x509crl.c
index b9ee5f0569..af4803f47d 100644
--- a/ext/openssl/ossl_x509crl.c
+++ b/ext/openssl/ossl_x509crl.c
@@ -166,26 +166,26 @@ ossl_x509crl_set_version(VALUE self, VALUE version)
     return version;
 }
 
+/*
+ * call-seq:
+ *    crl.signature_algorithm -> string
+ *
+ * Returns the signature algorithm used to sign this CRL.
+ *
+ * Returns the long name of the signature algorithm, or the dotted decimal
+ * notation if \OpenSSL does not define a long name for it.
+ */
 static VALUE
 ossl_x509crl_get_signature_algorithm(VALUE self)
 {
     X509_CRL *crl;
     const X509_ALGOR *alg;
     const ASN1_OBJECT *obj;
-    BIO *out;
 
     GetX509CRL(self, crl);
-    if (!(out = BIO_new(BIO_s_mem()))) {
-	ossl_raise(eX509CRLError, NULL);
-    }
     X509_CRL_get0_signature(crl, NULL, &alg);
     X509_ALGOR_get0(&obj, NULL, NULL, alg);
-    if (!i2a_ASN1_OBJECT(out, obj)) {
-	BIO_free(out);
-	ossl_raise(eX509CRLError, NULL);
-    }
-
-    return ossl_membio2str(out);
+    return ossl_asn1obj_to_string_long_name(obj);
 }
 
 static VALUE
diff --git a/ext/openssl/ossl_x509ext.c b/ext/openssl/ossl_x509ext.c
index 01aa3a8f51..01b342b5be 100644
--- a/ext/openssl/ossl_x509ext.c
+++ b/ext/openssl/ossl_x509ext.c
@@ -359,27 +359,20 @@ ossl_x509ext_set_critical(VALUE self, VALUE flag)
     return flag;
 }
 
+/*
+ * call-seq:
+ *    ext.oid -> string
+ *
+ * Returns the OID of the extension. Returns the short name or the dotted
+ * decimal notation.
+ */
 static VALUE
 ossl_x509ext_get_oid(VALUE obj)
 {
     X509_EXTENSION *ext;
-    ASN1_OBJECT *extobj;
-    BIO *out;
-    VALUE ret;
-    int nid;
 
     GetX509Ext(obj, ext);
-    extobj = X509_EXTENSION_get_object(ext);
-    if ((nid = OBJ_obj2nid(extobj)) != NID_undef)
-	ret = rb_str_new2(OBJ_nid2sn(nid));
-    else{
-	if (!(out = BIO_new(BIO_s_mem())))
-	    ossl_raise(eX509ExtError, NULL);
-	i2a_ASN1_OBJECT(out, extobj);
-	ret = ossl_membio2str(out);
-    }
-
-    return ret;
+    return ossl_asn1obj_to_string(X509_EXTENSION_get_object(ext));
 }
 
 static VALUE
diff --git a/ext/openssl/ossl_x509name.c b/ext/openssl/ossl_x509name.c
index 7d0fd35247..5483450aa7 100644
--- a/ext/openssl/ossl_x509name.c
+++ b/ext/openssl/ossl_x509name.c
@@ -341,34 +341,22 @@ static VALUE
 ossl_x509name_to_a(VALUE self)
 {
     X509_NAME *name;
-    X509_NAME_ENTRY *entry;
-    int i,entries,nid;
-    char long_name[512];
-    const char *short_name;
-    VALUE ary, vname, ret;
-    ASN1_STRING *value;
+    int entries;
+    VALUE ret;
 
     GetX509Name(self, name);
     entries = X509_NAME_entry_count(name);
     ret = rb_ary_new_capa(entries);
-    for (i=0; i<entries; i++) {
-	if (!(entry = X509_NAME_get_entry(name, i))) {
-	    ossl_raise(eX509NameError, NULL);
-	}
-	if (!i2t_ASN1_OBJECT(long_name, sizeof(long_name),
-			     X509_NAME_ENTRY_get_object(entry))) {
-	    ossl_raise(eX509NameError, NULL);
-	}
-	nid = OBJ_ln2nid(long_name);
-	if (nid == NID_undef) {
-	    vname = rb_str_new2((const char *) &long_name);
-	} else {
-	    short_name = OBJ_nid2sn(nid);
-	    vname = rb_str_new2(short_name); /*do not free*/
-	}
-	value = X509_NAME_ENTRY_get_data(entry);
-	ary = rb_ary_new3(3, vname, asn1str_to_str(value), INT2NUM(value->type));
-	rb_ary_push(ret, ary);
+    for (int i = 0; i < entries; i++) {
+        const X509_NAME_ENTRY *entry = X509_NAME_get_entry(name, i);
+        if (!entry)
+            ossl_raise(eX509NameError, "X509_NAME_get_entry");
+        const ASN1_OBJECT *obj = X509_NAME_ENTRY_get_object(entry);
+        VALUE vname = ossl_asn1obj_to_string(obj);
+        const ASN1_STRING *data = X509_NAME_ENTRY_get_data(entry);
+        VALUE vdata = asn1str_to_str(data);
+        VALUE type = INT2NUM(ASN1_STRING_type(data));
+        rb_ary_push(ret, rb_ary_new_from_args(3, vname, vdata, type));
     }
     return ret;
 }
diff --git a/ext/openssl/ossl_x509req.c b/ext/openssl/ossl_x509req.c
index eae5796924..1ae0fd56a6 100644
--- a/ext/openssl/ossl_x509req.c
+++ b/ext/openssl/ossl_x509req.c
@@ -255,27 +255,26 @@ ossl_x509req_set_subject(VALUE self, VALUE subject)
     return subject;
 }
 
+/*
+ * call-seq:
+ *    req.signature_algorithm -> string
+ *
+ * Returns the signature algorithm used to sign this request.
+ *
+ * Returns the long name of the signature algorithm, or the dotted decimal
+ * notation if \OpenSSL does not define a long name for it.
+ */
 static VALUE
 ossl_x509req_get_signature_algorithm(VALUE self)
 {
     X509_REQ *req;
     const X509_ALGOR *alg;
     const ASN1_OBJECT *obj;
-    BIO *out;
 
     GetX509Req(self, req);
-
-    if (!(out = BIO_new(BIO_s_mem()))) {
-	ossl_raise(eX509ReqError, NULL);
-    }
     X509_REQ_get0_signature(req, NULL, &alg);
     X509_ALGOR_get0(&obj, NULL, NULL, alg);
-    if (!i2a_ASN1_OBJECT(out, obj)) {
-	BIO_free(out);
-	ossl_raise(eX509ReqError, NULL);
-    }
-
-    return ossl_membio2str(out);
+    return ossl_asn1obj_to_string_long_name(obj);
 }
 
 static VALUE
diff --git a/test/openssl/test_asn1.rb b/test/openssl/test_asn1.rb
index 501e35151f..df8b0accb3 100644
--- a/test/openssl/test_asn1.rb
+++ b/test/openssl/test_asn1.rb
@@ -306,7 +306,11 @@ class  OpenSSL::TestASN1 < OpenSSL::TestCase
   end
 
   def test_object_identifier
-    encode_decode_test B(%w{ 06 01 00 }), OpenSSL::ASN1::ObjectId.new("0.0".b)
+    obj = encode_decode_test B(%w{ 06 01 00 }), OpenSSL::ASN1::ObjectId.new("0.0".b)
+    assert_equal "0.0", obj.oid
+    assert_nil obj.sn
+    assert_nil obj.ln
+    assert_equal obj.oid, obj.value
     encode_decode_test B(%w{ 06 01 28 }), OpenSSL::ASN1::ObjectId.new("1.0".b)
     encode_decode_test B(%w{ 06 03 88 37 03 }), OpenSSL::ASN1::ObjectId.new("2.999.3".b)
     encode_decode_test B(%w{ 06 05 2A 22 83 BB 55 }), OpenSSL::ASN1::ObjectId.new("1.2.34.56789".b)
@@ -314,6 +318,7 @@ class  OpenSSL::TestASN1 < OpenSSL::TestCase
     assert_equal "2.16.840.1.101.3.4.2.1", obj.oid
     assert_equal "SHA256", obj.sn
     assert_equal "sha256", obj.ln
+    assert_equal obj.sn, obj.value
     assert_raise(OpenSSL::ASN1::ASN1Error) {
       OpenSSL::ASN1.decode(B(%w{ 06 00 }))
     }
diff --git a/test/openssl/test_ts.rb b/test/openssl/test_ts.rb
index 7b154d1c37..cca7898bc1 100644
--- a/test/openssl/test_ts.rb
+++ b/test/openssl/test_ts.rb
@@ -88,8 +88,9 @@ _end_of_pem_
     assert_raise(TypeError) { req.version = nil }
     assert_raise(TypeError) { req.version = "foo" }
 
-    req.algorithm = "SHA1"
+    req.algorithm = "sha1"
     assert_equal("SHA1", req.algorithm)
+    assert_equal("SHA1", OpenSSL::ASN1.ObjectId("SHA1").sn)
     assert_raise(TypeError) { req.algorithm = nil }
     assert_raise(OpenSSL::ASN1::ASN1Error) { req.algorithm = "xxx" }
 
diff --git a/test/openssl/test_x509cert.rb b/test/openssl/test_x509cert.rb
index 55481690e9..877eac69ce 100644
--- a/test/openssl/test_x509cert.rb
+++ b/test/openssl/test_x509cert.rb
@@ -236,6 +236,7 @@ class OpenSSL::TestX509Certificate < OpenSSL::TestCase
 
   def test_sign_and_verify
     cert = issue_cert(@ca, @rsa1, 1, [], nil, nil, digest: "SHA256")
+    assert_equal("sha256WithRSAEncryption", cert.signature_algorithm) # ln
     assert_equal(true, cert.verify(@rsa1))
     assert_equal(false, cert.verify(@rsa2))
     assert_equal(false, certificate_error_returns_false { cert.verify(@ec1) })
diff --git a/test/openssl/test_x509crl.rb b/test/openssl/test_x509crl.rb
index 3c364f57d5..81c9247df2 100644
--- a/test/openssl/test_x509crl.rb
+++ b/test/openssl/test_x509crl.rb
@@ -20,6 +20,7 @@ class OpenSSL::TestX509CRL < OpenSSL::TestCase
     assert_equal(cert.issuer.to_der, crl.issuer.to_der)
     assert_equal(now, crl.last_update)
     assert_equal(now+1600, crl.next_update)
+    assert_equal("sha256WithRSAEncryption", crl.signature_algorithm) # ln
 
     crl = OpenSSL::X509::CRL.new(crl.to_der)
     assert_equal(1, crl.version)
diff --git a/test/openssl/test_x509req.rb b/test/openssl/test_x509req.rb
index 0a2df47bca..b198a1185a 100644
--- a/test/openssl/test_x509req.rb
+++ b/test/openssl/test_x509req.rb
@@ -42,6 +42,11 @@ class OpenSSL::TestX509Request < OpenSSL::TestCase
     assert_equal(@dn.to_der, req.subject.to_der)
   end
 
+  def test_signature_algorithm
+    req = issue_csr(0, @dn, @rsa1, "SHA256")
+    assert_equal("sha256WithRSAEncryption", req.signature_algorithm) # ln
+  end
+
   def create_ext_req(exts)
     ef = OpenSSL::X509::ExtensionFactory.new
     exts = exts.collect{|e| ef.create_extension(*e) }