Обсуждение: Re: Modern SHA2- based password hashes for pgcrypto

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

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

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)