Re: Modern SHA2- based password hashes for pgcrypto

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(at)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