Re: Modern SHA2- based password hashes for pgcrypto

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