Horton

Thread

Commits

  1. Follow-up fixes for SHA-2 patch (commit 749a9e20c).

  2. Add modern SHA-2 based password hashes to pgcrypto.

  1. Modern SHA2- based password hashes for pgcrypto

    Bernd Helmle <mailings@oopsware.de> — 2024-12-31T16:06:35Z

    Hi Hackers,

Some of you might already arrived 2025, so first a Happy New Year to
everyone already there ;)

Please find attached a patch to pgcrypto to add modern SHA-2 based
password hashes sha256crypt (256 bit) and sha512crypt (512 bit) for
crypt() and gen_salt() respectively. This is compatible on what crypt()
currently does on FreeBSD and Linux and both algorithms are considered
more secure than the currently implemented hashes.

I adapted the code from the publicly available reference implementation
at [1]. It's based on our existing OpenSSL infrastructure in pgcrypto
and produces compatible password hashes with crypt() and "openssl
passwd" with "-5" and "-6" switches.

I documented the new supported hashes for pgcrypto, but didn't do
anything to update the benchmark table for the supported password
hashes.

Modern OS (at least Linux, BSDs) implementations for crypt() also
support yescrypt, which is the recommended (and default) password hash
algorithm there. I am also looking to implement that, but thought it
would be useful to have the SHA-2 based hashes first in the review.

I am going to add this patch to the upcoming january commitfest for
initial review.

[1] https://www.akkadia.org/drepper/SHA-crypt.txt


-- 
Thanks,
	Bernd

  2. Re: Modern SHA2- based password hashes for pgcrypto

    Bernd Helmle <mailings@oopsware.de> — 2024-12-31T16:36:58Z

    Am Dienstag, dem 31.12.2024 um 17:06 +0100 schrieb Bernd Helmle:
> I am going to add this patch to the upcoming january commitfest for
> initial review.

I see cfbot fails Debian Bookworm with autoconf and on macOS with
meson. I will look into it.

  3. Re: Modern SHA2- based password hashes for pgcrypto

    Daniel Gustafsson <daniel@yesql.se> — 2025-01-02T14:57:49Z

    > On 31 Dec 2024, at 17:06, Bernd Helmle <mailings@oopsware.de> wrote:

> I adapted the code from the publicly available reference implementation
> at [1]. It's based on our existing OpenSSL infrastructure in pgcrypto
> and produces compatible password hashes with crypt() and "openssl
> passwd" with "-5" and "-6" switches.

Potentially daft question, but since we require OpenSSL to build pgcrypto, why
do we need to include sha2 code instead of using the sha2 implementation in
libcrypto? How complicated would it be to use the OpenSSL API instead?

--
Daniel Gustafsson

  4. Re: Modern SHA2- based password hashes for pgcrypto

    Bernd Helmle <mailings@oopsware.de> — 2025-01-02T15:17:40Z

    Am Donnerstag, dem 02.01.2025 um 15:57 +0100 schrieb Daniel Gustafsson:
> > I adapted the code from the publicly available reference
> > implementation
> > at [1]. It's based on our existing OpenSSL infrastructure in
> > pgcrypto
> > and produces compatible password hashes with crypt() and "openssl
> > passwd" with "-5" and "-6" switches.
> 
> Potentially daft question, but since we require OpenSSL to build
> pgcrypto, why
> do we need to include sha2 code instead of using the sha2
> implementation in
> libcrypto? How complicated would it be to use the OpenSSL API
> instead?

Not sure i got you, but i use OpenSSL and the SHA2 implementation
there. See the pgcrypto px_* API (px.h and openssl.c respectively) i am
using to create the digests.

Thanks,
	Bernd

  5. Re: Modern SHA2- based password hashes for pgcrypto

    Daniel Gustafsson <daniel@yesql.se> — 2025-01-02T15:28:50Z

    > On 2 Jan 2025, at 16:17, Bernd Helmle <mailings@oopsware.de> wrote:
> 
> Am Donnerstag, dem 02.01.2025 um 15:57 +0100 schrieb Daniel Gustafsson:
>>> I adapted the code from the publicly available reference
>>> implementation
>>> at [1]. It's based on our existing OpenSSL infrastructure in
>>> pgcrypto
>>> and produces compatible password hashes with crypt() and "openssl
>>> passwd" with "-5" and "-6" switches.
>> 
>> Potentially daft question, but since we require OpenSSL to build
>> pgcrypto, why
>> do we need to include sha2 code instead of using the sha2
>> implementation in
>> libcrypto? How complicated would it be to use the OpenSSL API
>> instead?
> 
> Not sure i got you, but i use OpenSSL and the SHA2 implementation
> there. See the pgcrypto px_* API (px.h and openssl.c respectively) i am
> using to create the digests.

Sorry, skimming the patch I misread it, nevermind.

--
Daniel Gustafsson

  6. Re: Modern SHA2- based password hashes for pgcrypto

    Japin Li <japinli@hotmail.com> — 2025-01-03T15:58:48Z

    On Tue, 31 Dec 2024 at 17:06, Bernd Helmle <mailings@oopsware.de> wrote:
> Hi Hackers,
>
> Some of you might already arrived 2025, so first a Happy New Year to
> everyone already there ;)
>
> Please find attached a patch to pgcrypto to add modern SHA-2 based
> password hashes sha256crypt (256 bit) and sha512crypt (512 bit) for
> crypt() and gen_salt() respectively. This is compatible on what crypt()
> currently does on FreeBSD and Linux and both algorithms are considered
> more secure than the currently implemented hashes.
>
> I adapted the code from the publicly available reference implementation
> at [1]. It's based on our existing OpenSSL infrastructure in pgcrypto
> and produces compatible password hashes with crypt() and "openssl
> passwd" with "-5" and "-6" switches.
>
> I documented the new supported hashes for pgcrypto, but didn't do
> anything to update the benchmark table for the supported password
> hashes.
>
> Modern OS (at least Linux, BSDs) implementations for crypt() also
> support yescrypt, which is the recommended (and default) password hash
> algorithm there. I am also looking to implement that, but thought it
> would be useful to have the SHA-2 based hashes first in the review.
>
> I am going to add this patch to the upcoming january commitfest for
> initial review.
>
> [1] https://www.akkadia.org/drepper/SHA-crypt.txt
>

Greate!  I have some questions after using it.

When I use the following query, it crashed!

[local]:4012204 postgres=# select crypt('hello', '$5$$6$rounds=10000$/Zg436s2vmTwsoSz');
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
The connection to the server was lost. Attempting reset: Failed.
: !?>

It is caused by checking the shacrypt digest.  The following can fix this crash,
but I'm unsure if it is correct.

@@ -151,8 +152,7 @@ px_crypt_shacrypt(const char *pw, const char *salt, char *passwd, unsigned dstle
         type = PGCRYPTO_SHA256CRYPT;
         dec_salt_binary += strlen(magic_bytes[0]);
     }
-
-    if (strncmp(dec_salt_binary, magic_bytes[1], strlen(magic_bytes[1])) == 0)
+    else if (strncmp(dec_salt_binary, magic_bytes[1], strlen(magic_bytes[1])) == 0)
     {
         type = PGCRYPTO_SHA512CRYPT;
         dec_salt_binary += strlen(magic_bytes[1]);

Another, if I run crypt with big rounds, it cannot be canceled, for example:

select crypt('hello', '$5$rounds=9999999999$/Zg436s2vmTwsoSz');

Maybe we should add CHECK_FOR_INTERRUPTS() in step 21.

@@ -411,6 +411,7 @@ px_crypt_shacrypt(const char *pw, const char *salt, char *passwd, unsigned dstle
      *    uses the notation "digest A/B" to describe this behavior.
      */
     for (block = 0; block < rounds; block++) {
+        CHECK_FOR_INTERRUPTS();

         /* a) start digest B */
         px_md_reset(digestB);

-- 
Regrads,
Japin Li

  7. Re: Modern SHA2- based password hashes for pgcrypto

    Bernd Helmle <mailings@oopsware.de> — 2025-01-03T16:55:26Z

    Am Freitag, dem 03.01.2025 um 23:57 +0800 schrieb Japin Li:
> 
> Greate!  I have some questions after using it.
> 
> When I use the following query, it crashed!
> 
> [local]:4012204 postgres=# select crypt('hello',
> '$5$$6$rounds=10000$/Zg436s2vmTwsoSz');
> server closed the connection unexpectedly
>         This probably means the server terminated abnormally
>         before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> The connection to the server was lost. Attempting reset: Failed.
> : !?>
> 
> It is caused by checking the shacrypt digest.  The following can fix
> this crash,
> but I'm unsure if it is correct.
> 

Hmm, can you provide a backtrace? I am currently debugging the CI
results and i currently have a thinko in my code at crypt-sha.c:530, i
am using the wrong length to copy the result buffer, see 

https://api.cirrus-ci.com/v1/artifact/task/6395274957946880/log/contrib/pgcrypto/log/postmaster.log

==33750==ERROR: AddressSanitizer: stack-buffer-overflow on address
0x7ffc385204c7 at pc 0x7fd65a24814b bp 0x7ffc38520240 sp 0x7ffc3851f9f0
READ of size 128 at 0x7ffc385204c7 thread T0
    #0 0x7fd65a24814a in __interceptor_memcpy
../../../../src/libsanitizer/sanitizer_common/sanitizer_common_intercep
tors.inc:827
    #1 0x7fd64c37c25f in px_crypt_shacrypt /tmp/cirrus-ci-
build/contrib/pgcrypto/crypt-sha.c:530
    #2 0x7fd64c3993f0 in run_crypt_sha /tmp/cirrus-ci-
build/contrib/pgcrypto/px-crypt.c:76

But i cannot reproduce your crash in this case, maybe this is related
to the above issue...

> @@ -151,8 +152,7 @@ px_crypt_shacrypt(const char *pw, const char
> *salt, char *passwd, unsigned dstle
>          type = PGCRYPTO_SHA256CRYPT;
>          dec_salt_binary += strlen(magic_bytes[0]);
>      }
> -
> -    if (strncmp(dec_salt_binary, magic_bytes[1],
> strlen(magic_bytes[1])) == 0)
> +    else if (strncmp(dec_salt_binary, magic_bytes[1],
> strlen(magic_bytes[1])) == 0)
>      {
>          type = PGCRYPTO_SHA512CRYPT;
>          dec_salt_binary += strlen(magic_bytes[1]);
> 

Yeah, seems the check is not strict enough for the magic byte and i
need to be more strict here. I am going to look into this. "$5$$6$"
certainly is bogus enough that we should error out instead.

> Another, if I run crypt with big rounds, it cannot be canceled, for
> example:
> 
> select crypt('hello', '$5$rounds=9999999999$/Zg436s2vmTwsoSz');
> 
> Maybe we should add CHECK_FOR_INTERRUPTS() in step 21.
> 
> @@ -411,6 +411,7 @@ px_crypt_shacrypt(const char *pw, const char
> *salt, char *passwd, unsigned dstle
>       *    uses the notation "digest A/B" to describe this behavior.
>       */
>      for (block = 0; block < rounds; block++) {
> +        CHECK_FOR_INTERRUPTS();
> 
>          /* a) start digest B */
>          px_md_reset(digestB);
> 

Hmm not sure how expensive it is to check for interrupts for each
block. Maybe its better to check for each 10.000 blocks or so. But i
like the idea. Will investigate.


Thanks for your review,

	Bernd

  8. Re: Modern SHA2- based password hashes for pgcrypto

    Japin Li <japinli@hotmail.com> — 2025-01-04T00:19:49Z

    On Fri, 03 Jan 2025 at 17:55, Bernd Helmle <mailings@oopsware.de> wrote:
> Am Freitag, dem 03.01.2025 um 23:57 +0800 schrieb Japin Li:
>> 
>> Greate!  I have some questions after using it.
>> 
>> When I use the following query, it crashed!
>> 
>> [local]:4012204 postgres=# select crypt('hello',
>> '$5$$6$rounds=10000$/Zg436s2vmTwsoSz');
>> server closed the connection unexpectedly
>>         This probably means the server terminated abnormally
>>         before or while processing the request.
>> The connection to the server was lost. Attempting reset: Failed.
>> The connection to the server was lost. Attempting reset: Failed.
>> : !?>
>> 
>> It is caused by checking the shacrypt digest.  The following can fix
>> this crash,
>> but I'm unsure if it is correct.
>> 
>
> Hmm, can you provide a backtrace? I am currently debugging the CI
> results and i currently have a thinko in my code at crypt-sha.c:530, i
> am using the wrong length to copy the result buffer, see 
>
Here is the backtrace.

(gdb) bt
#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=<optimized out>, signo=signo@entry=6) at ./nptl/pthread_kill.c:89
#3  0x00007ba852a4526e in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#4  0x00007ba852a288ff in __GI_abort () at ./stdlib/abort.c:79
#5  0x00007ba852a297b6 in __libc_message_impl (fmt=fmt@entry=0x7ba852bce765 "*** %s ***: terminated\n") at ../sysdeps/posix/libc_fatal.c:132
#6  0x00007ba852b36c19 in __GI___fortify_fail (msg=msg@entry=0x7ba852bce77d "stack smashing detected") at ./debug/fortify_fail.c:24
#7  0x00007ba852b37ea4 in __stack_chk_fail () at ./debug/stack_chk_fail.c:24
#8  0x00007ba853d8cb2e in px_crypt_shacrypt (pw=0x64e191229370 "hello", salt=0x64e191229388 "$5$$6$rounds=10000$/Zg436s2vmTwsoSz", passwd=0x64e1912293d8 "$6$rounds=10000$/Zg436s2vmTwsoSz$TTCnOO7S5pkJHBVJ.oL74WN1Yt0n1RfQOWd60CRb4xtd9q7ChipyZ00jwYZfhDGRRJOoJNOgYKAVGpdmA8qhT1", dstlen=128) at /data/Codes/pg/master/build/../contrib/pgcrypto/crypt-sha.c:546
#9  0x00007ba853d9d9f8 in run_crypt_sha (psw=<error reading variable: Cannot access memory at address 0x31546871384155>, salt=<error reading variable: Cannot access memory at address 0x3154687138414d>, buf=<error reading variable: Cannot access memory at address 0x31546871384145>, len=<error reading variable: Cannot access memory at address 0x31546871384141>) at /data/Codes/pg/master/build/../contrib/pgcrypto/px-crypt.c:76
Backtrace stopped: Cannot access memory at address 0x31546871384175

-- 
Regrads,
Japin Li

  9. Re: Modern SHA2- based password hashes for pgcrypto

    Bernd Helmle <mailings@oopsware.de> — 2025-01-07T17:48:25Z

    Am Samstag, dem 04.01.2025 um 08:19 +0800 schrieb Japin Li:
> (gdb) bt
> #0  __pthread_kill_implementation (no_tid=0, signo=6,
> threadid=<optimized out>) at ./nptl/pthread_kill.c:44
> #1  __pthread_kill_internal (signo=6, threadid=<optimized out>) at
> ./nptl/pthread_kill.c:78
> #2  __GI___pthread_kill (threadid=<optimized out>,
> signo=signo@entry=6) at ./nptl/pthread_kill.c:89
> #3  0x00007ba852a4526e in __GI_raise (sig=sig@entry=6) at
> ../sysdeps/posix/raise.c:26
> #4  0x00007ba852a288ff in __GI_abort () at ./stdlib/abort.c:79
> #5  0x00007ba852a297b6 in __libc_message_impl
> (fmt=fmt@entry=0x7ba852bce765 "*** %s ***: terminated\n") at
> ../sysdeps/posix/libc_fatal.c:132
> #6  0x00007ba852b36c19 in __GI___fortify_fail
> (msg=msg@entry=0x7ba852bce77d "stack smashing detected") at
> ./debug/fortify_fail.c:24
> #7  0x00007ba852b37ea4 in __stack_chk_fail () at
> ./debug/stack_chk_fail.c:24
> #8  0x00007ba853d8cb2e in px_crypt_shacrypt (pw=0x64e191229370
> "hello", salt=0x64e191229388 "$5$$6$rounds=10000$/Zg436s2vmTwsoSz",
> passwd=0x64e1912293d8
> "$6$rounds=10000$/Zg436s2vmTwsoSz$TTCnOO7S5pkJHBVJ.oL74WN1Yt0n1RfQOWd
> 60CRb4xtd9q7ChipyZ00jwYZfhDGRRJOoJNOgYKAVGpdmA8qhT1", dstlen=128) at
> /data/Codes/pg/master/build/../contrib/pgcrypto/crypt-sha.c:546

Thank you very much.

This points to the same area i've investigated and it turned out i
confused length macros for the password salt, leading to the out_buf
buffer in px_crypt_shacrypt() being too small, stupid me.

Attached is a new version of the patch:

- Use correct length for the out_buf result buffer in
px_crypt_shacrypt()

- Silent a compiler warning in the error goto branch in
px_crypt_shacrypt()

- Don't accept rounds values lower or larger than supported (previously
they were silently changed to minimum/maximum).

- Per your suggestions, use CHECK_FOR_INTERRUPTS() during block
calculation to make it possible to interrupt the code when using large
values for the rounds option and just rely on the very first three
bytes when parsing the magic bytes of the salt string.

Please test again.
	
	Bernd

  10. Re: Modern SHA2- based password hashes for pgcrypto

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-01-13T16:06:16Z

    Hello

I was passing by and I noticed that this needs badly pgindent, and some
comments are enumerations that would lose formatting once through it.
For example, this would happen which is not good:

    /*
-    * 1. Start digest A
-    * 2. Add the password string to digest A
-    * 3. Add the salt to digest A
+    * 1. Start digest A 2. Add the password string to digest A 3. Add the
+    * salt to digest A
     */

I suggest you can fix this by adding a "-" sign to the opening "/*" line
so that pgindent doesn't mangle it (so it becomes /*- ).  This appears
in several places.

It's been said in my presence that pgcrypto is obsolete and shouldn't be
used anymore.  I'm not sure I believe that, but even if that's true,
it's clear that there's plenty of people who has an interest on it, so I
don't see that as an objection to reject this work.  So let's move on.


Your test data (crypt-shacrypt.sql) looks a bit short.  I noticed that
Drepper's SHA-crypt.txt file has a bunch of test lines (starting with
the "Test vectors from FIPS 180-2: appendix B.1." comment line, as well
as "appendix C.1" at the bottom) which perhaps could be incorporated
into the .sql script, to ensure correctness (or at least,
bug-compatibility with the reference implementation).  I'd also add a
note that Drepper's implementation is public domain in crypt-sha.c's
license block.

I think the "ascii_dollar" variable is a bit useless.  Why not just use the
literal $ sign where needed (or a DOLLAR_CHR if we feel like avoiding a
magic value there)?  Also, I wonder if it would be better to use a
StringInfo instead of a fixed-size buffer, which would probably make
some string manipulations easier ... Or maybe not, but let's not mix
strlcat calls with strncat calls with no comments about why.

Some of your elog(ERROR)s should probably be ereport(), and I'm not sure
we want all the elog(DEBUG1)s.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"El que vive para el futuro es un iluso, y el que vive para el pasado,
un imbécil" (Luis Adler, "Los tripulantes de la noche")

  11. Re: Modern SHA2- based password hashes for pgcrypto

    Bernd Helmle <mailings@oopsware.de> — 2025-01-14T09:34:38Z

    Hi Alvaro,

Am Montag, dem 13.01.2025 um 17:06 +0100 schrieb Alvaro Herrera:
> Hello
> 
> I was passing by and I noticed that this needs badly pgindent, and
> some
> comments are enumerations that would lose formatting once through it.
> For example, this would happen which is not good:
> 
>     /*
> -    * 1. Start digest A
> -    * 2. Add the password string to digest A
> -    * 3. Add the salt to digest A
> +    * 1. Start digest A 2. Add the password string to digest A 3.
> Add the
> +    * salt to digest A
>      */

I didn't think about testing pg_ident, sorry.

> 
> I suggest you can fix this by adding a "-" sign to the opening "/*"
> line
> so that pgindent doesn't mangle it (so it becomes /*- ).  This
> appears
> in several places.

Will try.

> 
> It's been said in my presence that pgcrypto is obsolete and shouldn't
> be
> used anymore.  I'm not sure I believe that, but even if that's true,
> it's clear that there's plenty of people who has an interest on it,
> so I
> don't see that as an objection to reject this work.  So let's move
> on.
> 

Oh, that's news to me. Is there a plan for it somewhere? I agree that
pgcrypto is widley used and needs a proper replacement when we decide
to deprecate it.

> 
> Your test data (crypt-shacrypt.sql) looks a bit short.  I noticed
> that
> Drepper's SHA-crypt.txt file has a bunch of test lines (starting with
> the "Test vectors from FIPS 180-2: appendix B.1." comment line, as
> well
> as "appendix C.1" at the bottom) which perhaps could be incorporated
> into the .sql script, to ensure correctness (or at least,
> bug-compatibility with the reference implementation).  I'd also add a
> note that Drepper's implementation is public domain in crypt-sha.c's
> license block.

Good idea. Will do.

> 
> I think the "ascii_dollar" variable is a bit useless.  Why not just
> use the
> literal $ sign where needed (or a DOLLAR_CHR if we feel like avoiding
> a



> magic value there)?  Also, I wonder if it would be better to use a
> StringInfo instead of a fixed-size buffer, which would probably make
> some string manipulations easier ... Or maybe not, but let's not mix
> strlcat calls with strncat calls with no comments about why.
> 

I am going to give it a try, probably helps to get rid of the
strncat()/strlcat() mess.

I originally thought about StringInfo but went with just the fixed
length character buffers since we access them directly anyways (and the
px_*/OpenSSL API needs char * ).

> Some of your elog(ERROR)s should probably be ereport(), and I'm not
> sure
> we want all the elog(DEBUG1)s.
> 

I added them during development. I am not married to them, but found
them useful during testing. If we come to the conclusion they're not
really that important, i drop them entirely.


Thanks for your comments!

	Bernd

  12. Re: Modern SHA2- based password hashes for pgcrypto

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-01-14T10:47:21Z

    Hello Bernd,

On 2025-Jan-14, Bernd Helmle wrote:

> > It's been said in my presence that pgcrypto is obsolete and
> > shouldn't be used anymore.  I'm not sure I believe that, but even if
> > that's true, it's clear that there's plenty of people who has an
> > interest on it, so I don't see that as an objection to reject this
> > work.  So let's move on.
> 
> Oh, that's news to me. Is there a plan for it somewhere? I agree that
> pgcrypto is widley used and needs a proper replacement when we decide
> to deprecate it.

I don't know about a plan, but
https://www.youtube.com/watch?v=pp6xdr3TuWM&t=1088s

> I originally thought about StringInfo but went with just the fixed
> length character buffers since we access them directly anyways (and the
> px_*/OpenSSL API needs char * ).

Note that you can access the char * via StringInfo->data.  Just don't
modify it without the StringInfo APIs.

> > Some of your elog(ERROR)s should probably be ereport(), and I'm not
> > sure we want all the elog(DEBUG1)s.
> 
> I added them during development. I am not married to them, but found
> them useful during testing. If we come to the conclusion they're not
> really that important, i drop them entirely.

Yeah, the DEBUGs are a pretty minor issue -- it's easy to remove them
afterwards.  For any actual error condition that's not a "can't happen"
one, please use ereport() for consistency.  (There's no translation
support for contrib, so they won't be translated anyway.)

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"No deja de ser humillante para una persona de ingenio saber
que no hay tonto que no le pueda enseñar algo." (Jean B. Say)

  13. Re: Modern SHA2- based password hashes for pgcrypto

    Bernd Helmle <mailings@oopsware.de> — 2025-01-14T11:41:38Z

    Am Dienstag, dem 14.01.2025 um 11:47 +0100 schrieb Alvaro Herrera:
> > Oh, that's news to me. Is there a plan for it somewhere? I agree
> > that
> > pgcrypto is widley used and needs a proper replacement when we
> > decide
> > to deprecate it.
> 
> I don't know about a plan, but
> https://www.youtube.com/watch?v=pp6xdr3TuWM&t=1088s


Hmm, I understand this like Peter tries to make a point regarding
transparent data encryption solutions, which pgcrypto doesn't serve
very well. Maybe it's not a point for total deprecation but that we
need additional, better solutions (for sure)...

Thanks,

	Bernd

  14. Re: Modern SHA2- based password hashes for pgcrypto

    Bernd Helmle <mailings@oopsware.de> — 2025-01-20T17:41:42Z

    Hi, 

Please find attached a reworked patch according Alvaro's comments.

Am Montag, dem 13.01.2025 um 17:06 +0100 schrieb Alvaro Herrera:
> Hello
> 
> I was passing by and I noticed that this needs badly pgindent, and
> some
> comments are enumerations that would lose formatting once through it.
> For example, this would happen which is not good:
> 
>     /*
> -    * 1. Start digest A
> -    * 2. Add the password string to digest A
> -    * 3. Add the salt to digest A
> +    * 1. Start digest A 2. Add the password string to digest A 3.
> Add the
> +    * salt to digest A
>      */
> 
> I suggest you can fix this by adding a "-" sign to the opening "/*"
> line
> so that pgindent doesn't mangle it (so it becomes /*- ).  This
> appears
> in several places.
> 

This is even documented:

https://www.postgresql.org/docs/devel/source-format.html

I ran pgindent locally and verified it works like you suggested.
> 

[...]

> Your test data (crypt-shacrypt.sql) looks a bit short.  I noticed
> that
> Drepper's SHA-crypt.txt file has a bunch of test lines (starting with
> the "Test vectors from FIPS 180-2: appendix B.1." comment line, as
> well
> as "appendix C.1" at the bottom) which perhaps could be incorporated
> into the .sql script, to ensure correctness (or at least,
> bug-compatibility with the reference implementation).  I'd also add a
> note that Drepper's implementation is public domain in crypt-sha.c's
> license block.
> 

These FIPS cases tests explicit against sha{256|512}_process_bytes() in
Drepper's code, which seem to be the equivalent to OpenSSL's
EVP_Digest*() API we're using. I am not sure it makes sense to adapt
them.

I left them out for now but adapted all the testcases for the password
hashes defined in the 2nd test cases to make sure we create the same
hashes.

> I think the "ascii_dollar" variable is a bit useless.  Why not just
> use the
> literal $ sign where needed (or a DOLLAR_CHR if we feel like avoiding
> a
> magic value there)?  Also, I wonder if it would be better to use a
> StringInfo instead of a fixed-size buffer, which would probably make
> some string manipulations easier ... Or maybe not, but let's not mix
> strlcat calls with strncat calls with no comments about why.

Result buffer via out_buf is wrapped into a StringInfo now.

> 
> Some of your elog(ERROR)s should probably be ereport(), and I'm not
> sure
> we want all the elog(DEBUG1)s.

Done, but i kept some of the DEBUG output for now. I am not sure if i
really should set the errcode for ereport() explicitly, but i did on
some places where i thought it might be useful.

This patch version also changes the original behavior of crypt() when
called for sha{256|512}crypt hashes. Before we didn't accept values for
the rounds parameter exceeding the minimum and maximum values. This was
along with gen_salt(), which also errors out in such cases. But
Drepper's code accepts them and changes them behind the scenes either
to the minimum or maximum and calculates the password hash based on
them, depending on the exceeded boundary.

So when passing a user defined salt string to crypt(), we do the same
now but i added a NOTICE to indicate that we changed the user submitted
parameter behind the scene. This also enables us to stress
px_crypt_shacrypt() with the same test cases as Drepper's code.



Thanks,

	Bernd

  15. Re: Modern SHA2- based password hashes for pgcrypto

    Japin Li <japinli@hotmail.com> — 2025-01-23T13:36:02Z

    On Mon, 20 Jan 2025 at 18:41, Bernd Helmle <mailings@oopsware.de> wrote:
> Hi, 
>
> Please find attached a reworked patch according Alvaro's comments.
>
> Am Montag, dem 13.01.2025 um 17:06 +0100 schrieb Alvaro Herrera:
>> Hello
>> 
>> I was passing by and I noticed that this needs badly pgindent, and
>> some
>> comments are enumerations that would lose formatting once through it.
>> For example, this would happen which is not good:
>> 
>>     /*
>> -    * 1. Start digest A
>> -    * 2. Add the password string to digest A
>> -    * 3. Add the salt to digest A
>> +    * 1. Start digest A 2. Add the password string to digest A 3.
>> Add the
>> +    * salt to digest A
>>      */
>> 
>> I suggest you can fix this by adding a "-" sign to the opening "/*"
>> line
>> so that pgindent doesn't mangle it (so it becomes /*- ).  This
>> appears
>> in several places.
>> 
>
> This is even documented:
>
> https://www.postgresql.org/docs/devel/source-format.html
>
> I ran pgindent locally and verified it works like you suggested.
>> 
>
> [...]
>
>> Your test data (crypt-shacrypt.sql) looks a bit short.  I noticed
>> that
>> Drepper's SHA-crypt.txt file has a bunch of test lines (starting with
>> the "Test vectors from FIPS 180-2: appendix B.1." comment line, as
>> well
>> as "appendix C.1" at the bottom) which perhaps could be incorporated
>> into the .sql script, to ensure correctness (or at least,
>> bug-compatibility with the reference implementation).  I'd also add a
>> note that Drepper's implementation is public domain in crypt-sha.c's
>> license block.
>> 
>
> These FIPS cases tests explicit against sha{256|512}_process_bytes() in
> Drepper's code, which seem to be the equivalent to OpenSSL's
> EVP_Digest*() API we're using. I am not sure it makes sense to adapt
> them.
>
> I left them out for now but adapted all the testcases for the password
> hashes defined in the 2nd test cases to make sure we create the same
> hashes.
>
>> I think the "ascii_dollar" variable is a bit useless.  Why not just
>> use the
>> literal $ sign where needed (or a DOLLAR_CHR if we feel like avoiding
>> a
>> magic value there)?  Also, I wonder if it would be better to use a
>> StringInfo instead of a fixed-size buffer, which would probably make
>> some string manipulations easier ... Or maybe not, but let's not mix
>> strlcat calls with strncat calls with no comments about why.
>
> Result buffer via out_buf is wrapped into a StringInfo now.
>
>> 
>> Some of your elog(ERROR)s should probably be ereport(), and I'm not
>> sure
>> we want all the elog(DEBUG1)s.
>
> Done, but i kept some of the DEBUG output for now. I am not sure if i
> really should set the errcode for ereport() explicitly, but i did on
> some places where i thought it might be useful.
>
> This patch version also changes the original behavior of crypt() when
> called for sha{256|512}crypt hashes. Before we didn't accept values for
> the rounds parameter exceeding the minimum and maximum values. This was
> along with gen_salt(), which also errors out in such cases. But
> Drepper's code accepts them and changes them behind the scenes either
> to the minimum or maximum and calculates the password hash based on
> them, depending on the exceeded boundary.
>
> So when passing a user defined salt string to crypt(), we do the same
> now but i added a NOTICE to indicate that we changed the user submitted
> parameter behind the scene. This also enables us to stress
> px_crypt_shacrypt() with the same test cases as Drepper's code.
>
>

Hi, Bernd Helmle

Thanks for updating the patch.  Here are some comments for v3 patch.

1.
+char *
+_crypt_gensalt_sha256_rn(unsigned long count,
+                                                const char *input, int size,
+                                                char *output, int output_size)
+{
+       memset(output, 0, output_size);
+       /* set magic byte for sha512crypt */

s/sha512crypt/sha256crypt/g

2.
Both PX_SHACRYPT_BUF_LEN and PX_SHACRYPT_DIGEST_MAX_LENGTH are macros for
length; however, they use different suffixes.  How about unifying them?
I prefer to use the LEN suffix.

3.
+       if ((dec_salt_binary[0] != '$')
+               && (dec_salt_binary[2] != '$'))
+       {
+               ereport(ERROR,
+                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                                errmsg("invalid format of salt")),
+                               errhint("magic byte format for shacrypt is either \"$5$\" or \"$6$\""));
+       }
...
+       if (strncmp(dec_salt_binary, magic_bytes[0], strlen(magic_bytes[0])) == 0)
...
+       else if (strncmp(dec_salt_binary, magic_bytes[1], strlen(magic_bytes[1])) == 0)

IMO, we could change the above code as:

+       if (strncmp(dec_salt_binary, magic_bytes[0], strlen(magic_bytes[0])) == 0)
...
+       else if (strncmp(dec_salt_binary, magic_bytes[1], strlen(magic_bytes[1])) == 0)
...
+       else
+       {
+               ereport(ERROR,
+                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                                errmsg("invalid format of salt")),
+                               errhint("magic byte format for shacrypt is either \"$5$\" or \"$6$\""));
+       }

4.
+/* Default number of rounds of shacrypt if not explicitly specified.  */
+#define PX_SHACRYPT_ROUNDS_DEFAULT 5000

It seems you were missing the `l` suffix, since both PX_SHACRYPT_ROUNDS_MIN and
PX_SHACRYPT_ROUNDS_MAX have this suffix.

5.
Does the following work as expected?

postgres=# select crypt('hello', '$5$$6$rounds=10000$/Zg436s2vmTwsoSz');
                      crypt
-------------------------------------------------
 $5$$TCRu/ts4Npu8OJyeWy2WnUHCe/6bKVMSi0sROUrPh48
(1 row)

postgres=# select crypt('hello', '$5$rounds=10000$/Zg436s2vmTwsoSz');
                                    crypt
------------------------------------------------------------------------------
 $5$rounds=10000$/Zg436s2vmTwsoSz$N4Ad0oGzE6ak30z4EGSEXOc2OXQOpmauicPT3560lY2
(1 row)

According to the documentation, I think the first should output as following:

$5$rounds=5000$TCRu/ts4Npu8OJyeWy2WnUHCe/6bKVMSi0sROUrPh48

-- 
Regrads,
Japin Li

  16. Re: Modern SHA2- based password hashes for pgcrypto

    Bernd Helmle <mailings@oopsware.de> — 2025-01-24T17:14:45Z

    Am Donnerstag, dem 23.01.2025 um 21:36 +0800 schrieb Japin Li:

Thanks for your review again. I am going to work on the other items,
but this one might need further discussion:

> 5.
> Does the following work as expected?
> 
> postgres=# select crypt('hello',
> '$5$$6$rounds=10000$/Zg436s2vmTwsoSz');
>                       crypt
> -------------------------------------------------
>  $5$$TCRu/ts4Npu8OJyeWy2WnUHCe/6bKVMSi0sROUrPh48
> (1 row)
> 
> postgres=# select crypt('hello', '$5$rounds=10000$/Zg436s2vmTwsoSz');
>                                     crypt
> ---------------------------------------------------------------------
> ---------
>  $5$rounds=10000$/Zg436s2vmTwsoSz$N4Ad0oGzE6ak30z4EGSEXOc2OXQOpmauicP
> T3560lY2
> (1 row)
> 
> According to the documentation, I think the first should output as
> following:
> 
> $5$rounds=5000$TCRu/ts4Npu8OJyeWy2WnUHCe/6bKVMSi0sROUrPh48

So the password hash/salt string used here is broken. '$' is exclusive
to define the boundaries of each part of the password hash string, so
it's very ambiguous what this preample in your example should do.

Note that the code i'm using is nearly identical to what the current
implementation of 'md5' does:

bernd@localhost:bernd :1 #= select crypt('hello',
'$1$$6$/Zg436s2vmTwsoSz');
DEBUG:  md5 salt len = 0, salt = $6$/Zg436s2vmTwsoSz
           crypt            
────────────────────────────
 $1$$/PWPe740uvaQxKzRH.Zxj1
(1 row)

The DEBUG output was added by me. Since the salt len is evaluated to 0,
effectively no salt is handled at all.

So we behave exactly the same way as px_crypt_md5(): It stops after the
first '$' after the magic byte preamble. For shacrypt, this could be
the next '$' after the closing one of the non-mandatory 'rounds'
option, but with your example this doesn't happen since it gets never
parsed. The salt length will be set to 0.

Furthermore, EVP_DigestUpdate() will do nothing if a count parameter
with 0 is handed over, which happens in our case here when adding the
salt. It happily returns 1 (success), so px_update_digest() will never
ERROR out.

Btw., blowfish seems more strict about this: playing around with it a
little i couldn't make it accept a garbaged password hash string like
the current example.

So, I am not sure what to do about this. Originally i had the feeling
we just throw an ERROR if the salt couldn't be evaluated properly, e.g
when we couldn't examine any salt by checking its length being larger
than 0. And '$' is not even a valid character within a salt string. But
then decided to go along with px_crypt_md5(). 

In short: If you throw a garbaged password hash string as the 2nd
argument to crypt() for md5/shacrypt, you currently might get something
obscure back.

I'd like to hear other opinions on this issue.


	Bernd

  17. Re: Modern SHA2- based password hashes for pgcrypto

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-01-24T18:06:21Z

    On 2025-Jan-24, Bernd Helmle wrote:

> So we behave exactly the same way as px_crypt_md5(): It stops after the
> first '$' after the magic byte preamble. For shacrypt, this could be
> the next '$' after the closing one of the non-mandatory 'rounds'
> option, but with your example this doesn't happen since it gets never
> parsed. The salt length will be set to 0.

IMO silently using no salt or 0 iterations because the input is somewhat
broken is bad security and should be rejected.  If we did so in the past
without noticing, that's bad already, but we should not replicate that
behavior any further.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Doing what he did amounts to sticking his fingers under the hood of the
implementation; if he gets his fingers burnt, it's his problem."  (Tom Lane)

  18. Re: Modern SHA2- based password hashes for pgcrypto

    Japin Li <japinli@hotmail.com> — 2025-01-27T04:28:16Z

    On Fri, 24 Jan 2025 at 19:06, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2025-Jan-24, Bernd Helmle wrote:
>
>> So we behave exactly the same way as px_crypt_md5(): It stops after the
>> first '$' after the magic byte preamble. For shacrypt, this could be
>> the next '$' after the closing one of the non-mandatory 'rounds'
>> option, but with your example this doesn't happen since it gets never
>> parsed. The salt length will be set to 0.
>
> IMO silently using no salt or 0 iterations because the input is somewhat
> broken is bad security and should be rejected.  If we did so in the past
> without noticing, that's bad already, but we should not replicate that
> behavior any further.
>

I agree with this point, so maybe we should fix this for px_crypt_md().

-- 
Regrads,
Japin Li

  19. Re: Modern SHA2- based password hashes for pgcrypto

    Bernd Helmle <mailings@oopsware.de> — 2025-01-28T10:25:51Z

    Am Freitag, dem 24.01.2025 um 19:06 +0100 schrieb Alvaro Herrera:
> > So we behave exactly the same way as px_crypt_md5(): It stops after
> > the
> > first '$' after the magic byte preamble. For shacrypt, this could
> > be
> > the next '$' after the closing one of the non-mandatory 'rounds'
> > option, but with your example this doesn't happen since it gets
> > never
> > parsed. The salt length will be set to 0.
> 
> IMO silently using no salt or 0 iterations because the input is
> somewhat
> broken is bad security and should be rejected.  If we did so in the
> past
> without noticing, that's bad already, but we should not replicate
> that
> behavior any further.

Following Japin's example we parse the broken input after the magic
byte into

$6$rounds=10000$/Zg436s2vmTwsoSz

So this is what gets passed over to the next steps after extracting the
magic byte.

We then try to parse the optional rounds option, which is expected to
be at the very first characters at this stage, which isn't true. In
that case we fall back to default 5000 iterations. So no problem with
iterations here. Note that we don't output the rounds option in that
case. I adapted this from Drepper's code to be compatible with the
tests (it suppresses the rounds option in case the default is used). I
forgot to adjust the docs about this change.

But we don't extract the salt because of the first '$', which confused
the input handling that there isn't anything interesting for it. And
that get unnoticed currently, which might be a problem for the caller.
Well, you see that something is wrong when looking at the result, but
if that happens programmatically it might be hidden.

I've looked what others do:

Drepper's code with the input above produces the following:

$5$=10000$cWSkAaJa1mL912.HQqjIhODJ9b3S7hmgsb/k9Efp7.7

It parses the salt from the input as "=10000".

Python's passlib is very strict when it comes to supported characters
within a salt string. It rejects everything thats not matching '[./0-
9A-Za-z]'. So when you provide the example above you get

  File "/usr/lib/python3.13/site-packages/passlib/utils/handlers.py",
line 1459, in _norm_salt
    raise ValueError("invalid characters in %s salt" % cls.name)
ValueError: invalid characters in sha256_crypt salt

openssl-passwd does this:

echo test | openssl passwd -5 -stdin -salt
'$6$rounds=10000$/Zg436s2vmTwsoSz'

$5$$6$rounds=10000$$BFtTxJrvpb/ra8SnnXkiNCJ1HGZ3OOmwvyQ2TJZx44B

It absorbs everything up to 16 bytes and uses that as the salt string.

Interestingly, python-passlib and Drepper's code accept both empty
salt, openssl-passwd seems to be buggy:

echo test | openssl passwd -5 -stdin -salt ''                         
<NULL>

So at least, they do *something* with the input and don't silently omit
salting the input passphrase, but the results aren't the same hash.

So i think we have two options: adapt Drepper's code and throw away
everything or be very strict like python-passlib. I myself can find
support for both, so not sure.


	Bernd

  20. Re: Modern SHA2- based password hashes for pgcrypto

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-06T10:20:31Z

    On 2025-Jan-28, Bernd Helmle wrote:

> Python's passlib is very strict when it comes to supported characters
> within a salt string. It rejects everything thats not matching '[./0-
> 9A-Za-z]'. So when you provide the example above you get

The reason it uses these chars is that in their scheme the salt bytes
are base64-encoded.

The passlib docs has this page about the "modular crypt format":
https://passlib.readthedocs.io/en/stable/modular_crypt_format.html

and they point this other page as a "modern, non-ambiguous standard":
https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md
About the salt, this last document says:

  The role of salts is to achieve uniqueness. A random salt is fine for
  that as long as its length is sufficient; a 16-byte salt would work
  well (by definition, UUID are very good salts, and they encode over
  exactly 16 bytes). 16 bytes encode as 22 characters in B64. Functions
  should disallow salt values that are too small for security (4 bytes
  should be viewed as an absolute minimum).

This "Password Hashing Competition" organization hardly seems an
authority though.  It'd be great to have an IETF standard about this ...

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

  21. Re: Modern SHA2- based password hashes for pgcrypto

    Japin Li <japinli@hotmail.com> — 2025-02-07T08:17:11Z

    On Thu, 06 Feb 2025 at 11:20, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2025-Jan-28, Bernd Helmle wrote:
>
>> Python's passlib is very strict when it comes to supported characters
>> within a salt string. It rejects everything thats not matching '[./0-
>> 9A-Za-z]'. So when you provide the example above you get
>
> The reason it uses these chars is that in their scheme the salt bytes
> are base64-encoded.
>
> The passlib docs has this page about the "modular crypt format":
> https://passlib.readthedocs.io/en/stable/modular_crypt_format.html
>
> and they point this other page as a "modern, non-ambiguous standard":
> https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md
> About the salt, this last document says:
>
>   The role of salts is to achieve uniqueness. A random salt is fine for
>   that as long as its length is sufficient; a 16-byte salt would work
>   well (by definition, UUID are very good salts, and they encode over
>   exactly 16 bytes). 16 bytes encode as 22 characters in B64. Functions
>   should disallow salt values that are too small for security (4 bytes
>   should be viewed as an absolute minimum).
>
> This "Password Hashing Competition" organization hardly seems an
> authority though.  It'd be great to have an IETF standard about this ...

Yeah.

Since there is no standard, how do we handle this?  I prefer to use the strict
mode like passlib.

-- 
Regrads,
Japin Li

  22. Re: Modern SHA2- based password hashes for pgcrypto

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-07T09:31:44Z

    On 2025-Feb-07, Japin Li wrote:

> Since there is no standard, how do we handle this?  I prefer to use
> the strict mode like passlib.

I definitely like that passlib have documented their thought process
thoroughly.

I think using their strict mode is good on principle, but if we're going
to do that, then the salt string should not be used verbatim, but
instead base64-decoded first to get the actual salt bytes, like they do.
Does this break compabitibility with other systems?  Are
passlib-generated password hashes incompatible with, say, "openssl
passwd" which you (Bernd) mentioned at the beginning of the thread?
Maybe if the password hashes are no longer compatible, then we should
ditch the idea of restricting salts to base64 chars and accept the whole
range of bytes, like Drepper.

But in any case ISTM we should reject, as they suggest, the use of less
than 4 bytes of salt (and should perhaps settle for a default of 16, as
passlib suggests).  I suppose this is why passlib returns NULL with
empty salt.  What we should do in that case IMO is ereport(ERROR).  

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Porque Kim no hacía nada, pero, eso sí,
con extraordinario éxito" ("Kim", Kipling)

  23. Re: Modern SHA2- based password hashes for pgcrypto

    Japin Li <japinli@hotmail.com> — 2025-02-08T02:16:40Z

    On Fri, 07 Feb 2025 at 10:31, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2025-Feb-07, Japin Li wrote:
>
>> Since there is no standard, how do we handle this?  I prefer to use
>> the strict mode like passlib.
>
> I definitely like that passlib have documented their thought process
> thoroughly.
>
> I think using their strict mode is good on principle, but if we're going
> to do that, then the salt string should not be used verbatim, but
> instead base64-decoded first to get the actual salt bytes, like they do.
> Does this break compabitibility with other systems?  Are
> passlib-generated password hashes incompatible with, say, "openssl
> passwd" which you (Bernd) mentioned at the beginning of the thread?
> Maybe if the password hashes are no longer compatible, then we should
> ditch the idea of restricting salts to base64 chars and accept the whole
> range of bytes, like Drepper.

Thinking about compatibility, the Drepper's behavior is a good choice.

Or we can accept the whole range of bytes except the $ character since it is
a separator.  Of course, it is also not compatible with other systems.

>
> But in any case ISTM we should reject, as they suggest, the use of less
> than 4 bytes of salt (and should perhaps settle for a default of 16, as
> passlib suggests).  I suppose this is why passlib returns NULL with
> empty salt.  What we should do in that case IMO is ereport(ERROR).

+1

-- 
Regrads,
Japin Li

  24. Re: Modern SHA2- based password hashes for pgcrypto

    Bernd Helmle <mailings@oopsware.de> — 2025-03-11T16:58:02Z

    Hi,

> I definitely like that passlib have documented their thought process
> thoroughly.
> 

Please find attached v4 of this patch. I added the following changes:

- Check for non-supported characters in the salt like passlib does.
- Check for reserved tokens when parsing the salt string (i find this
very strict, but it covers the cases Japin Li pointed out).

I didn't implement being more strict when it comes to the salt length:
the salt might be provided from external sources and the password hash
is just used for validating within the database.

When hashing the password within postgres, users always should use
gen_salt(), which generates a reasonable salt string. 

Maybe, in case of empty salts, we should issue a WARNING instead of
erroring out and put additional documentation on how to use it right.

> I think using their strict mode is good on principle, but if we're
> going
> to do that, then the salt string should not be used verbatim, but
> instead base64-decoded first to get the actual salt bytes, like they
> do.
> Does this break compabitibility with other systems?  Are
> passlib-generated password hashes incompatible with, say, "openssl
> passwd" which you (Bernd) mentioned at the beginning of the thread?
> Maybe if the password hashes are no longer compatible, then we should
> ditch the idea of restricting salts to base64 chars and accept the
> whole
> range of bytes, like Drepper.
> 
> But in any case ISTM we should reject, as they suggest, the use of
> less
> than 4 bytes of salt (and should perhaps settle for a default of 16,
> as
> passlib suggests).  I suppose this is why passlib returns NULL with
> empty salt.  What we should do in that case IMO is ereport(ERROR). 
> 


Hmm, i didn't understand that passlib does decode them first, i thought
they use it encoded... at least, in our current form we're pretty much
compatible with Drepper, passlib and OpenSSL, as far as i tested:

Empty salt
----------

# Drepper reference code

shacrypt password "" sha512crypt
$6$$bLTg4cpho8PIUrjfsE7qlU08Qx2UEfw..xOc6I1wpGVtyVYToGrr7BzRdAAnEr5lYFr
1Z9WcCf1xNZ1HG9qFW1

# Patch

SELECT crypt('password', '$6$$');
                                           crypt                      
───────────────────────────────────────────────────────────────────────
─────────────────────
$6$$bLTg4cpho8PIUrjfsE7qlU08Qx2UEfw..xOc6I1wpGVtyVYToGrr7BzRdAAnEr5lYFr1
Z9WcCf1xNZ1HG9qFW1
(1 row)

# Passlib

from passlib.hash import sha256_crypt,sha512_crypt
hash = sha512_crypt.using(salt='').using(rounds=5000).hash("password")
print(hash)
$6$$bLTg4cpho8PIUrjfsE7qlU08Qx2UEfw..xOc6I1wpGVtyVYToGrr7BzRdAAnEr5lYFr
1Z9WcCf1xNZ1HG9qFW1

# OpenSSL

echo password | openssl passwd -6 -stdin -salt ''             
<NULL>

Short salt
----------

# Drepper reference code

./shacrypt password abcdef sha512crypt
$6$abcdef$2yYFwtar3.rlLovG0bXbxXMjZe7Z/1EhQ0gEs0nXalsCW04p2iy6unkvbrBFH
1SXexabbdNXXOO11F7sdyurk/

# Patch

SELECT crypt('password', '$6$abcdef$');
                                              crypt                   
───────────────────────────────────────────────────────────────────────
───────────────────────────
$6$abcdef$2yYFwtar3.rlLovG0bXbxXMjZe7Z/1EhQ0gEs0nXalsCW04p2iy6unkvbrBFH1
SXexabbdNXXOO11F7sdyurk/
(1 row)

# Passlib

from passlib.hash import sha256_crypt,sha512_crypt
hash =
sha512_crypt.using(salt='abcdef').using(rounds=5000).hash("password")
print(hash)
$6$abcdef$2yYFwtar3.rlLovG0bXbxXMjZe7Z/1EhQ0gEs0nXalsCW04p2iy6unkvbrBFH
1SXexabbdNXXOO11F7sdyurk/

# OpenSSL

echo password | openssl passwd -6 -stdin -salt 'abcdef'
$6$abcdef$2yYFwtar3.rlLovG0bXbxXMjZe7Z/1EhQ0gEs0nXalsCW04p2iy6unkvbrBFH
1SXexabbdNXXOO11F7sdyurk/

Salt > MAXLEN(16 Bytes)
-----------------------

# Drepper reference code
(truncates salt)

shacrypt password abcdefghijklmnopqrstuvwxyz sha512crypt
$6$abcdefghijklmnop$0aenUFHf897F9u0tURIHOeACWajSuVGa7jgJGyq.DKZm/WXl/IZ
FvPbneFydBjomEOgM.Sh1m0L3KsS1.H5b//

# Patch
(truncates salt)

SELECT crypt('password', '$6$abcdefghijklmnopqrstuvwxyz$');
                                                   crypt              
───────────────────────────────────────────────────────────────────────
─────────────────────────────────────
$6$abcdefghijklmnop$0aenUFHf897F9u0tURIHOeACWajSuVGa7jgJGyq.DKZm/WXl/IZF
vPbneFydBjomEOgM.Sh1m0L3KsS1.H5b//
(1 row)

# Passlib

Errors out with

ValueError: salt too large (sha512_crypt requires <= 16 chars)

# OpenSSL
(truncates salt)

echo password | openssl passwd -6 -stdin -salt
'abcdefghijklmnopqrstuvwxyz'
$6$abcdefghijklmnop$0aenUFHf897F9u0tURIHOeACWajSuVGa7jgJGyq.DKZm/WXl/IZ
FvPbneFydBjomEOgM.Sh1m0L3KsS1.H5b//

Salt with "invalid" character
-----------------------------

# Drepper reference code

./shacrypt password abcdefghi%jklmno sha512crypt
$6$abcdefghi%jklmno$LMN/V1pW97IoK0rWSDVqCo9EYd6zpqP0TdTX9.cxFAFqsdSMWQM
jehkmMtDzL36VBKeG6dg.kFAQKoFvZpK0G.

# Patch
current version V4 (attached)

Errors out:

SELECT crypt('password', '$6$abcdefghi%jklmno$');
FEHLER:  invalid character in salt string: "%"
Time: 0,217 ms

# Passlib

Errors out

ValueError: invalid characters in sha512_crypt salt

# OpenSSL

echo password | openssl passwd -6 -stdin -salt 'abcdefghi%jklmno'     
$6$abcdefghi%jklmno$LMN/V1pW97IoK0rWSDVqCo9EYd6zpqP0TdTX9.cxFAFqsdSMWQM
jehkmMtDzL36VBKeG6dg.kFAQKoFvZpK0G.


So the hashes we produce are identical, but with being more strict we
differ in the handling of the provided salt.

	Bernd

  25. Re: Modern SHA2- based password hashes for pgcrypto

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-04-03T18:39:18Z

    On 2025-Mar-11, Bernd Helmle wrote:

> Please find attached v4 of this patch. I added the following changes:
> 
> - Check for non-supported characters in the salt like passlib does.
> - Check for reserved tokens when parsing the salt string (i find this
> very strict, but it covers the cases Japin Li pointed out).
> 
> I didn't implement being more strict when it comes to the salt length:
> the salt might be provided from external sources and the password hash
> is just used for validating within the database.
> 
> When hashing the password within postgres, users always should use
> gen_salt(), which generates a reasonable salt string. 

This sounds reasonable to me.

> Maybe, in case of empty salts, we should issue a WARNING instead of
> erroring out and put additional documentation on how to use it right.

I don't know, that doesn't seem ideal to me, because it's very easy to
run stuff and never see the warnings.  If we find that people are
desperate to use empty salts, we can relax that later (turn the error to
a warning), but I'd rather not have it in the first cut.

> Hmm, i didn't understand that passlib does decode them first, i thought
> they use it encoded... at least, in our current form we're pretty much
> compatible with Drepper, passlib and OpenSSL, as far as i tested:

I am ready to believe that I misinterpreted what I read.

> So the hashes we produce are identical, but with being more strict we
> differ in the handling of the provided salt.

Okay.


I can offer a few cosmetic changes.  0001 is a pgindent run, and 0002 is
some manual adjustments after that.  There are only two nontrivial
changes

1. the calculation for rounds was using type long, which is confusing
because the range is different according to the platform.  Since it's
limited by the macro definitions to no more than 999999999, we can make
it an int32 instead.  So we use strtoint() instead of strtoul() to parse
the value, and remove the "l" suffixes from the macros that define the
limits and default, which were bugging me a bit when used in the
gen_list struct.

2. I changed "if (block % 3)" to "if ((block % 3) != 0)".

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"El destino baraja y nosotros jugamos" (A. Schopenhauer)

  26. Re: Modern SHA2- based password hashes for pgcrypto

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-04-05T09:47:32Z

    Hello,

I triggered a run of this on CI on all platforms.  It seems to have gone
well, so unless I hear complaints, I intend to get this out later today.

https://cirrus-ci.com/build/4613871211642880

Thanks,

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

  27. Re: Modern SHA2- based password hashes for pgcrypto

    Bernd Helmle <mailings@oopsware.de> — 2025-04-05T10:45:02Z

    Am Donnerstag, dem 03.04.2025 um 20:39 +0200 schrieb Alvaro Herrera:
> 
> > Maybe, in case of empty salts, we should issue a WARNING instead of
> > erroring out and put additional documentation on how to use it
> > right.
> 
> I don't know, that doesn't seem ideal to me, because it's very easy
> to
> run stuff and never see the warnings.  If we find that people are
> desperate to use empty salts, we can relax that later (turn the error
> to
> a warning), but I'd rather not have it in the first cut.
> 

That's a good idea, Let's go with that. Thanks again for working on
this.

> > Hmm, i didn't understand that passlib does decode them first, i
> > thought
> > they use it encoded... at least, in our current form we're pretty
> > much
> > compatible with Drepper, passlib and OpenSSL, as far as i tested:
> 
> I am ready to believe that I misinterpreted what I read.
> 

I hope i didn't parse it wrong either. But i didn't see forcing
something like this according in either passlib and Drepper's code.
Maybe we need have to look closer again ...

[...]

> 
> 
> I can offer a few cosmetic changes.  0001 is a pgindent run, and 0002
> is
> some manual adjustments after that.  There are only two nontrivial
> changes
> 
> 1. the calculation for rounds was using type long, which is confusing
> because the range is different according to the platform.  Since it's
> limited by the macro definitions to no more than 999999999, we can
> make
> it an int32 instead.  So we use strtoint() instead of strtoul() to
> parse
> the value, and remove the "l" suffixes from the macros that define
> the
> limits and default, which were bugging me a bit when used in the
> gen_list struct.

+1



-- 
Bernd Helmle

Blücherstrasse 17
41061 Mönchengladbach

Tel.: +49 172 726 99 66

  28. Re: Modern SHA2- based password hashes for pgcrypto

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-04-05T17:22:58Z

    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"

  29. Re: Modern SHA2- based password hashes for pgcrypto

    Bernd Helmle <mailings@oopsware.de> — 2025-04-06T17:39:56Z

    Am Samstag, dem 05.04.2025 um 19:22 +0200 schrieb Alvaro Herrera:
> Hello,
> 
> I have pushed this now, hoping it won't explode.
> 
> Thanks!

Very cool, i keep my fingers crossed.

Thanks,
	Bernd

  30. Re: Modern SHA2- based password hashes for pgcrypto

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-06T19:43:36Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> 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

  31. Re: Modern SHA2- based password hashes for pgcrypto

    Andres Freund <andres@anarazel.de> — 2025-04-07T03:02:50Z

    Hi,

On 2025-04-05 19:22:58 +0200, Alvaro Herrera wrote:
> 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

  32. Re: Modern SHA2- based password hashes for pgcrypto

    Bernd Helmle <mailings@oopsware.de> — 2025-04-07T07:03:30Z

    Am Sonntag, dem 06.04.2025 um 15:43 -0400 schrieb Tom Lane:
> 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)
> 

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.

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

That's an oversight by me.

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

Thanks

	Bernd

  33. Re: Modern SHA2- based password hashes for pgcrypto

    Bernd Helmle <mailings@oopsware.de> — 2025-04-07T07:09:39Z

    Am Sonntag, dem 06.04.2025 um 23:02 -0400 schrieb Andres Freund:
> Hi,
> 
> On 2025-04-05 19:22:58 +0200, Alvaro Herrera wrote:
> > 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...

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

  34. Re: Modern SHA2- based password hashes for pgcrypto

    Bernd Helmle <mailings@oopsware.de> — 2025-04-07T11:48:49Z

    Am Sonntag, dem 06.04.2025 um 15:43 -0400 schrieb Tom Lane:
> 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?)

Here is a patch that tries to address all these issues (including
Andres' report). I've adjusted the error message and use ereport(), so
it might be more useful if we deal with not just single byte letters.

I've also changed _crypt_itoa64 from unsigned char to char, since this
seems what strchr() expects (at least on my machine) and we don't deal
specifically elsewhere with that.


Thanks,
	Bernd

  35. Re: Modern SHA2- based password hashes for pgcrypto

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-07T17:50:35Z

    Bernd Helmle <mailings@oopsware.de> writes:
> Here is a patch that tries to address all these issues (including
> Andres' report). I've adjusted the error message and use ereport(), so
> it might be more useful if we deal with not just single byte letters.

I'd like to get mamba back to green, so I'll take care of this
(unless Alvaro is already on it?)

			regards, tom lane

  36. Re: Modern SHA2- based password hashes for pgcrypto

    Andres Freund <andres@anarazel.de> — 2025-04-07T18:00:43Z

    Hi,

On 2025-04-07 09:09:39 +0200, Bernd Helmle wrote:
> Am Sonntag, dem 06.04.2025 um 23:02 -0400 schrieb Andres Freund:
> > On 2025-04-05 19:22:58 +0200, Alvaro Herrera wrote:
> > > 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...
> 
> 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.

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

  37. Re: Modern SHA2- based password hashes for pgcrypto

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-04-07T18:15:43Z

    
On Mon, Apr 7, 2025, at 7:50 PM, Tom Lane wrote:
> I'd like to get mamba back to green, so I'll take care of this
> (unless Alvaro is already on it?)

Please feel free, thank you.

  38. Re: Modern SHA2- based password hashes for pgcrypto

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-07T18:16:55Z

    I wrote:
> Bernd Helmle <mailings@oopsware.de> writes:
>> Here is a patch that tries to address all these issues (including
>> Andres' report). I've adjusted the error message and use ereport(), so
>> it might be more useful if we deal with not just single byte letters.

> I'd like to get mamba back to green, so I'll take care of this

And done.  FYI, we actually have a standard practice for error
messages that want to complain about a single character without
assuming it's a single-byte character:

                ereport(ERROR,
                        errcode(ERRCODE_INVALID_PARAMETER_VALUE),
                        errmsg("invalid character in salt string: \"%.*s\"",
                               pg_mblen(ep), ep));

			regards, tom lane