From: Bernd Helmle <mailings@oopsware.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Andres Freund <andres@anarazel.de>
Cc: Japin Li <japinli@hotmail.com>
Cc: PostgreSQL Development <pgsql-hackers@postgresql.org>
Subject: Re: Modern SHA2- based password hashes for pgcrypto
Date: Mon, 07 Apr 2025 13:48:49 +0200
Message-ID: <6fab35422df6b6b9727fdcc243c5fa1c667dd3b5.camel@oopsware.de> (raw)
In-Reply-To: <2977905.1743968616@sss.pgh.pa.us>
References: <202504051722.3jv3ashzaibd@alvherre.pgsql>
<2977905.1743968616@sss.pgh.pa.us>
Am Sonntag, dem 06.04.2025 um 15:43 -0400 schrieb Tom Lane:
> I'd be more comfortable with a check like
>
> if (strchr("...valid chars...", *ep) != NULL)
>
> It looks like "_crypt_itoa64" might be directly usable as the
> valid-chars string, too. (BTW, why is _crypt_itoa64 not
> marked const?)
Here is a patch that tries to address all these issues (including
Andres' report). I've adjusted the error message and use ereport(), so
it might be more useful if we deal with not just single byte letters.
I've also changed _crypt_itoa64 from unsigned char to char, since this
seems what strchr() expects (at least on my machine) and we don't deal
specifically elsewhere with that.
Thanks,
Bernd
Attachments:
[text/x-patch] 0001-Follow-up-fixes-for-commit-749a9e20c9790006f3af47f7a.patch (1.8K, ../6fab35422df6b6b9727fdcc243c5fa1c667dd3b5.camel@oopsware.de/2-0001-Follow-up-fixes-for-commit-749a9e20c9790006f3af47f7a.patch)
download | inline diff:
From d9aa1524a8280bb3997998c2a3499b1f8cfdee6c Mon Sep 17 00:00:00 2001
From: Bernd Helmle <Bernd Helmle mailings@oopsware.de>
Date: Mon, 7 Apr 2025 13:45:12 +0200
Subject: [PATCH] Follow up fixes for commit
749a9e20c9790006f3af47f7a8faf4ad8dc358d9.
This removes the platform dependant use of isalpha() and friends and replaces
the check for supported letters in the salt string to just check against
plain ASCII letters. While this makes the use of salt letters more strict,
we expect this to make the code more robust.
This fixes a careless use-pointer-after-pfree spotted by Andres Freund, too.
---
contrib/pgcrypto/crypt-sha.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/contrib/pgcrypto/crypt-sha.c b/contrib/pgcrypto/crypt-sha.cindex 7e9440ca784..bcf5b480732 100644--- a/contrib/pgcrypto/crypt-sha.c+++ b/contrib/pgcrypto/crypt-sha.c@@ -58,7 +58,7 @@ typedef enum
PGCRYPTO_SHA_UNKOWN
} PGCRYPTO_SHA_t;
-static unsigned char _crypt_itoa64[64 + 1] =+static const char _crypt_itoa64[64 + 1] =
"./0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
/*
@@ -321,10 +321,12 @@ px_crypt_shacrypt(const char *pw, const char *salt, char *passwd, unsigned dstle
if (*ep != '$')
{
- if (isalpha(*ep) || isdigit(*ep) || (*ep == '.') || (*ep == '/'))+ if (strchr(_crypt_itoa64, *ep) != NULL)
appendStringInfoCharMacro(decoded_salt, *ep);
else
- elog(ERROR, "invalid character in salt string: \"%c\"", *ep);+ ereport(ERROR,+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),+ errmsg("invalid character in salt string at: \"%s\"", ep));
}
else
{
@@ -602,8 +604,6 @@ px_crypt_shacrypt(const char *pw, const char *salt, char *passwd, unsigned dstle
elog(ERROR, "unsupported digest length");
}
- *cp = '\0';-
/*
* Copy over result to specified buffer.
*
--
2.49.0
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: mailings@oopsware.de, tgl@sss.pgh.pa.us, alvherre@alvh.no-ip.org, andres@anarazel.de, japinli@hotmail.com
Subject: Re: Modern SHA2- based password hashes for pgcrypto
In-Reply-To: <6fab35422df6b6b9727fdcc243c5fa1c667dd3b5.camel@oopsware.de>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox