From: Bruce Momjian <bruce@momjian.us>
To: Michael Paquier <michael@paquier.xyz>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Proposed patch for key managment
Date: Sat, 5 Dec 2020 10:42:05 -0500
Message-ID: <20201205154205.GA31727@momjian.us> (raw)
In-Reply-To: <X8uDCWWf9BlBFa/B@paquier.xyz>
References: <20201202213814.GG20285@momjian.us>
<20201205020803.GA8757@momjian.us>
<CA+fd4k7eHyvqU8FbDTcK_GV0Qc6+QyJCoaFF9dHipJSG9pWmcQ@mail.gmail.com>
<20201205035229.GC8757@momjian.us>
<X8uDCWWf9BlBFa/B@paquier.xyz>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: bruce@momjian.us, michael@paquier.xyz, masahiko.sawada@2ndquadrant.com
Subject: Re: Proposed patch for key managment
In-Reply-To: <20201205154205.GA31727@momjian.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox