Обсуждение: Some more hackery around cryptohashes (some fixes + SHA1)

Hi all,

The remnant work that I have on my agenda to replace the remaining
low-level cryptohash calls of OpenSSL (SHAXXInit and such) by EVP is
the stuff related to SHA1, that gets used in two places: pgcrypto and
uuid-ossp.

First, I got to wonder if it would be better to support SHA1 directly
in cryptohash{_openssl}.c, glue some code to pgcrypto to use EVP
discreetly or just do nothing.  Contrary to SHA256 and MD5 that are
used for authentication or backup manifests, SHA1 has a limited use in
core, so I wanted first to just stick something in pgcrypto or just
let it go, hoping for the day where we'd remove those two modules but
that's not a call I think we can make now.

But then, my very-recent history with uuid-ossp has made me look at
what kind of tricks we use to pull in SHA1 from pgcrypto to
uuid-ossp, and I did not like much the shortcuts used in ./configure
or uuid-ossp's Makefile to get those files when needed, depending on
the version of libuuid used (grep for UUID_EXTRA_OBJS for example).
So, I got to look at the second option of moving SHA1 directly into
the new cryptohash stuff, and quite liked the cleanup this gives.

Please find attached a set of two patches:
- 0001 is a set of small adjustments for the existing code of
cryptohashes: some cleanup for MD5 in uuid-ossp, and more importantly
one fix to call explicit_bzero() on the context data for the fallback
implementations.  With the existing code, we may leave behind some
context data.  That could become a problem if somebody has access to
this area of the memory even when they should not be able to do so,
something that should not happen, but I see no reason to not play it
safe and eliminate any traces.  If there are no objections, I'd like
to apply this part.
- 0002 is the addition of sha1 in the cryptohash infra, that includes
the cleanup between uuid-ossp and pgcrypto.  This makes any caller of
cryptohash for SHA1 to use EVP when building with OpenSSL, or the
fallback implementation.  I have adapted the fallback implementation
of SHA1 to have some symmetry with src/common/{md5.c,sha2.c}.

I am adding this patch set to the next commit fest.  Thanks for
reading!
--
Michael

On Thu, Dec 10, 2020 at 05:07:05PM +0900, Michael Paquier wrote:
> - 0001 is a set of small adjustments for the existing code of
> cryptohashes: some cleanup for MD5 in uuid-ossp, and more importantly
> one fix to call explicit_bzero() on the context data for the fallback
> implementations.  With the existing code, we may leave behind some
> context data.  That could become a problem if somebody has access to
> this area of the memory even when they should not be able to do so,
> something that should not happen, but I see no reason to not play it
> safe and eliminate any traces.  If there are no objections, I'd like
> to apply this part.

This is a nice cleanup, so I have moved ahead and applied it.  A
rebased version of the SHA1 business is attached.
--
Michael

On Mon, Dec 14, 2020 at 12:48:15PM +0900, Michael Paquier wrote:
> This is a nice cleanup, so I have moved ahead and applied it.  A
> rebased version of the SHA1 business is attached.

Rebased version attached to address the conflicts caused by 55fe26a.
I have fixed three places in pgcrypto where this missed to issue an
error if one of the init/update/final cryptohash calls failed for
SHA1.
--
Michael

On 07/01/2021 05:41, Michael Paquier wrote:
> On Mon, Dec 14, 2020 at 12:48:15PM +0900, Michael Paquier wrote:
>> This is a nice cleanup, so I have moved ahead and applied it.  A
>> rebased version of the SHA1 business is attached.
> 
> Rebased version attached to address the conflicts caused by 55fe26a.
> I have fixed three places in pgcrypto where this missed to issue an
> error if one of the init/update/final cryptohash calls failed for
> SHA1.

> diff --git a/contrib/pgcrypto/sha1.h b/src/common/sha1_int.h
> similarity index 72%
> rename from contrib/pgcrypto/sha1.h
> rename to src/common/sha1_int.h
> index 4300694a34..40fbffcd0b 100644
> --- a/contrib/pgcrypto/sha1.h
> +++ b/src/common/sha1_int.h
> @@ -1,3 +1,17 @@
> +/*-------------------------------------------------------------------------
> + *
> + * sha1_int.h
> + *      Internal headers for fallback implementation of SHA1
> + *
> + * Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
> + * Portions Copyright (c) 1994, Regents of the University of California
> + *
> + * IDENTIFICATION
> + *          src/common/sha1_int.h
> + *
> + *-------------------------------------------------------------------------
> + */
> +
>  /*    contrib/pgcrypto/sha1.h */
>  /*       $KAME: sha1.h,v 1.4 2000/02/22 14:01:18 itojun Exp $    */

Leftover reference to "contrib/pgcrypto/sha1.h"

Other than that, looks good to me.

- Heikki

On Fri, Jan 22, 2021 at 03:50:04PM +0200, Heikki Linnakangas wrote:
> Leftover reference to "contrib/pgcrypto/sha1.h"
>
> Other than that, looks good to me.

Thanks!  I have looked at that again this morning, and this was still
one indentation short.  I have also run more tests with different
combinations of --with-openssl and --with-uuid just to be sure, and
applied it.
--
Michael