Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: fix to check ext method for shared lib #800

Closed
wants to merge 1 commit into from

Conversation

shigeki
Copy link
Contributor

@shigeki shigeki commented Feb 11, 2015

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

@shigeki shigeki added the tls Issues and PRs related to the tls subsystem. label Feb 11, 2015
@bnoordhuis
Copy link
Member

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
Copy link
Member

indutny commented Feb 11, 2015

Good tip, Ben!

@shigeki
Copy link
Contributor Author

shigeki commented Feb 11, 2015

@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.

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 method of NID_subject_alt_name

This patch originally was created by Fedor Indutny
and Ben Noordhuis

Fixes: nodejs#617
@bnoordhuis
Copy link
Member

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
Copy link
Member

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

@indutny
Copy link
Member

indutny commented Feb 11, 2015

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

@bnoordhuis
Copy link
Member

I think so. Better too strict than too loose.

@shigeki
Copy link
Contributor Author

shigeki commented Feb 11, 2015

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

@indutny
Copy link
Member

indutny commented Feb 11, 2015

Ok, please update it

shigeki pushed a commit that referenced this pull request Feb 11, 2015
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 method of NID_subject_alt_name

This patch originally was created by Fedor Indutny
and Ben Noordhuis

Fixes: #617
PR-URL: #800

Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
@shigeki
Copy link
Contributor Author

shigeki commented Feb 11, 2015

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

@bnoordhuis
Copy link
Member

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
Copy link
Member

indutny commented Feb 11, 2015

@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
Copy link
Contributor Author

shigeki commented Feb 11, 2015

I agree with @indutny

@indutny
Copy link
Member

indutny commented Feb 11, 2015

Good job @shigeki !

@bnoordhuis
Copy link
Member

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

@rvagg rvagg mentioned this pull request Feb 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants