crypto: fix to check ext method for shared lib

[shigeki]

In the case of using openssl with shared library, reinterpret_cast<X509V3_EXT_I2V>(i2v_GENERAL_NAMES)) refers plt pointer so that SafeX509ExtPrint returns false.

Fix it to check it with nid of ext method.

This patch originally was created by Fedor Indutny.

Fixes: #617

Reviewer: @indutny @bnoordhuis

[bnoordhuis]

LGTM but perhaps it can be further simplified?

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 5432eae..c9d1cf1 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -1099,15 +1099,8 @@ void SSLWrap<Base>::OnClientHello(void* arg,


 static bool SafeX509ExtPrint(BIO* out, X509_EXTENSION* ext) {
-  // Only alt_name is escaped at the moment
-  if (OBJ_obj2nid(ext->object) != NID_subject_alt_name)
-    return false;
-
   const X509V3_EXT_METHOD* method = X509V3_EXT_get(ext);
-  if (method == NULL || method->it == NULL)
-    return false;
-
-  if (method->i2v != reinterpret_cast<X509V3_EXT_I2V>(i2v_GENERAL_NAMES))
+  if (method != X509V3_EXT_get_nid(NID_subject_alt_name))
     return false;

   const unsigned char* p = ext->value->data;

[indutny]

Good tip, Ben!

[shigeki]

@bnoordhuis It seems to me that
method != X509V3_EXT_get_nid(NID_subject_alt_name)
is equivalent to
method !=NULL || method->ext_nid != NID_subject_alt_name
and removing the first checks before this. Is there any difference?
And yes, I agree that it can be simplified.

Edit:
Sorry I mistook
method ==NULL || method->ext_nid != NID_subject_alt_name
That means your changes are better.

[bnoordhuis]

Is there any difference?

Functionally? I don't think so. I used X509V3_EXT_get_nid because I'm not 100% sure if ext_nid is intended for public consumption. Your approach is probably fine, though.

[bnoordhuis]

I see you updated the PR. LGTM. :-)

[indutny]

Guys, I have a question for you. Do we care about NULL char in NID_issuer_alt_name and NID_certificate_issuer ?

[bnoordhuis]

I think so. Better too strict than too loose.

[shigeki]

Yes, ideally we should care all the fields. It will be better to another fixes.

[indutny]

Ok, please update it

[shigeki]

Okay, landed in e63b517 . I will submit an another issue. Thanks for reviewing and discussion.

[bnoordhuis]

Should we do the responsible thing here and ask for a CVE? Or at least get in touch with packagers and tell them they need to apply this patch when building with --shared-openssl?

Also, /cc @tjfontaine and/or @misterdjules - this most likely affects joyent/node too.

[indutny]

@bnoordhuis I don't think that we should. Again this issue is very unlikely to be exploitable, because all CA issued certs are validated now.

Though, it won't harm to ask people to update :)

[shigeki]

I agree with @indutny

[indutny]

Good job @shigeki !

[bnoordhuis]

Okay, I'll let joyent/node deal with, that's what's still shipped in Fedora and Debian.