Subject: Re: Modern SHA2- based password hashes for pgcrypto

From: Alvaro Herrera <[email-masked]>
Date: Sat, 5 Apr 2025 19:22:58 +0200

Hello,

I have pushed this now, hoping it won't explode.

Thanks!

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"No es bueno caminar con un hombre muerto"

---

From: Bernd Helmle <[email-masked]>
Date: Sun, 06 Apr 2025 19:39:56 +0200

> I have pushed this now, hoping it won't explode.

Very cool, i keep my fingers crossed.

Thanks,
	Bernd

---

From: Tom Lane <[email-masked]>
Date: Sun, 06 Apr 2025 15:43:36 -0400

> I have pushed this now, hoping it won't explode.

mamba is not happy:

ccache cc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type -Wshadow=compatible-local -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-format-truncation -Wno-stringop-truncation -g -O2 -Werror -fPIC -DPIC -fvisibility=hidden -I. -I. -I../../src/include   -I/usr/pkg/include/libxml2 -I/usr/pkg/include -I/usr/pkg/include  -I/usr/pkg/include  -c -o pgcrypto.o pgcrypto.c
In file included from /usr/include/ctype.h:100,
                 from ../../src/include/port.h:16,
                 from ../../src/include/c.h:1345,
                 from ../../src/include/postgres.h:48,
                 from crypt-sha.c:46:
crypt-sha.c: In function 'px_crypt_shacrypt':
crypt-sha.c:324:16: error: array subscript has type 'char' [-Werror=char-subscripts]
  324 |    if (isalpha(*ep) || isdigit(*ep) || (*ep == '.') || (*ep == '/'))
      |                ^
crypt-sha.c:324:32: error: array subscript has type 'char' [-Werror=char-subscripts]
  324 |    if (isalpha(*ep) || isdigit(*ep) || (*ep == '.') || (*ep == '/'))
      |                                ^
cc1: all warnings being treated as errors
gmake[1]: *** [<builtin>: crypt-sha.o] Error 1

What this is on about is that portable use of isalpha() or isdigit()
requires casting a "char" value to "unsigned char".  I was about to
make that simple change when I started to question if we actually
want to be using <ctype.h> here at all.  Do you REALLY intend that
any random non-ASCII letter is okay to include in the decoded_salt?
Are you okay with that filter being locale-dependent?

I'd be more comfortable with a check like

	if (strchr("...valid chars...", *ep) != NULL)

It looks like "_crypt_itoa64" might be directly usable as the
valid-chars string, too.  (BTW, why is _crypt_itoa64 not
marked const?)

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Sun, 6 Apr 2025 23:02:50 -0400

Hi,

> I have pushed this now, hoping it won't explode.

I have a WIP patch that adds gcc specific allocator attributes for palloc et
al. Just rebased that. It warns on the new code as follows:

[1489/1804 42  82%] Compiling C object contrib/pgcrypto/pgcrypto.so.p/crypt-sha.c.o
../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-sha.c: In function 'px_crypt_shacrypt':
../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-sha.c:605:13: warning: pointer 'cp' may be used after 'pfree' [-Wuse-after-free]
  605 |         *cp = '\0';
      |         ~~~~^~~~~~
../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-sha.c:533:9: note: call to 'pfree' here
  533 |         pfree(s_bytes);
      |         ^~~~~~~~~~~~~~

And it sure seems to have a point.  I'm surprised this isn't causing wider
issues...

Greetings,

Andres Freund

---

From: Bernd Helmle <[email-masked]>
Date: Mon, 07 Apr 2025 09:03:30 +0200

> 	if (strchr("...valid chars...", *ep) != NULL)

Hmm, the idea was to allow a wider range of letters for the salt. This
came from my experiments that openssl allows more than just the ones
from _crypt_itoa64. But after reading this, i realized even isalpha()
isn't enough, since e.g. mulitbyte character wouldn't work anyways,
like openssl allows:

echo -n password | openssl passwd -5 -salt ÄÖÜ -stdin
$5$ÄÖÜ$NduBUgCdzvnW1lW8EMxW9DuxN6HmT0niS7H4ftRxuX0

So we would have to test for these cases, too. I looked again into
pyhon's passlib, and they don't accept any non-ASCII fancy characters,
neither.

So i think Tom has a point, we can restrict this to the characters from
_crypt_itoa64 and go that route. I am not sure about externally
generated hashes which are going to be stored in postgres and validated
later there. This can restrict the use case perhaps.

> valid-chars string, too.  (BTW, why is _crypt_itoa64 not

That's an oversight by me.

I can create a patch with the fixes (and the one Andres' reported) for
today.

Thanks

	Bernd

---

From: Bernd Helmle <[email-masked]>
Date: Mon, 07 Apr 2025 09:09:39 +0200

Am Sonntag, dem 06.04.2025 um 23:02 -0400 schrieb Andres Freund:
> [1489/1804 42  82%] Compiling C object
> contrib/pgcrypto/pgcrypto.so.p/crypt-sha.c.o
> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-
> sha.c:605:13: warning: pointer 'cp' may be used after 'pfree' [-Wuse-
> after-free]
>   605 |         *cp = '\0';
>       |         ~~~~^~~~~~

Indeed. I think this assignment is useless anyways, since s_bytes is
already allocated with palloc0. I must have overseen this one when
rearranging code...but yes, strange that it didn't cause drama.

Thanks
	Bernd

---

From: Andres Freund <[email-masked]>
Date: Mon, 7 Apr 2025 14:00:43 -0400

Hi,

On 2025-04-07 09:09:39 +0200, Bernd Helmle wrote:
> > sha.c: In function 'px_crypt_shacrypt':
> > sha.c:533:9: note: call to 'pfree' here
> >   533 |         pfree(s_bytes);
> >       |         ^~~~~~~~~~~~~~
> > 

Valgrind did find it actually, I just missed it due to other failures:

https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=skink&dt=2025-04-05%2020%3A52%3A38&stg=recovery-check

==2917549== VALGRINDERROR-BEGIN
==2917549== Invalid write of size 1
==2917549==    at 0x11D4AD9C: px_crypt_shacrypt (crypt-sha.c:605)
==2917549==    by 0x11D54B50: run_crypt_sha (px-crypt.c:76)
==2917549==    by 0x11D54BCB: px_crypt (px-crypt.c:119)
==2917549==    by 0x11D4BCBB: pg_crypt (pgcrypto.c:228)
==2917549==    by 0x3BBCEA: ExecInterpExpr (execExprInterp.c:1001)
==2917549==    by 0x3B790E: ExecInterpExprStillValid (execExprInterp.c:2299)
==2917549==    by 0x48FD40: ExecEvalExprSwitchContext (executor.h:466)
==2917549==    by 0x48FD40: evaluate_expr (clauses.c:5012)
==2917549==    by 0x48FF08: evaluate_function (clauses.c:4519)
==2917549==    by 0x492054: simplify_function (clauses.c:4108)
==2917549==    by 0x4901FA: eval_const_expressions_mutator (clauses.c:2593)
==2917549==    by 0x432CDE: expression_tree_mutator_impl (nodeFuncs.c:3486)
==2917549==    by 0x48FFB9: eval_const_expressions_mutator (clauses.c:3727)
==2917549==  Address 0x82fa740 is 912 bytes inside a recently re-allocated block of size 8,192 alloc'd
==2917549==    at 0x4844818: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2917549==    by 0x71EF75: AllocSetContextCreateInternal (aset.c:444)
==2917549==    by 0x3D17AE: CreateExprContextInternal (execUtils.c:260)
==2917549==    by 0x3D1CA8: CreateExprContext (execUtils.c:310)
==2917549==    by 0x3D1ED8: MakePerTupleExprContext (execUtils.c:462)
==2917549==    by 0x48FDBF: evaluate_expr (clauses.c:5013)
==2917549==    by 0x48FF08: evaluate_function (clauses.c:4519)
==2917549==    by 0x492054: simplify_function (clauses.c:4108)
==2917549==    by 0x4901FA: eval_const_expressions_mutator (clauses.c:2593)
==2917549==    by 0x432CDE: expression_tree_mutator_impl (nodeFuncs.c:3486)
==2917549==    by 0x48FFB9: eval_const_expressions_mutator (clauses.c:3727)
==2917549==    by 0x432EFD: expression_tree_mutator_impl (nodeFuncs.c:3572)
==2917549== 
==2917549== VALGRINDERROR-END
{
   <insert_a_suppression_name_here>
   Memcheck:Addr1
   fun:px_crypt_shacrypt
   fun:run_crypt_sha
   fun:px_crypt
   fun:pg_crypt
   fun:ExecInterpExpr
   fun:ExecInterpExprStillValid
   fun:ExecEvalExprSwitchContext
   fun:evaluate_expr
   fun:evaluate_function
   fun:simplify_function
   fun:eval_const_expressions_mutator
   fun:expression_tree_mutator_impl
   fun:eval_const_expressions_mutator
}
**2917549** Valgrind detected 1 error(s) during execution of "SELECT crypt('', '$5$Szzz0yzz');"

Greetings,

Andres