Date: Sat, 5 Dec 2020 10:42:05 -0500
From: Bruce Momjian <bruce@momjian.us>
To: Michael Paquier <michael@paquier.xyz>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Proposed patch for key managment
Message-ID: <20201205154205.GA31727@momjian.us>
References: <20201202213814.GG20285@momjian.us>
 <20201205020803.GA8757@momjian.us>
 <CA+fd4k7eHyvqU8FbDTcK_GV0Qc6+QyJCoaFF9dHipJSG9pWmcQ@mail.gmail.com>
 <20201205035229.GC8757@momjian.us>
 <X8uDCWWf9BlBFa/B@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <X8uDCWWf9BlBFa/B@paquier.xyz>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

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

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee