Re: Proposed patch for key managment

On Fri, Dec 04, 2020 at 10:52:29PM -0500, Bruce Momjian wrote:
> OK, I worked with Sawada-san and added the attached patch. The updated
> full patch is at the same URL: :-)
>
> https://github.com/postgres/postgres/compare/master...bmomjian:key.diff

Oh, I see that you use SHA256 during firstboot, which is why you
bypass the resowner paths in cryptohash_openssl.c. Wouldn't it be
better to use IsBootstrapProcessingMode() then?

> @@ -72,14 +72,15 @@ pg_cryptohash_create(pg_cryptohash_type type)
> ctx = ALLOC(sizeof(pg_cryptohash_ctx));
> if (ctx == NULL)
> return NULL;
> + explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
>
> state = ALLOC(sizeof(pg_cryptohash_state));
> if (state == NULL)
> {
> - explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
> FREE(ctx);
> return NULL;
> }
> + explicit_bzero(state, sizeof(pg_cryptohash_state));

explicit_bzero() is used to give the guarantee that any sensitive data
gets cleaned up after an error or on free. So I think that your use
is incorrect here for an initialization.

> if (state->evpctx == NULL)
> {
> - explicit_bzero(state, sizeof(pg_cryptohash_state));
> - explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
> #ifndef FRONTEND
> ereport(ERROR,
> (errcode(ERRCODE_OUT_OF_MEMORY),

And this original position is IMO more correct.

Anyway, at quick glance, the backtrace of upthread is indicating a
double-free with an attempt to free a resource owner that has been
already free'd.
--
Michael