Support Disabling Renegotiation for SSLContext

[ex172000]

BPO	32257
Nosy	@tiran, @ned-deily, @njsmith, @ex172000
PRs	bpo-32257: [SSL] Support Disabling Renegotiation #4763 bpo-32257: Add ssl.OP_NO_RENEGOTIATION #5904 [3.7] bpo-32257: Add ssl.OP_NO_RENEGOTIATION (GH-5904) #6877

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/tiran'
closed_at = <Date 2018-05-16.15:42:03.588>
created_at = <Date 2017-12-09.06:38:22.986>
labels = ['type-security', 'expert-SSL', '3.7', '3.8']
title = 'Support Disabling Renegotiation for SSLContext'
updated_at = <Date 2018-05-16.15:42:03.587>
user = 'https://github.com/ex172000'

bugs.python.org fields:

activity = <Date 2018-05-16.15:42:03.587>
actor = 'ned.deily'
assignee = 'christian.heimes'
closed = True
closed_date = <Date 2018-05-16.15:42:03.588>
closer = 'ned.deily'
components = ['SSL']
creation = <Date 2017-12-09.06:38:22.986>
creator = 'chuq'
dependencies = []
files = []
hgrepos = []
issue_num = 32257
keywords = ['patch']
message_count = 21.0
messages = ['307879', '307891', '307893', '307906', '307911', '307924', '307931', '307956', '307958', '307963', '307965', '307966', '307983', '307988', '308015', '308016', '308017', '312881', '316718', '316810', '316821']
nosy_count = 4.0
nosy_names = ['christian.heimes', 'ned.deily', 'njs', 'chuq']
pr_nums = ['4763', '5904', '6877']
priority = None
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue32257'
versions = ['Python 3.7', 'Python 3.8']

[ex172000]

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

[tiran]

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.

[tiran]

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]

[ex172000]

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

[tiran]

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?

[ex172000]

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);
}

[njsmith]

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

[tiran]

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.

[tiran]

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

[ex172000]

Thank you for the investigation. This does seem better than the flag. Shall we go ahead implement this?

[tiran]

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.

[ex172000]

How about exposing the internal ssl object? This will allow applications to control the flag.

[njsmith]

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.

[tiran]

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.

[tiran]

I took Guido's keys to the time machine: openssl/openssl#4739

[ex172000]

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.

[tiran]

We don't work that way. Closed means closed forever. Please leave the bug open.

[tiran]

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.

[ned-deily]

New changeset 67c4801 by Ned Deily (Christian Heimes) in branch 'master':
bpo-32257: Add ssl.OP_NO_RENEGOTIATION (GH-5904)
67c4801

[tiran]

New changeset e2db6ad by Christian Heimes (Miss Islington (bot)) in branch '3.7':
[3.7] bpo-32257: Add ssl.OP_NO_RENEGOTIATION (GH-5904) (bpo-6877)
e2db6ad

[ned-deily]

Thanks, Christian! Merged for 3.7.0 and 3.8.0.