Bug 1745499

Also see https://bugzilla.redhat.com/show_bug.cgi?id=1402235 and https://github.com/pyca/cryptography/pull/3278

Wait, are you really saying that hashlib should clear and ignore all errors?  How would that be a safe thing to do?

If we can't guarantee that mod_ssl would clean up the stack in all cases, should Python guarantee that it works with an error on the stack in all cases?
Are the expectations for OpenSSL API usage documented anywhere?

No, I meant something different. get_fips_mode() should ignore and clear all *existing* errors on the error stack. Of course it should not ignore any error that is raised by any calls inside the function or any other hashlib function.

The general expectation is that application code should check all OpenSSL function results for error and then handle any error appropriately. An application may either decide to ignore an error or to report an error. In either case the application should deal with the error stack correctly. As common courtesy applications should clean up after themselves. The recommendation is not written down in the OpenSSL documentation or wiki.

It might be sufficient to clear the error stack when _ssl or _hashlib are imported.

Since it's not the first time mod_ssl is causing trouble, perhaps it's a good idea to get back to mod_ssl upstream and ask upstream to review the code?

> get_fips_mode() should ignore and clear all *existing* errors on the error stack

Should it? I'm not convinced; clearing and ignoring any error sounds very shaky. Could you point me to some upstream documentation/discussion saying this should be done?
I believe OpenSSL's own SSL_* code is, in this regard, in a very similar situation as Python's hashlib. The docs of SSL_get_error say that "The current thread's error queue must be empty before the TLS/SSL I/O operation is attempted".


Or I can add a workaround, but let's treat it as a workaround: please open issue(s) to clarify the intended usage and/or track the bug.

The get_fips_mode() function from the downstream patch looks like this:

static PyObject *
_hashlib_get_fips_mode_impl(PyObject *module)
/*[clinic end generated code: output=ad8a7793310d3f98 input=f42a2135df2a5e11]*/
{
    int result = FIPS_mode();
    if (result == 0) {
        // "If the library was built without support of the FIPS Object Module,
        // then the function will return 0 with an error code of
        // CRYPTO_R_FIPS_MODE_NOT_SUPPORTED (0x0f06d065)."
        // But 0 is also a valid result value.

        unsigned long errcode = ERR_peek_last_error();
        if (errcode) {
            _setException(PyExc_ValueError);
            return NULL;
        }
    }
    return PyLong_FromLong(result);
}

Since the function uses the current error stack to determinate if result == 0 is an error or a valid result, it has to clear the error stack. This is the same design principal as using errno to figure out if a function call fails, because library function never set errno to 0. The fact is not explicitly written down in the documentation for libcrypto's error handling. Perhaps the documentation should be improved. The rest of _hashopenssl.c is not affected by the problem, because _setException() is only called whenever a libcrypto function call returns an error indicator.

There are two ways to address the issue:

1) Raise the issue as blocker+ with mod_ssl and have mod_ssl address all places that can leak an OpenSSL error on the error stack.
2) Change the downstream get_fips_mode() function to clear the error stack before it calls FIPS_mode().

Solution (2) is a mix of a workaround for a mod_ssl problem and following common practices for error handling. IMHO it's more best practices than workaround. It's definitely easier than reviewing the entire code base of mod_ssl.

Ah! Makes sense now. I'll clear the stack.
Thank you for your patience!

Thanks for being thorough! :)

I didn't explain the issue properly at first.

It seems appropriate to use ERR_print_errors_cb, and give a RuntimeWarning for each error. Would that work for you?

The use of ERR_print_errors_cb may change behavior of applications. I cannot foresee consequences and ill side effects of the callback.

I'm also worried that a RuntimeWarning in Apache HTTPd's error log is going to confuse customers and create additional support tickets. A customer or application developer cannot get rid of the root cause. At best a warning is going to annoy engineers and remind them to file bugs against mod_ssl.

I would just call ERR_clear_error() and maybe file a bug against mod_ssl.

{
    int result;

    /* mod_ssl leaves error on the OpenSSL error stack. Clear the error stack and ignore
     * left over errors, so we can use ERR_peek_last_error() to detect error condition,
     * see RHBZ#1745499. */
    ERR_clear_error();

    result = FIPS_mode();
    if (result == 0) {
        // "If the library was built without support of the FIPS Object Module,
        // then the function will return 0 with an error code of
        // CRYPTO_R_FIPS_MODE_NOT_SUPPORTED (0x0f06d065)."
        // But 0 is also a valid result value.

        unsigned long errcode = ERR_peek_last_error();
        if (errcode) {
            _setException(PyExc_ValueError);
            return NULL;
        }
    }
    return PyLong_FromLong(result);
}

> The use of ERR_print_errors_cb may change behavior of applications. I cannot foresee consequences and ill side effects of the callback.

I don't understand. How does this change behavior of applications?
According to the documentation, it calls the callback for each error on the stack, and clears the stack. It shouldn't change global state, apart from clearing the stack.

> I'm also worried that a RuntimeWarning in Apache HTTPd's error log is going to confuse customers and create additional support tickets. A customer or application developer cannot get rid of the root cause. At best a warning is going to annoy engineers and remind them to file bugs against mod_ssl.

But I'm changing hashlib for all users, not just FreeIPA. I'm quite worried about clearing and forgetting unknown errors.
Customers can't get rid of *this* root cause, but this can also hide any other causes in other applications.

Or, since this is a RHEL patch and I know we have the FIPS Object Module, I can just skip the error checking entirely:

{
    return PyLong_FromLong(FIPS_mode());
}

That wouldn't be acceptable for upstream, of course.

> I don't understand. How does this change behavior of applications?
> According to the documentation, it calls the callback for each error on the stack, and clears the stack. It shouldn't change global state, apart from clearing the stack.

My fault, I thought that ERR_print_errors_cb() sets a global error handler callback. OpenSSL's naming convention is not consistent.

> Or, since this is a RHEL patch and I know we have the FIPS Object Module, I can just skip the error checking entirely:

I'm all in favor to keep the patch as simple as possible. Less code means less chances to screw things up and cause additional regressions. Since the patch is only applied to RHEL and RHEL always has FIPS module, the one liner should do the trick.

For upstream we can do something with ERR_peek_last_error() and then check for library == ERR_LIB_CRYPTO and reasons == CRYPTO_R_FIPS_MODE_NOT_SUPPORTED.

*** Bug 1744614 has been marked as a duplicate of this bug. ***

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2019:3520