Re: Proposed patch for key managment

On Sun, 6 Dec 2020 at 00:42, Bruce Momjian <bruce@momjian.us> wrote:
>
> On Sat, Dec  5, 2020 at 09:54:33PM +0900, Michael Paquier wrote:
> > 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?
>
> I tried that, but we also use the resource references before the
> resource system is started even in non-bootstrap mode.  Maybe we should
> be creating a resource owner for this, but it is so early in the boot
> process I don't know if that is safe.
>
> > > @@ -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.
>
> It turns out what we were missing was a clear of state->resowner in
> cases where the resowner was null.  I removed the bzero calls completely
> and it now runs fine.
>
> > >     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.
>
> Do we even need them?
>
> > 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.
>
> I think that is now fixed too.  Updated patch at the same URL:
>
>         https://github.com/postgres/postgres/compare/master...bmomjian:key.diff

Thank you for updating the patch!

I think we need explicit_bzero() also in freeing the keywrap context.

BTW, when we need -R option pg_ctl command to start the server, how
can we start it in the single-user mode?

Regards,

-- 
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services