> On 26 Apr 2022, at 03:55, Michael Paquier <michael@paquier.xyz> wrote:
>
> 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".
Will fix.
> 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.
It's also the only one in this case since the computation won't get past the
init step with the error no? The queue will be cleared for each computation so
the risk of cross contamination is removed.
> 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.
Not really, the error system in OpenSSL has been defined as a queue with
multiple errors per call possible at least since SSLeay 0.9.1. I think this is
very much intentional, but a rare case of it.
>> 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.
I looked at that briefly, and might revisit it during the 16 cycle, but it does
have a smell of diminishing returns to it. With non-OpenSSL code ripped out
from pgcrypto it's clearly more interesting than before.
--
Daniel Gustafsson https://vmware.com/
