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

Enhance ssl verification to make hostname matching possible #378

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jtulley
Copy link

@jtulley jtulley commented Nov 7, 2012

A critical part of SSL verification is matching the host name of
the certificate, and this is not done automatically by the
OpenSSL libraries. Also, simply calling ssl_verify_peer with
no context but the certificate means that the ruby code needs to
fully reimplement a certificate store and validation. Even then,
there is not enough information to know when to assert the host
name match.

This commit adds "ca-cert-file", and "ca-path", very similar to
the standard libcurl way of handing ca certificates. The current
cert-chain-file is very insufficient since it can only contain
one chain. This patch does not get rid of that, but merely adds
these two new options, and uses them in a call to
SSL_Ctx_load_verify_locations.

Also, ssl_verify_peer is now passed the error code of the
OpenSSL verification that has already occurred, as well as the
certificate depth. At depth 0, the ruby code can do a domain /
host name check.

A critical part of SSL verification is matching the host name of
the certificate, and this is not done automatically by the
OpenSSL libraries.  Also, simply calling ssl_verify_peer with
no context but the certificate means that the ruby code needs to
fully reimplement a certificate store and validation. Even then,
there is not enough information to know when to assert the host
name match.

This commit adds "ca-cert-file", and "ca-path", very similar to
the standard libcurl way of handing ca certificates. The current
cert-chain-file is very insufficient since it can only contain
one chain.  This patch does not get rid of that, but merely adds
these two new options, and uses them in a call to
SSL_Ctx_load_verify_locations.

Also, ssl_verify_peer is now passed the error code of the
OpenSSL verification that has already occurred, as well as the
certificate depth.  At depth 0, the ruby code can do a domain /
host name check.
@jtulley
Copy link
Author

jtulley commented Nov 7, 2012

I realize that there is already a similar pull request, #334 Mine has the following differences:

  1. With mine, I allow you to pass in a full ca-cert file (not a cert chain, which is limited), and / or a capath. So, in this regard it is rather like libcurl or command-line curl. No bundling of certs.

  2. I pass to ssl_verify_peer the pem(as usual), an error code (with "0" / X509_V_OK for succes), and a certificate depth, which is useful for the method to know when to do a domain name match. Then it is completely up to ssl_verify_peer to do the domain name match.

The other pull request is superior in having eventmachine verify the host name. In that way, the developer does not end up skipping this critical step. But, I think the ca-cert-file, and ca-path options are better than bundling a cert file. Bundling a cert file is a non-starter, since certificates need to be maintained. Expiring certificates, revoked certificates, new trusted root certificates all would create a maintenance problem.

@jtulley
Copy link
Author

jtulley commented Nov 7, 2012

I'd like to propose we merge the two, and I'd be willing to do that work if needed.

Besides my concern about pull 334's bundling of certs, I only saw one concern in the discussion on that change -- that was how to match the domain name. Are there reasons why the following would not be sufficient?:

if depth == 0
cert = OpenSSL::X509::Certificate.new(pem) rescue nil
return_val = OpenSSL::SSL.verify_certificate_identity(cert, @Domain) rescue false
end

This would rely on the domain / host name being passed in as per pull 334's technique. (my pull request leaves this completely up to ssl_verify_peer)

@@ -142,7 +150,7 @@ def ssl_handshake_completed
# start_tls(:verify_peer => true)
# end
#
# def ssl_verify_peer(cert)
# def ssl_verify_peer(cert, error, depth)
Copy link
Contributor

@tmm1 tmm1 Nov 7, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break backwards compatibility with any code accepting only one argument.

Copy link
Author

@jtulley jtulley Nov 8, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that was definitely one concern of mine. On the other hand, ssl_verify_peer is pretty much useless at this point.

Copy link
Author

@jtulley jtulley Nov 8, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without any context, there is no way to make a correct determination in ssl_verify_peer, especially whether the domain name match (or lack of a match) is important enough to cause a connection failure. The error, and the depth level are the minimum items required to do this. The other patch, 334, does the host name check internally (which should be enhanced as ibc suggests), but there should still be more information passed to ssl_verify_peer.

@jtulley
Copy link
Author

jtulley commented Nov 7, 2012

One other concern is ensuring that the calling code can figure out what went wrong. So, we'd still need to send enough information to ssl_verify_peer to make this possible.

ext/ed.cpp Outdated

// our event handling mechanism only takes a const char *, but we need to pass back more data than that
// use a struct and some reinterpret casts to pass the information:
(*EventCallback)(GetBinding(), EM_SSL_VERIFY, reinterpret_cast<const char*>(&verify), strlen(cert));
Copy link
Contributor

@tmm1 tmm1 Nov 7, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yuck.

Copy link
Author

@jtulley jtulley Nov 8, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but the mechanism only allows passing one thing back, and this is certainly not sufficient in this case. Everything doesn't fit into a "one string and one string only" methodology. I'm open to alternatives, of course.

Copy link
Author

@jtulley jtulley Nov 8, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to change the event mechanism to support a "void *" and document this as an opaque pointer? Then have the code cast it to one or the other on the other side?
Or
Some sort of object hierarchy, once again with most operations ending up with an object that contains a char *, but others with the enhanced data fields. I don't like this very much, since it ends up being almost the same thing as the other objects, with reinterpret_casts or dynamic_casts there as well.

@ibc
Copy link
Contributor

ibc commented Nov 7, 2012

@jtulley, as I described here and here, the world is bigger than just HTTP. Comparing the certificate Common Name with a given domain is NOT enough in many cases, so please don't try to include such a limited and incorrect comparison at core level since it's the application layer the only that can decide how the certificate must be inspected for validating anything related to the underlying application protocol.

Regards.

@jtulley
Copy link
Author

jtulley commented Nov 8, 2012

Is that OpenSSL verify_certificate_identity call not sufficient?
That might be an argument for the check to be completely done by ssl_verify_peer, like with my pull request. The downside being, of course, that this causes many to completely skip this check, resulting in no real SSL security.

@jtulley
Copy link
Author

jtulley commented Nov 8, 2012

Iñaki, is the license of OverSIP such that we could just copy the validation code from there? Would that be sufficient if we did that, to still do the host checking internally to event machine? I'd still want to augment ssl_verify_peer since the current implementation does not give enough context, and also because sometimes that method will want to ignore host name mismatches. (Or, that could be yet another tls_option).

@ibc
Copy link
Contributor

ibc commented Nov 8, 2012

@jtulley OverSIP has MIT license so yes. But OverSIP implements procedures for validating a certificate following SIP protocol rules. Each application protocol may decide its own rules for inspecting the fields of a certificate. Please don't do that at low level in any way, that's up to the application on top of EventMachine. If you code a XMPP server on top of EM you need another way for inspecting the certificate fields, and so on.

Maybe EventMachine could validate the certificate by checking if it's correctly signed, not expired and so on, that could be a configurable option in start_tls. But please, again, don't try to include the CommonName or AltSubjectName fields of the certificate at C++ level (or within the EM code itself), that's UP to the application being coded on top of EventMachine.

@jtulley
Copy link
Author

jtulley commented Nov 8, 2012

Ok. Well, the good news is that my pull request defers that validation to ssl_verify_peer, but simply provides enough context in that method via the error and depth arguments for that validation to be able to succeed. It was the other pull request that did internal host name validation.

So, that leaves Aman's concerns.

  1. ssl_verify_peer backwards compatibility. I don't think there is any way around this. The current technique of only passing in the pem just isn't sufficient. Currently in ssl_verify_peer, you have no idea whether openssl has already validated the certificate, and you have no idea if you are at a certificate level at which host name validation is possible ("depth 0" certificates).

  2. Passing that increased amount of information from ed.cpp's VerifySslPeer through the EventCallback mechanism to the event_callback method in rubymain.cpp. Currently this mechanism only allows for a const char* and a size. That had been sufficient for all events in the past but once you have to send more than one item through, it is no longer enough. Right now I'm creating a struct that has all the information, casting that to a const char* and then casting it back to the struct on the other side in the EM_SSL_VERIFY case. I'm open to suggestions on alternatives.

Jeff Tulley added 2 commits Nov 8, 2012
Some spacing fixes, and some parentheses to make a statement
more clear.
Instead of trying to change a struct into a const void * then
changing it back in the event_callback of rubymain, simply pass
one more numeric value in the event callback struct.  This works
for the specific case of VerifySslPeer.
@jtulley
Copy link
Author

jtulley commented Nov 13, 2012

Any further opinions on that last patch I uploaded, which contained an alternate way to pass information from VerifySslPeer to the event_callback EM_SSL_VERIFY case? I just added an additional integer field to the callbacks and the struct that is in use. Not a general case solution, but it solves this in a fairly clean way without any casts of a different structure to const char * and then back to that structure.

@rud
Copy link

rud commented May 22, 2014

So the competing PR #334 was closed but not merged. This PR was never merged either. Is this not still a problem with the SSL hostname verification?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants