TLS/SSL support is vulnerable to MITM attacks

[tiran]

Pika's TLS/SSL code accepts all X.509 certificate because it doesn't verify the peer's cert at all. As consequence of this pika is vulnerable to all active attacks like man-in-the-middle attacks. An adversary can use self-signed certificate to listen to or modify all AMQP traffic. TLS without proper cert validation cancels all security of TLS.

In order to verify the peer you have to do two things:

1. Check that the leaf certificate is signed by a trusted root certification authority. "cert_reqs=ssl.CERT_REQUIRED" enables the check. Trusted root CA certs must be loaded first.

2. Verify that the common name (CN) or subject alternative name (SAN) DNS names matches the peer's FQDN. Python 3.x has ssl.match_hostname().

Both checks MUST be performed in order to validate the certificate correctly.

[gmr]

#1 is something that's up to the user of the client library to do. The goal is to be non-opinionated in SSL use. You basically have the full set of SSL socket options available to you.
#2 would be too opinionated to put into the core at this time.

Perhaps an example of pika with SSL using these options would be helpful in the docs?

[kherrala]

I would argue that optional support for 2) is required in core, because it is too cumbersome to override this logic. Would it be to opinionated to have hostname check in place if "cert_reqs" is set as in part 1) ? Python 2.x can be supported using https://pypi.python.org/pypi/backports.ssl_match_hostname

I can make a pull request for this change if needed.

[kherrala]

@vitaly-krugl Any comment on this?

[vitaly-krugl]

@kherrala, I will need to wrap my brain around SSL once again, before I can answer your question. Another area that pika probably doesn't support is ssl renegotiation, as pointed out in #703.

https://github.com/openwebos/libpalmsocket may be a good algorithmic reference for re-handshakes, etc. I rewrote libpalmsocket a while back, adding support for SSL renegotiation, certificates, and workarounds for a number of OpenSSL bugs.

[kherrala]

@vitaly-krugl I have released some of the SSL patches we are currently running in production on hundreds of clients with some times poor connectivity. They can be found from my fork in master branch. These are essentially the same changes as in my previous pull request #501. Unfortunately I don't have the time to verify them on current master but they are working fine on my original fork based on Pika 0.9.14. Maybe this can be useful for you if you are intending to fix this?

From my second patch it should be obvious that MITM protection cannot be implemented using ssl_options or the current API.

I haven't looked at SSL renegotiations, but they don't seem to be a problem for us. I believe they are handled using Python socket wrapper and standard event processing automatically?

[vitaly-krugl]

@gmr, would enabling the user to pass a callback to perform whatever certificate validation the user code deems necessary eliminate the issue of introducing opinionated code in pika core? The callback would be called upon completion of ssl handshake, and would have access to SSLContext.

From my past work on what is now https://github.com/openwebos/libpalmsocket, I recall apps needing a feature of this kind, as the type of validation sometimes differed from app to app. I also recall apps needing to save and reuse ssl contexts in order to speed-up SSL connection establishment a lot.

[kherrala]

I don't see why anyone would like to override certificate name validation in HTTP clients so why would it be required here? See for example sane default behaviour found here: https://github.com/shazow/urllib3/blob/master/urllib3/connection.py

In my opinion every TLS client library should do these check at the least. This kind of MITM vulnerability would be found in a normal security audit very easily.

Another rather more opinionated suggestion is to depend on certifi (http://certifiio.readthedocs.org/en/latest/) used by the requests library.

[vitaly-krugl]

@gmr, I would like to reopen this issue. I think that pika needs to provide a mechanism to enable users to configure server_hostname + context.check_hostname (server_hostname could be a switch for context.check_hostname) as well as deal with additional critical security configuration as in the excerpt from ssl.create_default_context below. My rationale follows:

I took a look at the builtin ssl.py, and see that it supplies a convenience public API function for creating contexts that represent recommended practices:

• ssl.create_default_context(...): doc: Create a SSLContext object with default settings. NOTE: The protocol and settings may change anytime without prior deprecation. The values represent a fair balance between maximum compatibility and security.

ssl.create_default_context enables context.check_hostname = True, which will cause ssl.match_hostname to be called upon completion of the handshake in SSLSocket.do_handshake (on python 2.7, too). However, if context.check_hostname is set, then SSLSocket.do_handshake expects server_hostname to have been provided by the user, otherwise do_handshake raises ValueError.

If pika were to support check_hostname, then it would need to provide a mechanism for passing and applying server_hostname, since ssl.wrap_socket doesn't take it as an arg (at least not on python 2.7.10)

There are additional security measures that ssl.create_default_context enables that I recall from my prior experience were critical for avoiding known vulnerabilities:

    # SSLv2 considered harmful.
    context.options |= OP_NO_SSLv2

    # SSLv3 has problematic security and is only required for really old
    # clients such as IE6 on Windows XP
    context.options |= OP_NO_SSLv3

    # disable compression to prevent CRIME attacks (OpenSSL 1.0+)
    context.options |= getattr(_ssl, "OP_NO_COMPRESSION", 0)

[vitaly-krugl]

Hi @gmr, I reopened this issue to keep it on our radar per rationale in #464 (comment).

I think there are changes that we can make, which would not be opinionated.

[vitaly-krugl]

Also, see https://www.rabbitmq.com/ssl.html

As of RabbitMQ 3.4.0, SSLv3 is disabled automatically to prevent the POODLE attack

[vitaly-krugl]

Note that pika is now python 2.7 + and python 2.7.9 has ssl.match_hostname(), so this wouldn't need https://pypi.python.org/pypi/backports.ssl_match_hostname

[vitaly-krugl]

cc @tiran

I am closing this issue because the code in pika master now allows the application to control all the SSL features called out in the description of this issue.

In pika master targeting pika v1.0.0 release, the connection parameters support passing SSLContext and server_hostname via SSLOptions. This enables the app to control all the SSL features called out in the description of this issue. In particular:

1. The user can set context.verify_mode = ssl.CERT_REQUIRED; and
2. The user can set context.check_hostname = True

• This causes SSLSocket.do_handshake() to call ssl.match_hostname(), which is supported since python 2.7.9.

[vitaly-krugl]

I opened issue #1035 to make sure we have adequate test coverage for these scenarios.