NULL pointer deref on error path in _ssl debughelpers.c

[ariccio]

BPO	40079
Nosy	@tiran, @ariccio

^{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 2021-04-17.19:40:27.881>
created_at = <Date 2020-03-26.21:56:53.776>
labels = ['expert-SSL', '3.9']
title = 'NULL pointer deref on error path in _ssl debughelpers.c'
updated_at = <Date 2021-04-17.19:40:27.880>
user = 'https://github.com/ariccio'

bugs.python.org fields:

activity = <Date 2021-04-17.19:40:27.880>
actor = 'christian.heimes'
assignee = 'christian.heimes'
closed = True
closed_date = <Date 2021-04-17.19:40:27.881>
closer = 'christian.heimes'
components = ['SSL']
creation = <Date 2020-03-26.21:56:53.776>
creator = 'Alexander Riccio'
dependencies = []
files = []
hgrepos = []
issue_num = 40079
keywords = []
message_count = 2.0
messages = ['365114', '391310']
nosy_count = 2.0
nosy_names = ['christian.heimes', 'Alexander Riccio']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue40079'
versions = ['Python 3.9']

[ariccio]

At line 138 in debughelpers.c, ssl_obj, which was set to NULL on line 122, is dereferenced.

I think the original intent was to actually bubble the error up through the ssl object.

Full function:

static void
_PySSL_keylog_callback(const SSL *ssl, const char *line)
{
    PyGILState_STATE threadstate;
    PySSLSocket *ssl_obj = NULL;  /* ssl._SSLSocket, borrowed ref */
    int res, e;
    static PyThread_type_lock *lock = NULL;

    threadstate = PyGILState_Ensure();

    /* Allocate a static lock to synchronize writes to keylog file.
     * The lock is neither released on exit nor on fork(). The lock is
     * also shared between all SSLContexts although contexts may write to
     * their own files. IMHO that's good enough for a non-performance
     * critical debug helper.
     */
    if (lock == NULL) {
        lock = PyThread_allocate_lock();
        if (lock == NULL) {
            PyErr_SetString(PyExc_MemoryError, "Unable to allocate lock");
            PyErr_Fetch(&ssl_obj->exc_type, &ssl_obj->exc_value,
                        &ssl_obj->exc_tb);
            return;
        }
    }

    ssl_obj = (PySSLSocket *)SSL_get_app_data(ssl);
    assert(PySSLSocket_Check(ssl_obj));
    if (ssl_obj->ctx->keylog_bio == NULL) {
        return;
    }

    PySSL_BEGIN_ALLOW_THREADS
    PyThread_acquire_lock(lock, 1);
    res = BIO_printf(ssl_obj->ctx->keylog_bio, "%s\n", line);
    e = errno;
    (void)BIO_flush(ssl_obj->ctx->keylog_bio);
    PyThread_release_lock(lock);
    PySSL_END_ALLOW_THREADS

    if (res == -1) {
        errno = e;
        PyErr_SetFromErrnoWithFilenameObject(PyExc_OSError,
                                             ssl_obj->ctx->keylog_filename);
        PyErr_Fetch(&ssl_obj->exc_type, &ssl_obj->exc_value, &ssl_obj->exc_tb);
    }
    PyGILState_Release(threadstate);
}

[tiran]

The issue has been fixed by fbf94af in on 2020-Jun-21.