classification
Title: Support Disabling Renegotiation for SSLContext
Type: security Stage: resolved
Components: SSL Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: christian.heimes Nosy List: christian.heimes, chuq, ned.deily, njs
Priority: Keywords: patch

Created on 2017-12-09 06:38 by chuq, last changed 2018-05-16 15:42 by ned.deily. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 4763 closed chuq, 2017-12-09 07:24
PR 5904 merged christian.heimes, 2018-02-26 08:11
PR 6877 merged miss-islington, 2018-05-15 20:26
Messages (21)
msg307879 - (view) Author: Qichao Chu (chuq) Date: 2017-12-09 06:38
Adding a new method in SSLContext so that we can disable renegotiation easier.
This resolves CVE-2009-3555 and attack demoed by thc-ssl-dos
msg307891 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-12-09 11:57
Thanks for your patch, a few comments

We generally don't have special functions to set flags. SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS is an OpenSSL < 1.1.0 option. OpenSSL 1.1.0 still defines the flag but no longer uses it. With your patch, the Python function would fail with a NameError.

I don't think that self.options is the right way to set that flag. The option attribute manipulates SSL_CTX->options, which affects SSL->options. The flag has to be set on SSL->s3->flags.

Your patch is missing documentation update and tests.
msg307893 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-12-09 12:20
I don't think your PR is required. The issue has been addressed in OpenSSL 0.9.8m over 7 years ago, https://access.redhat.com/security/cve/cve-2009-3555.


From https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_options.html

> OpenSSL always attempts to use secure renegotiation as described in RFC5746. This counters the prefix attack described in CVE-2009-3555 and elsewhere.


OpenSSL changelog

Changes between 0.9.8l and 0.9.8m [25 Feb 2010]


  *) Implement RFC5746. Re-enable renegotiation but require the extension
     as needed. Unfortunately, SSL3_FLAGS_ALLOW_UNSAFE_LEGACY_RENEGOTIATION
     turns out to be a bad idea. It has been replaced by
     SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION which can be set with
     SSL_CTX_set_options(). This is really not recommended unless you
     know what you are doing.
     [Eric Rescorla <ekr@networkresonance.com>, Ben Laurie, Steve Henson]
msg307906 - (view) Author: Qichao Chu (chuq) Date: 2017-12-09 18:04
Hi Christian,

Thank you for review! I have changed the code to directly setting this flag by using s3->flag. Code is copied from nginx repo: https://github.com/nginx/nginx/blob/ed0cc4d52308b75ab217724392994e6828af4fda/src/event/ngx_event_openssl.c.

I think this change is still needed. Although OpenSSL claimed it is fixed, THC-SSL-DOS showed it is vulnerable. If this is not the case, then nginx won't need to set the flag.

Thanks,
Qichao
msg307911 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-12-09 19:09
If it's a bug in OpenSSL, please report the bug with OpenSSL and request a fix. Bugs should be patched where they occur. Can you contact OpenSSL development team, please?

The flag is not documented and I don't know how it influences OpenSSL. Does the flag only affect SSL 3.0 or also TLS 1.0 and newer?
msg307924 - (view) Author: Qichao Chu (chuq) Date: 2017-12-09 21:59
I don't think it is a bug in OpenSSL. For various reasons, certain applications must allow renegotiation while this leaves security problem for others. That's why if python can control this flag, applications will be more confident in dealing with DoS attacks aimed at renegotiation.

This flag controls not only SSL3 but also TLSv1.1 and TLSv1.2 after testing on Nginx and Gevent. 

As of OpenSSL 1.0.2h, in file ssl/s3_lib.c

int ssl3_renegotiate(SSL *s)
{
    if (s->handshake_func == NULL)
        return (1);

    if (s->s3->flags & SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS)
        return (0);

    s->s3->renegotiate = 1;
    return (1);
}
msg307931 - (view) Author: Nathaniel Smith (njs) * (Python committer) Date: 2017-12-10 00:08
Another reason to consider making it possible to disable renegotiation is HTTP/2. RFC 7540 says:

   A deployment of HTTP/2 over TLS 1.2 MUST disable renegotiation.  An
   endpoint MUST treat a TLS renegotiation as a connection error
   (Section 5.4.1) of type PROTOCOL_ERROR.

https://tools.ietf.org/html/rfc7540#section-9.2.1
msg307956 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-12-10 10:37
Sounds about right, but I cannot find a good way to disable renegotiation.

* SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS doesn't look right. For one it's an internal, undocumented flag. But more important it is no longer supported in OpenSSL 1.1.0.

* The info_callback trick does not work. The info callback cannot return an error indicator. In OpenSSL 1.1.0 the function signature is ``void (*cb) (const SSL *ssl, int type, int val)``, which means it cannot modify the SSL object in order to abort the connection forcefully.
msg307958 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-12-10 11:13
Apache mod_ssl implements CVE-2009-3555 by carefully tracking renegotiation state through-out the code base and a custom IO layer that refuses IO when the reneg_state becomes invalid.

[1] https://github.com/apache/httpd/blob/trunk/modules/ssl/ssl_private.h#L502-L513
[2] https://github.com/apache/httpd/blob/trunk/modules/ssl/ssl_engine_io.c#L202-L250
msg307963 - (view) Author: Qichao Chu (chuq) Date: 2017-12-10 17:31
Thank you for the investigation. This does seem better than the flag. Shall we go ahead implement this?
msg307965 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-12-10 17:39
There must be a better way than custom BIOs. An implemented based on Apache's approach is probably > 1,000 lines of C code and a massive compliciation of the module.
msg307966 - (view) Author: Qichao Chu (chuq) Date: 2017-12-10 17:43
How about exposing the internal ssl object? This will allow applications to control the flag.
msg307983 - (view) Author: Nathaniel Smith (njs) * (Python committer) Date: 2017-12-10 20:21
It looks like openssl master has SSL_OP_NO_RENEGOTIATION: https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_options.html

Before that, I guess one could use SSL_CTX_sess_{connect,accept}_renegotiate to detect when a renegotiation has occurred and then error out? Admittedly this is more effective in nonblocking or memorybio mode. Or you could do something similar with the info callback: allow the current operation to succeed, but mark the connection as "poisoned". (Heck, in socket bio mode you could flat out close the socket. That'll shut things down.)

For bonus annoyance, note that RFC 7240 does allow implementations to support renegotiation that happens before any data is exchanged, to allow for the encrypted client cert hack.
msg307988 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-12-10 20:51
Thanks for checking! I had only checked 1.0.2 and 1.1.0 branch...

I can easily expose the info cb in Python -- but there is no simple way to bubble up an exception from a callback to Python. The server name callback ignores exception and just prints them with PyErr_WriteUnraisable().

Since OpenSSL 1.1.1 will have SSL_OP_NO_RENEGOTIATION, I'm leaning towards not making the code more complicated. Either we have to wait for 1.1.1 or ask OpenSSL to backport the feature to 1.0.2 and 1.1.0.
msg308015 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-12-11 06:33
I took Guido's keys to the time machine: https://github.com/openssl/openssl/issues/4739
msg308016 - (view) Author: Qichao Chu (chuq) Date: 2017-12-11 06:40
Thanks Christian! Let's wait for OpenSSL then.
I will close this bug for now and reopen when OpenSSL releases 1.1.1 with the new flag.
msg308017 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-12-11 06:45
We don't work that way. Closed means closed forever. Please leave the bug open.
msg312881 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-02-26 08:17
The OP_NO_RENEGOTIATION option prevents renegotiation in TLS 1.2 and lower. Renegotiation is a problematic TLS feature that has led to security issues like CVE-2009-3555. TLS 1.3 has removed renegotiation completely in favor of much more reliable and simpler rekeying.

PR5904 just adds the constant to the list of options and documents it. I didn't add it earlier because it wasn't available in the OpenSSL 1.1.0 branch until now. The next upcoming release of 1.1.0 will have it.
msg316718 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-05-15 20:25
New changeset 67c48016638aac9a15afe6fd6754d53d2bdd6b76 by Ned Deily (Christian Heimes) in branch 'master':
bpo-32257: Add ssl.OP_NO_RENEGOTIATION (GH-5904)
https://github.com/python/cpython/commit/67c48016638aac9a15afe6fd6754d53d2bdd6b76
msg316810 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-05-16 14:26
New changeset e2db6ad1d96ca3e8bd29178f7093785c5d550bb7 by Christian Heimes (Miss Islington (bot)) in branch '3.7':
[3.7] bpo-32257: Add ssl.OP_NO_RENEGOTIATION (GH-5904) (#6877)
https://github.com/python/cpython/commit/e2db6ad1d96ca3e8bd29178f7093785c5d550bb7
msg316821 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-05-16 15:42
Thanks, Christian!  Merged for 3.7.0 and 3.8.0.
History
Date User Action Args
2018-05-16 15:42:03ned.deilysetstatus: open -> closed
priority: deferred blocker ->
messages: + msg316821

resolution: fixed
stage: patch review -> resolved
2018-05-16 14:26:22christian.heimessetmessages: + msg316810
2018-05-15 20:26:50miss-islingtonsetpull_requests: + pull_request6549
2018-05-15 20:25:42ned.deilysetmessages: + msg316718
2018-02-26 08:17:59christian.heimessetpriority: high -> deferred blocker
versions: + Python 3.8, - Python 2.7, Python 3.6
nosy: + ned.deily

messages: + msg312881
2018-02-26 08:11:28christian.heimessetstage: patch review
pull_requests: + pull_request5673
2017-12-11 06:45:05christian.heimessetstatus: closed -> open
priority: normal -> high
type: enhancement -> security

versions: + Python 3.6, Python 3.7
messages: + msg308017
resolution: later -> (no value)
stage: resolved -> (no value)
2017-12-11 06:40:41chuqsetstatus: open -> closed
resolution: later
messages: + msg308016

stage: patch review -> resolved
2017-12-11 06:33:57christian.heimessetmessages: + msg308015
2017-12-10 20:51:54christian.heimessetmessages: + msg307988
2017-12-10 20:21:26njssetmessages: + msg307983
2017-12-10 17:43:42chuqsetmessages: + msg307966
2017-12-10 17:39:09christian.heimessetmessages: + msg307965
2017-12-10 17:31:15chuqsetmessages: + msg307963
2017-12-10 11:13:58christian.heimessetmessages: + msg307958
2017-12-10 10:37:36christian.heimessetmessages: + msg307956
2017-12-10 00:08:04njssetnosy: + njs
messages: + msg307931
2017-12-09 21:59:16chuqsetmessages: + msg307924
2017-12-09 19:09:45christian.heimessetmessages: + msg307911
2017-12-09 18:04:12chuqsetmessages: + msg307906
2017-12-09 12:20:46christian.heimessetmessages: + msg307893
2017-12-09 11:57:28christian.heimessetmessages: + msg307891
2017-12-09 07:25:43chuqsetpull_requests: - pull_request4665
2017-12-09 07:25:27chuqsetpull_requests: - pull_request4664
2017-12-09 07:24:21chuqsetpull_requests: + pull_request4666
2017-12-09 07:16:29chuqsetpull_requests: + pull_request4665
2017-12-09 07:02:12chuqsetkeywords: + patch
stage: patch review
pull_requests: + pull_request4664
2017-12-09 06:38:23chuqcreate