Re: Proposed patch for key managment - Mailing list pgsql-hackers

From Michael Paquier
Subject Re: Proposed patch for key managment
Date
Msg-id X8uDCWWf9BlBFa/B@paquier.xyz
Whole thread Raw
In response to Re: Proposed patch for key managment  (Bruce Momjian <bruce@momjian.us>)
Responses Re: Proposed patch for key managment
List pgsql-hackers
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

Attachment

pgsql-hackers by date:

Previous
From: Laurenz Albe
Date:
Subject: Re: Add session statistics to pg_stat_database
Next
From: Stephen Frost
Date:
Subject: Re: A few new options for CHECKPOINT