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.

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

Support Disabling Renegotiation for SSLContext #76438

Closed
ex172000 mannequin opened this issue Dec 9, 2017 · 21 comments
Closed

Support Disabling Renegotiation for SSLContext #76438

ex172000 mannequin opened this issue Dec 9, 2017 · 21 comments
Assignees
Labels
3.7 (EOL) end of life 3.8 only security fixes topic-SSL type-security A security issue

Comments

@ex172000
Copy link
Mannequin

ex172000 mannequin commented Dec 9, 2017

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
    Copy link
    Mannequin Author

    ex172000 mannequin commented Dec 9, 2017

    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

    @ex172000 ex172000 mannequin added the type-feature A feature request or enhancement label Dec 9, 2017
    @ex172000 ex172000 mannequin assigned tiran Dec 9, 2017
    @ex172000 ex172000 mannequin added the topic-SSL label Dec 9, 2017
    @tiran
    Copy link
    Member

    tiran commented Dec 9, 2017

    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
    Copy link
    Member

    tiran commented Dec 9, 2017

    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
    Copy link
    Mannequin Author

    ex172000 mannequin commented Dec 9, 2017

    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
    Copy link
    Member

    tiran commented Dec 9, 2017

    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
    Copy link
    Mannequin Author

    ex172000 mannequin commented Dec 9, 2017

    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
    Copy link
    Contributor

    njsmith commented Dec 10, 2017

    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
    Copy link
    Member

    tiran commented Dec 10, 2017

    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
    Copy link
    Member

    tiran commented Dec 10, 2017

    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
    Copy link
    Mannequin Author

    ex172000 mannequin commented Dec 10, 2017

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

    @tiran
    Copy link
    Member

    tiran commented Dec 10, 2017

    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
    Copy link
    Mannequin Author

    ex172000 mannequin commented Dec 10, 2017

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

    @njsmith
    Copy link
    Contributor

    njsmith commented Dec 10, 2017

    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
    Copy link
    Member

    tiran commented Dec 10, 2017

    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
    Copy link
    Member

    tiran commented Dec 11, 2017

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

    @ex172000
    Copy link
    Mannequin Author

    ex172000 mannequin commented Dec 11, 2017

    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.

    @ex172000 ex172000 mannequin closed this as completed Dec 11, 2017
    @tiran
    Copy link
    Member

    tiran commented Dec 11, 2017

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

    @tiran tiran added the 3.7 (EOL) end of life label Dec 11, 2017
    @tiran tiran reopened this Dec 11, 2017
    @tiran tiran added type-security A security issue and removed type-feature A feature request or enhancement labels Dec 11, 2017
    @tiran
    Copy link
    Member

    tiran commented Feb 26, 2018

    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.

    @tiran tiran added 3.8 only security fixes deferred-blocker labels Feb 26, 2018
    @ned-deily
    Copy link
    Member

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

    @tiran
    Copy link
    Member

    tiran commented May 16, 2018

    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
    Copy link
    Member

    Thanks, Christian! Merged for 3.7.0 and 3.8.0.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes topic-SSL type-security A security issue
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants