Re: Cryptohash OpenSSL error queue in FIPS enabled builds

On Tue, Apr 26, 2022 at 12:07:32AM +0200, Daniel Gustafsson wrote:
> In this particular codepath I think we can afford clearing it on the way out,
> with a comment explaining why. It's easily reproducible and adding a call and
> a comment is a good documentation for ourselves of this OpenSSL behavior. That
> being said, clearing on the way in is the important bit.

+ * consumed an error, but cipher initialization can in FIPS enabled
It seems to me that this comment needs a hyphen, as of
"FIPS-enabled".

I am a bit annoyed to assume that having only a localized
ERR_clear_error() in the error code path of the init() call is the
only problem that would occur, only because that's the first one we'd
see in a hash computation. So my choice would be to call
ERR_get_error() within SSLerrmessage() and clear the queue after
fetching the error code via ERR_get_error() for both
cryptohash_openssl.c and hmac_openssl.c, but I won't fight hard
against both of you on this point, either.

Perhaps this should be reported to the upstream folks? We'd still
need this code for already released versions, but getting two errors
looks like a mistake.

> pgcrypto doesn't really consume or even inspect the OpenSSL errors, but pass on
> a PXE error based on the context of the operation. We could clear the queue as
> we leave, but as you say we already clear it before calling in other places so
> it's not clear that it's useful. We've had EVP in pgcrypto for some time
> without seeing issues from error queues, one error left isn't that different
> from two when not consumed.

Okay. I did not recall the full error stack used in pgcrypto. It is
annoying to not get from OpenSSL the details of the error, though.
With FIPS enabled, one computing a hash with pgcrypto would just know
about the initialization error, but would miss why the computation
failed. It looks like we could use a new error code to tell
px_strerror() to look at the OpenSSL error queue instead of one of the
hardcoded strings. Just saying.

> The attached 0002 does however correctly (IMO) report the error as an init
> error instead of the non-descript generic error, which isn't really all that
> helpful. I think that also removes the last consumer of the generic error, but
> I will take another look with fresh eyes to confirm that.
>
> 0003 removes what I think is a weirdly placed questionmark from the message
> that make it seem strangely ambiguous. This needs to update the test answer
> files as well, but I first wanted to float the idea before doing that.

Good catches.
--
Michael