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.

Sign up for GitHub

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

Added TLS_method bindings for OpenSSL and LibreSSL #5483

Closed
wants to merge 8 commits into from
Closed

Added TLS_method bindings for OpenSSL and LibreSSL #5483

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
07b8132
Added TLS_method TLS_client_method and TLS_server_method hooks into t…
th3b0x Oct 6, 2020
dc9cde7
Merging so that Alex doesn't break me on the wheel like the OpenSSL 1…
th3b0x Oct 6, 2020
a5f30c8
Removed redundant tests for TLS_method bindings
th3b0x Oct 7, 2020
f9b9e05
Updated binding to backport TLS_method
th3b0x Oct 7, 2020
1814d4f
Code Review: simplify binding (per reaperhulk)
th3b0x Oct 7, 2020
f1a5433
Removed extraneous newline
th3b0x Oct 10, 2020
50c398c
Setters and Getters for SSL/CTX Protocols
th3b0x Oct 24, 2020
d0dd934
Setters and Getters for SSL/CTX Protocols
th3b0x Oct 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/_cffi_src/openssl/ssl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
static const long Cryptography_HAS_TLSv1_1;
static const long Cryptography_HAS_TLSv1_2;
static const long Cryptography_HAS_TLSv1_3;
static const long Cryptography_HAS_TLS_METHOD;
static const long Cryptography_HAS_SECURE_RENEGOTIATION;
static const long Cryptography_HAS_TLSEXT_STATUS_REQ_CB;
static const long Cryptography_HAS_STATUS_REQ_OCSP_RESP;
Expand Down Expand Up @@ -362,6 +363,11 @@
const SSL_METHOD *DTLSv1_server_method(void);
const SSL_METHOD *DTLSv1_client_method(void);

/* Added in 1.1.0 */
const SSL_METHOD *TLS_method(void);
const SSL_METHOD *TLS_client_method(void);
const SSL_METHOD *TLS_server_method(void);

/* Added in 1.0.2 */
const SSL_METHOD *DTLS_method(void);
const SSL_METHOD *DTLS_server_method(void);
Expand Down Expand Up @@ -501,6 +507,7 @@
"""

CUSTOMIZATIONS = """

Copy link
Member

Choose a reason for hiding this comment

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

Extraneous newline. Otherwise this LGTM now.

#if CRYPTOGRAPHY_OPENSSL_LESS_THAN_110
static const long Cryptography_HAS_VERIFIED_CHAIN = 0;
Cryptography_STACK_OF_X509 *(*SSL_get0_verified_chain)(const SSL *) = NULL;
Expand Down Expand Up @@ -755,4 +762,13 @@
#else
static const long Cryptography_HAS_TLSv1_3 = 1;
#endif

#if CRYPTOGRAPHY_OPENSSL_LESS_THAN_110 && !CRYPTOGRAPHY_IS_LIBRESSL
static const long Cryptography_HAS_TLS_METHOD = 0;
Copy link
Member

Choose a reason for hiding this comment

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

  1. When defining TLS_method to be SSLv23_method you no longer need to conditionally remove the function. It will always be available. So the else block, the HAS_TLS_METHOD variable, and the cryptography_has_tls_method func in _conditional.py are not required.
  2. I'd prefer this to be done using #define rather than assigning func ptrs

Copy link
Author

@th3b0x th3b0x Oct 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@reaperhulk Thank you for the very thorough and useful review (lots of great stuff that would've slipped through the cracks due to my inexperience with CFFI and lack of attention). I'm reviewing one last issue here related to how this functionally modifies program behavior. I suspect I'll have my final item ready for submission today or tomorrow. Hold tight.

(This update caused paramiko to fail its downstream build in Travis CI)

Copy link
Author

@th3b0x th3b0x Oct 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@reaperhulk

Initially, I was in agreement on this, but now I am thinking that it might be unwise to implement the TLS_method interfaces in this manner, because it would allow a developer to invoke TLS_method on OpenSSL implementations that might not support any version of TLS.

Additionally, I am beginning to think that I might need to actually implement more tests in this PR before it's ready due to the kinds of problems described in this Code Review for Google's BoringSSL: https://boringssl-review.googlesource.com/c/boringssl/+/8513/
I happen to know that the problems described by @davidben do happen to persist in OpenSSL 1.1.1g (because I performed an inspection earlier this year to identify potential avenues of exploitation related to the problems described in his BoringSSL commit comment).

As a specific test, I think it might be important to add a test related to whether SSL_OP_NO* causes "surprising" (dangerous) behavior as @davidben describes. I am personally wondering if we should specifically retain conditional removal, but replace cryptography_has_tls_method with a flag similar to CRYPTOGRAPHY_OPENSSL_LESS_THAN_110 that would indicate whether or not the library in use is a version of OpenSSL that retains the stupidly dangerous behavior of forcing the use of downgraded SSL/TLS methods when a user provides an SSL_OP bitmask that creates discontinuities in the SSL/TLS version ranges supported. (I would provide code to demonstrate what I'm talking about, but I think @davidben or yourself might already know what I mean).

Note: I have the jupyter notebook describing the issue on Github, but it's private and the code demonstrating the discontinuous range issue "got lost" because I apparently forgot to push it before deleting the folder. Not sure what happened actually, because (upon reinspection) it looks like I might have lost a little more than just the demo code.

Copy link
Member

Choose a reason for hiding this comment

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

Err, what is my location?

Copy link
Member

Choose a reason for hiding this comment

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

It's true I live in Washington, DC but I don't see the relevance?

Copy link
Member

Choose a reason for hiding this comment

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

TLS_method is intended as a drop-in replacement for SSLv23_method and we should treat it as such...

cryptography's bindings layer cannot enforce sanity in OpenSSL's APIs. That way lies madness. We can only expose their API and attempt use it safely/sanely within our own code. We don't even consider our bindings public surface area any more (pyOpenSSL is a special case that we ensure does not break).

I'm opposed to trying to test for behavioral quirks (even dangerous ones) since it almost definitionally can't be useful to this project. That effort is much better expended upstream in conversations with OpenSSL about how to improve/replace dangerous APIs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah there's no difference in behavior between SSLv23_method and TLS_method. The version disabling flags do indeed behave a little oddly but I don't think it's related to this. The less surprising API is SSL_set_min/max_proto_version if you all want to switch to those, but they only exist starting 1.1.0.

Copy link
Author

Choose a reason for hiding this comment

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

@alex I couldn't replicate the paramiko build failure.

note, I'm going to let this one sit for a while in case anyone is maintaining versions of OpenSSL that don't implement the normal behavior.

Meanwhile, I might try to push those interfaces from issue #5379

const SSL_METHOD* (*TLS_method)(void) = NULL;
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't this just #define TLS_method SSLv23_method and then TLS_method is available in all cases?

Copy link
Author

Choose a reason for hiding this comment

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

Lack of attention on my part, I failed to pull this in from the prior PR. Excellent catch and suggestion, I've committed and pushed. Thank you, very much.

const SSL_METHOD* (*TLS_client_method)(void) = NULL;
const SSL_METHOD* (*TLS_server_method)(void) = NULL;
#else
static const long Cryptography_HAS_TLS_METHOD = 1;
#endif
"""
9 changes: 9 additions & 0 deletions src/cryptography/hazmat/bindings/openssl/_conditional.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,14 @@ def cryptography_has_srtp():
]


def cryptography_has_tls_method():
return [
"TLS_method",
"TLS_client_method",
"TLS_server_method",
]


# This is a mapping of
# {condition: function-returning-names-dependent-on-that-condition} so we can
# loop over them and delete unsupported names at runtime. It will be removed
Expand Down Expand Up @@ -342,4 +350,5 @@ def cryptography_has_srtp():
"Cryptography_HAS_ENGINE": cryptography_has_engine,
"Cryptography_HAS_VERIFIED_CHAIN": cryptography_has_verified_chain,
"Cryptography_HAS_SRTP": cryptography_has_srtp,
"Cryptography_HAS_TLS_METHOD": cryptography_has_tls_method,
}
10 changes: 10 additions & 0 deletions tests/hazmat/backends/test_openssl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,16 @@ def test_ssl_ciphers_registered(self):
assert ctx != backend._ffi.NULL
backend._lib.SSL_CTX_free(ctx)

@pytest.mark.skipif(
backend._lib.Cryptography_HAS_TLS_METHOD == 0,
reason="Requires Library with TLS_method",
)
def test_tls_ciphers_registered(self):
meth = backend._lib.TLS_method()
ctx = backend._lib.SSL_CTX_new(meth)
assert ctx != backend._ffi.NULL
backend._lib.SSL_CTX_free(ctx)

def test_evp_ciphers_registered(self):
cipher = backend._lib.EVP_get_cipherbyname(b"aes-256-cbc")
assert cipher != backend._ffi.NULL
Expand Down
55 changes: 55 additions & 0 deletions tests/hazmat/bindings/test_openssl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,61 @@ def test_ssl_mode(self):
assert resp == expected_options
assert b.lib.SSL_get_mode(ssl) == expected_options

@pytest.mark.skipif(
Binding.lib.Cryptography_HAS_TLS_METHOD == 0,
reason="TLS_method requires OpenSSL >= 1.1.0",
)
def test_tls_ctx_options(self):
Copy link
Member

Choose a reason for hiding this comment

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

Duplicating these tests just to pass TLS_method isn't necessary.

Copy link
Author

Choose a reason for hiding this comment

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

Noted in the commit update (currently building). Thanks for catching that.

# Test that we're properly handling 32-bit unsigned on all platforms.
b = Binding()
assert b.lib.SSL_OP_ALL > 0
ctx = b.lib.SSL_CTX_new(b.lib.TLS_method())
assert ctx != b.ffi.NULL
ctx = b.ffi.gc(ctx, b.lib.SSL_CTX_free)
current_options = b.lib.SSL_CTX_get_options(ctx)
resp = b.lib.SSL_CTX_set_options(ctx, b.lib.SSL_OP_ALL)
expected_options = current_options | b.lib.SSL_OP_ALL
assert resp == expected_options
assert b.lib.SSL_CTX_get_options(ctx) == expected_options

@pytest.mark.skipif(
Binding.lib.Cryptography_HAS_TLS_METHOD == 0,
reason="TLS_method requires OpenSSL >= 1.1.0",
)
def test_tls_options(self):
# Test that we're properly handling 32-bit unsigned on all platforms.
b = Binding()
assert b.lib.SSL_OP_ALL > 0
ctx = b.lib.SSL_CTX_new(b.lib.TLS_method())
assert ctx != b.ffi.NULL
ctx = b.ffi.gc(ctx, b.lib.SSL_CTX_free)
ssl = b.lib.SSL_new(ctx)
ssl = b.ffi.gc(ssl, b.lib.SSL_free)
current_options = b.lib.SSL_get_options(ssl)
resp = b.lib.SSL_set_options(ssl, b.lib.SSL_OP_ALL)
expected_options = current_options | b.lib.SSL_OP_ALL
assert resp == expected_options
assert b.lib.SSL_get_options(ssl) == expected_options

@pytest.mark.skipif(
Binding.lib.Cryptography_HAS_TLS_METHOD == 0,
reason="TLS_method requires OpenSSL >= 1.1.0",
)
def test_tls_mode(self):
# Test that we're properly handling 32-bit unsigned on all platforms.
b = Binding()
assert b.lib.SSL_OP_ALL > 0
ctx = b.lib.SSL_CTX_new(b.lib.TLS_method())
assert ctx != b.ffi.NULL
ctx = b.ffi.gc(ctx, b.lib.SSL_CTX_free)
ssl = b.lib.SSL_new(ctx)
ssl = b.ffi.gc(ssl, b.lib.SSL_free)
current_options = b.lib.SSL_get_mode(ssl)
resp = b.lib.SSL_set_mode(ssl, b.lib.SSL_OP_ALL)
expected_options = current_options | b.lib.SSL_OP_ALL
assert resp == expected_options
assert b.lib.SSL_get_mode(ssl) == expected_options

def test_conditional_removal(self):
b = Binding()

Expand Down