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
Attachments:
v2-0001-Introduce-SHA1-in-cryptohash-infrastructure.patchtext/x-diff; charset=us-asciiDownload+480-424
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
Attachments:
v3-0001-Introduce-SHA1-in-cryptohash-infrastructure.patchtext/x-diff; charset=us-asciiDownload+478-424
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