Re: Modern SHA2- based password hashes for pgcrypto

On Mon, 20 Jan 2025 at 18:41, Bernd Helmle <mailings(at)oopsware(dot)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