Skip to content

FFI interface to SSL_set_min_proto_version etc #5379

Closed

Description

@richvdh

OpenSSL 1.1.0 added a number of new functions for controlling the acceptable TLS protocol versions:

 int SSL_CTX_set_min_proto_version(SSL_CTX *ctx, int version);
 int SSL_CTX_set_max_proto_version(SSL_CTX *ctx, int version);
 int SSL_set_min_proto_version(SSL *ssl, int version);
 int SSL_set_max_proto_version(SSL *ssl, int version);

Additionally, OpenSSL 1.1.1 added getters for the same information:

 int SSL_CTX_get_min_proto_version(SSL_CTX *ctx);
 int SSL_CTX_get_max_proto_version(SSL_CTX *ctx);
 int SSL_get_min_proto_version(SSL *ssl);
 int SSL_get_max_proto_version(SSL *ssl);

See https://www.openssl.org/docs/man1.1.1/man3/SSL_set_max_proto_version.html for more details of these functions.

The intention, as documented at https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_new.html, is that these functions should be used instead of SSL_CTX_set_options and SSL_set_options for this purpose.

Anyway: it appears that these functions are not exposed via the cryptography.hazmat.bindings.openssl.binding.Binding.lib FFI interface. Could they be added?

Activity

  1. richvdh commented on Aug 6, 2020

    @richvdh
    Author

    While I'm on the subject, would it also be possible to add support for the TLS_method(), TLS_server_method() and TLS_client_method() methods which were introduced in OpenSSL 1.1.0:

     const SSL_METHOD *TLS_method(void);
     const SSL_METHOD *TLS_server_method(void);
     const SSL_METHOD *TLS_client_method(void);
    

    (see https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_new.html)

    They are the same as SSLv23_method(), SSLv23_server_method(), and SSLv23_client_method() respectively, but those three have very confusing names (which is why they were deprecated, I believe).

  2. th3b0x commented on Aug 6, 2020

    @th3b0x

    I would like to second the update of the pyca-cryptography interface to provide the interfaces recommended by @richvdh .

    I would like to specifically note that TLS_method() and related interfaces should be included. In spite of modern OpenSSL versions using a processor macro to map SSLv23_method to TLS_method it is important to note that the use of the old interface name makes systems vulnerable to malicious library/package replacement. By forcing users to use SSLv23_method you are creating avenues of exploitation surrounding malicious insertion of deprecated/obsolete code into system libraries. Since pyca-cryptography merely interfaces with the underlying system OpenSSL library, forcing a programmer to go through SSLv23_method does not guarantee that they will end up using TLS_method even though the current source code has a macro that makes these methods equivalent.

  3. reaperhulk commented on Aug 6, 2020

    @reaperhulk
    Member

    Adding these bindings is fine, but there are a few steps:

    1. A PR to this repository adding them and handling the case where they aren't available (we support OpenSSL 1.0.2 for a few more releases so this is necessary). You can see examples of that handling in various binding files and the logic we use to remove null func pointers from the lib is in _conditional.py.
    2. A PR to pyOpenSSL that adds methods to use these bindings (and switches to calling TLS_method in the tests probably). That PR will fail the tests (except for the cryptography master branch tests) until we do a release, but those master branch tests can confirm you've implemented it properly.

    The latter is a requirement because maintaining an arbitrarily large set of bindings without knowing consumers turns out to be prohibitively difficult. We've recently stripped numerous methods due to the maintenance burden so we need to have a consumer in our downstream test suite.

  4. th3b0x commented on Aug 6, 2020

    @th3b0x

    @reaperhulk thanks for the information, I already have some stuff in the works, so I'll look into the issues you've raised.

  5. th3b0x commented on Aug 19, 2020

    @th3b0x

    @reaperhulk regarding _conditional.py, given that older versions of OpenSSL would not support TLSv1_3_method, what seems like an appropriate list of dependent functions? I'm tempted to include up to TLSv1_2_method (since TLS_method first appears in major release OpenSSL 1.1.0), but I'm not sure if project guidelines stipulate something more stringent than this. I'm also observing that TLSv1_3 support doesn't seem to always externalized (at least in the last version I pulled; can provide example if required). Any thoughts before I proceed?

  6. reaperhulk commented on Aug 19, 2020

    @reaperhulk
    Member

    There are two things at play here:

    1. To the extent that it is possible make the behavior between versions the same. For example, TLS_method and SSLv23_method are the same method, so on versions prior to TLS_method being defined we should just define it. That way it's always available.
    2. For methods that are new capabilities (e.g. SSL_CTX_set_max_proto_version) bind them with the _HAS_<NAME> pattern we use and remove them in _conditional.py.

    TLSv1_2_method is already bound so you shouldn't need to add that. (https://github.com/pyca/cryptography/blob/master/src/_cffi_src/openssl/ssl.py#L349)

  7. th3b0x commented on Aug 19, 2020

    @th3b0x

    @reaperhulk duly noted, thanks for the update. I'm hoping to have something for you guys in the next day or so. This is fairly straightforward since (as noted) SSLv23_method and TLS_method are defined by a macro to be equivalent.

  8. th3b0x commented on Aug 25, 2020

    @th3b0x

    Edit: looks like it didn't handle correctly on OpenSSL 1.0.2 for some reason, have to check my local environment.

    @reaperhulk I've put #5428 in to address the TLS_method related issues. It looks like some of the tests are failing, as you indicated earlier. I'm hoping this tiny PR will help me figure out the odds and ends of doing a PR for this project.

    Subsequent to this PR getting through (it's a simple method exposure) I plan on examining what to do about

     int SSL_CTX_set_min_proto_version(SSL_CTX *ctx, int version);
     int SSL_CTX_set_max_proto_version(SSL_CTX *ctx, int version);
     int SSL_set_min_proto_version(SSL *ssl, int version);
     int SSL_set_max_proto_version(SSL *ssl, int version);
     int SSL_CTX_get_min_proto_version(SSL_CTX *ctx);
     int SSL_CTX_get_max_proto_version(SSL_CTX *ctx);
     int SSL_get_min_proto_version(SSL *ssl);
     int SSL_get_max_proto_version(SSL *ssl);
    
    

    These functions require a peak at the compiler options, since there isn't a convenient way to handle them for OpenSSL 1.0.2 (will need to do something similar to how SSLv3_method is handled)

  9. mhils commented on Oct 4, 2020

    @mhils
    Member

    8bc6920 dropped support for 1.0.2, so implementing this should be more straightforward now. :)

  10. th3b0x commented on Oct 6, 2020

    @th3b0x

    Got distracted and had to make a new PR, because I borked my branch too much for a clean Rebase.

    Current PR in relation to the TLS_method* interfaces is now #5483

    I will then be able to proceed to adding the new SSL_CTX option bindings, as requested by @richvdh

    Kindly requesting a review at the PR.

  11. added this to the Thirty Third Release milestone on Nov 29, 2020
  12. added 5 commits that reference this issue on Dec 1, 2020
    dff882a
    8eaed59
    fd3e6c0
    ffc4f9b
    12da376
  13. added a commit that references this issue on Dec 1, 2020
    a209669
  14. added a commit that references this issue on Dec 2, 2020
  15. locked as resolved and limited conversation to collaborators on Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions