From: Japin Li <japinli@hotmail.com>
To: Bernd Helmle <mailings@oopsware.de>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>,  PostgreSQL Development
 <pgsql-hackers@postgresql.org>
Subject: Re: Modern SHA2- based password hashes for pgcrypto
In-Reply-To: <3e48cd7c02f437ed980ee57340cb5c54f60e359c.camel@oopsware.de>
	(Bernd Helmle's message of "Mon, 20 Jan 2025 18:41:42 +0100")
References: <202501131606.h6ctz533qhy6@alvherre.pgsql>
	<3e48cd7c02f437ed980ee57340cb5c54f60e359c.camel@oopsware.de>
Date: Thu, 23 Jan 2025 21:36:02 +0800
Message-ID:
 <ME0P300MB0445F3242216930DE6D44A24B6E02@ME0P300MB0445.AUSP300.PROD.OUTLOOK.COM>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
X-MS-PublicTrafficType: Email
X-MS-TrafficTypeDiagnostic: ME0P300MB0445:EE_|SY8P300MB0759:EE_
X-MS-Office365-Filtering-Correlation-Id: b372f25d-9c40-4326-e7ed-08dd3bb2e344
X-Microsoft-Antispam:
	BCL:0;ARA:14566002|7092599003|15080799006|19110799003|6090799003|5072599009|461199028|8060799006|10035399004|19061999003|3412199025|440099028|12071999003|21061999003;
X-Microsoft-Antispam-Message-Info:
	=?utf-8?B?eTRDc013bEVuSDFiRXRaVGlBbEhLR2phbU1CenBzbTY4VlNiblNML2hxdWUw?=
 =?utf-8?B?SEY1N1pVTnZYWWk0VzMveTJaanJaU09lMTVKYm4zNGovbzNBUVJkcXU2QkFB?=
 =?utf-8?B?NEQyZE9yUEI2KzE1V1pwV2N1cHBMazRYTDIvVjVCOGl5YklFUC83eERlMnhz?=
 =?utf-8?B?R0kyWTFCZFpCSDB1aFlGbjlqWHF3bThVM01mL3owcnZhWlQ5b1hKVjVsdkJz?=
 =?utf-8?B?bUhmbVFqVjZOYU5KQ2ptY21tMVEraXU2VlJUQlZFTklBYVVnU3FCOVJDZUJa?=
 =?utf-8?B?Si9hOTl3Qm9VdUloUXdoUUFSU2cxb2U2QnUrOUttb2VMcGd1R0hZeVdQTW9Q?=
 =?utf-8?B?OEpySmZjNThHQzFNN2tndVQ5S1hBRmphZ1g0S283ZWVoeTVvTG5KTlBXb05i?=
 =?utf-8?B?Z2VvdmNiREZSODhrM3orZldzN1E2K0NINDFiN21IcGtZM0p5TUcycUN6emhJ?=
 =?utf-8?B?OXlyZ3gvQms4anFxZHpTV1FJcXNPRk10YW9oQU1paFBoMUZDeUlwWlZFOHEx?=
 =?utf-8?B?dURJRGUyRFNqN010ODJ2MmdlendKRzVJeWFNRnEyaGZxUXROa2h2MGR6LzEx?=
 =?utf-8?B?ekpBbXUrdHJZb1hWTlhNcCszRjhLanh6S3lGeWh3dVN3V3MvSHRIOFhoTjgz?=
 =?utf-8?B?Yyt4WHhLa2o1ZmdLY2dBQzI3Z0tkS3RGQ21tNzJQeG1EdDlGaVI2WDlFTTZZ?=
 =?utf-8?B?cU5zTy8rQjV1OXJHNzZML2JjOFhCSkhDRXhSUkNOazBaM1pQd3FBY3NhSUlL?=
 =?utf-8?B?N3lTTDBieVNnSmVtOEFESHJzT2VoTUd1MlN5Qzc0L3gzclFGTmNUQUovV2dD?=
 =?utf-8?B?SG1PNStvVDM5S1JnUHZEOHp6dTBKNVVic0lxUlYwejk5MU1nYmdVYTl5SmZk?=
 =?utf-8?B?V0dSWnhGRDZSb2ViV2JZZ2srcHJLQ0l0OEdUMXBidy93NkdKR2h5RC9vSjB0?=
 =?utf-8?B?bkJEaXRLby9WRlZaZWdhb0w2L2ZBKy90cTVpOWYrUVpRZGdUWTRRWHdNeHBF?=
 =?utf-8?B?TmtsbW40ci9VT080VGFISUw5bEFRQTcycXhJUUxDdUZZTjcydjNsclRDNVd5?=
 =?utf-8?B?ZVNqY3dWOHpWZVZzVHI5TzFGMy9QTTh3eWF4a0lJaDdRNjh1Q1k2L2dhdWFT?=
 =?utf-8?B?T3pSY1gxNFJRWDIwdkdodnVDMGl6d2NjWkEwVHk3WFd3YUlrV3RvSEF6M2pq?=
 =?utf-8?B?dXpJTnpBSVlBWlNVUWhBVDhFYktlcEdhSFRUWGFDVjdOYTlUQzZOL3NIMW4w?=
 =?utf-8?B?YjdaN2pSclZvNlE2ZFp5RVdKenZrL2gwMlZOYUVMYjBMYXZTbnN3d3dLUGEz?=
 =?utf-8?B?U1FqdjA0UzZKVWZmSS9sZldLVkMvemZ5akQ3RTdjeG5acDFPQUY2OU5JdXQ3?=
 =?utf-8?B?MThXUnpnZGpRMUJkREFuRDBzNjhGUi8xOGg4dXZqRWhTdzg4L3lBVHdsSXEv?=
 =?utf-8?B?L1Q4aUp1YXhucENISXF3YW9uQldiaDliMHhQeFhWNXpsNkN4UFgxL00zMUd2?=
 =?utf-8?B?QVgvUnVWMzlhSHJCTk1Ndkw5ZGlJS0dZVUFTV2V6aEdBQ1AvanpaelN4M1Y3?=
 =?utf-8?B?ckwwSW45MGV5TllpSUZPbXV2aVdwRU9iakgyeWN1WlQraFVQVDFJZWw3VjAx?=
 =?utf-8?B?OWN1WVJXZGN5U0YxNC9qdmVmQW01TzJmb2pTVWZPWUY4Y3VWYkRVbkUrQm5V?=
 =?utf-8?B?ejhlNWQ3Wml1bmZsUEdNb3ZJSlJTcDgySTlybUZKeU5TREFjNjZweldFU2N0?=
 =?utf-8?Q?bXX+Nq5cO2N6VJOdSba1CYgEIrUeOSYcCGI/JJI?=
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?OHpLU3R4SW8vY3pOYmJZVGtxWVArUm5DUU9SQjNYeXpQaVkxSmNGQ0EwanMw?=
 =?utf-8?B?cXR5ejNyclp1MFpRMldEaFQ3NkRTTHlUTVF6ZzlCZDdMeG9OcmtXTW00Z2N0?=
 =?utf-8?B?dXZVT09ORnBhTmdnNndVUzF3dSsvdU94a05SZi9PSUFCeWpSU1BQbnBOMHly?=
 =?utf-8?B?YjV5T2lVejkzRG1IZFhHSmFsWHBaUWN6U0xNUTZlQXVnRFZvZEhoazk2SHFk?=
 =?utf-8?B?WDR2NVV5c05NNnMrSU0vVHNORFRtQXNNZnNCRlg1bDVOVGI3U1MyM1hCM0Jl?=
 =?utf-8?B?U21IVWx4STFSbFFLNmVwVDlyTjBJYVF2SjdDc3lKaGVTNkpqZU9hYUZlZWtI?=
 =?utf-8?B?ZE1lUTJJQVFMQjFTbDRlUGhVb2hacFE3WENQSmU1M3dXakFkbHpVQmRlUTF0?=
 =?utf-8?B?b055YkxDUlZiSVZzMEc0TXdBZFZybEMrWVdCZCt6bm9YaXFhUHFBbXpvd0li?=
 =?utf-8?B?RDBhYUQ5Q3NBaGRMT2JrTzdzNmdyS09wbmh5b3k3SCszN2NVYis2NHR1K1NV?=
 =?utf-8?B?c2ZBSnkrMkJCa3NWSERmaDMybStROTNkWFJQNEtRak52b3hSbDUyQmg4a2Jo?=
 =?utf-8?B?Q05xdFdEbTZlS2orOTE4T1pSNHBtVUx4UU5rZ0xDalAycUl0WHlUVVBaeFlr?=
 =?utf-8?B?YmdOWnBid0lVNC9IVDFnTFR5V3pxN2dBdCtMVmk1Q1NyaGNmVFFkZytSeEZ1?=
 =?utf-8?B?SlFVTmlrZ2twNFNSelQxaXprcTA4aTYzUzI1aHUwWmtuOUhrYUIySEYxbEha?=
 =?utf-8?B?bnZuMnZUSTZEdUpPWmxwb1ZnL2dVOTN3dkcrM1o2N1UxOERjVE1sem8xakxX?=
 =?utf-8?B?ZkRaMks3U21hUEVkYURXRjhBa1RMQXVIRHAvZzQ5dWdZMU5md09jSndyWW9u?=
 =?utf-8?B?YmFQNkpGRVNQazF2OVpKYk5kS0FQSi9MaHBPRE5lNkw3VmhoTlpBenAyTmpp?=
 =?utf-8?B?d2pzMVNBemVmZDJHUnhxTGk0YjdaZWZHUHpRVGc1ZXdpeCtoQVFXN1gwWkR4?=
 =?utf-8?B?ZlpJRkJoN0c1OFNndit5RXVPUStNWERlRHJHd3RFdXM3WEJndVhxc0hyWW01?=
 =?utf-8?B?VE5nK0IvWmU4d29ONTlRYnJvU3pSaHdRRms2TXNlWUNTWnR2dkZJNENOc1Fy?=
 =?utf-8?B?L0pscmIyT0JiWHVUaStXc25YckhzY3d3bmE4Mkhadmo1Uy9BQk9vVTdoRVBO?=
 =?utf-8?B?aHdTZDFqV1BTSFN1VEJEb1VlWTl6OGpYYjNZdDU0eXZwWFlTNXk5U21nVXRp?=
 =?utf-8?B?ckVoVmRnVmRybWFOaEVKaHpzaURVaHNEV3hZOFgwSXVGbGNBSFBOcmU1VnQ5?=
 =?utf-8?B?dTEvR2s4b0cxUlVqbElTaUNNdGoxUW5UMUtRc2cyZ25YQWlSSm5ZWHRXOWli?=
 =?utf-8?B?YXd0elhVZTVFZnlPNkV1aHBBS2k2Zk4zNGx4eHJpem5jSVFiNHkzYnRqaFlE?=
 =?utf-8?B?L3EwajVPVUhnRUtoS0JiT1YwZ2dqd2RtT1ZkTFJ3ZkhtSmpUVEF4NmFGcTgw?=
 =?utf-8?B?RGhtQUo0R3pJcFRQWSsyVElGbVZRcGNRTFU3d2RqRHdhMW92TFptNWVBMDM5?=
 =?utf-8?B?ZTl5N3huNVZ3TVZIK2lrVGp6dmJYRVZNZmEwY1lnZnJoaysxTzBlOXdtZDdp?=
 =?utf-8?Q?JqQvy+89rhHB2559kaXEsXu5P5nggnQSwZLghcZaG5/k=3D?=
X-OriginatorOrg: sct-15-20-7719-20-msonline-outlook-448bf.templateTenant
X-MS-Exchange-CrossTenant-Network-Message-Id: b372f25d-9c40-4326-e7ed-08dd3bb2e344
X-MS-Exchange-CrossTenant-AuthSource: ME0P300MB0445.AUSP300.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Jan 2025 13:36:06.2039
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg:
	00000000-0000-0000-0000-000000000000
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SY8P300MB0759
List-Id: <pgsql-hackers.lists.postgresql.org>
List-Help: <https://lists.postgresql.org/manage/>
List-Subscribe: <https://lists.postgresql.org/manage/>
List-Post: <mailto:pgsql-hackers@lists.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@lists.postgresql.org>
List-Archive: <https://www.postgresql.org/list/pgsql-hackers>
Archived-At: <https://www.postgresql.org/message-id/ME0P300MB0445F3242216930DE6D44A24B6E02%40ME0P300MB0445.AUSP300.PROD.OUTLOOK.COM>
Precedence: bulk

On Mon, 20 Jan 2025 at 18:41, Bernd Helmle <mailings@oopsware.de> wrote:
> Hi,=20
>
> Please find attached a reworked patch according Alvaro's comments.
>
> Am Montag, dem 13.01.2025 um 17:06 +0100 schrieb Alvaro Herrera:
>> Hello
>>=20
>> 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:
>>=20
>> =C2=A0=C2=A0=C2=A0 /*
>> -=C2=A0=C2=A0=C2=A0 * 1. Start digest A
>> -=C2=A0=C2=A0=C2=A0 * 2. Add the password string to digest A
>> -=C2=A0=C2=A0=C2=A0 * 3. Add the salt to digest A
>> +=C2=A0=C2=A0=C2=A0 * 1. Start digest A 2. Add the password string to di=
gest A 3.
>> Add the
>> +=C2=A0=C2=A0=C2=A0 * salt to digest A
>> =C2=A0=C2=A0=C2=A0=C2=A0 */
>>=20
>> I suggest you can fix this by adding a "-" sign to the opening "/*"
>> line
>> so that pgindent doesn't mangle it (so it becomes /*- ).=C2=A0 This
>> appears
>> in several places.
>>=20
>
> This is even documented:
>
> https://www.postgresql.org/docs/devel/source-format.html
>
> I ran pgindent locally and verified it works like you suggested.
>>=20
>
> [...]
>
>> Your test data (crypt-shacrypt.sql) looks a bit short.=C2=A0 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).=C2=A0 I'd also add=
 a
>> note that Drepper's implementation is public domain in crypt-sha.c's
>> license block.
>>=20
>
> 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.=C2=A0 Why not just
>> use the
>> literal $ sign where needed (or a DOLLAR_CHR if we feel like avoiding
>> a
>> magic value there)?=C2=A0 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.
>
>>=20
>> 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 siz=
e,
+                                                char *output, int output_s=
ize)
+{
+       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] !=3D '$')
+               && (dec_salt_binary[2] !=3D '$'))
+       {
+               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])=
) =3D=3D 0)
...
+       else if (strncmp(dec_salt_binary, magic_bytes[1], strlen(magic_byte=
s[1])) =3D=3D 0)

IMO, we could change the above code as:

+       if (strncmp(dec_salt_binary, magic_bytes[0], strlen(magic_bytes[0])=
) =3D=3D 0)
...
+       else if (strncmp(dec_salt_binary, magic_bytes[1], strlen(magic_byte=
s[1])) =3D=3D 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=3D# select crypt('hello', '$5$$6$rounds=3D10000$/Zg436s2vmTwsoSz')=
;
                      crypt
-------------------------------------------------
 $5$$TCRu/ts4Npu8OJyeWy2WnUHCe/6bKVMSi0sROUrPh48
(1 row)

postgres=3D# select crypt('hello', '$5$rounds=3D10000$/Zg436s2vmTwsoSz');
                                    crypt
---------------------------------------------------------------------------=
---
 $5$rounds=3D10000$/Zg436s2vmTwsoSz$N4Ad0oGzE6ak30z4EGSEXOc2OXQOpmauicPT356=
0lY2
(1 row)

According to the documentation, I think the first should output as followin=
g:

$5$rounds=3D5000$TCRu/ts4Npu8OJyeWy2WnUHCe/6bKVMSi0sROUrPh48

--=20
Regrads,
Japin Li