Re: Improve error handling of HMAC computations and SCRAM
On 13.01.2022 10:24, Michael Paquier wrote: Thanks for the review. The comments about pg_hmac_ctx->data were wrong from the beginning, coming, I guess, from one of the earlier patch versions where this was discussed. So I have applied that independently. I have also spent a good amount of time on that to close the loop and make sure that no code paths are missing an error context, adjusted a couple of comments to explain more the role of *errstr in all the SCRAM routines, and finally applied it on HEAD. Thanks! -- Sergey Shinderukhttps://postgrespro.com/
Re: Improve error handling of HMAC computations and SCRAM
On Thu, Jan 13, 2022 at 02:01:24AM +0300, Sergey Shinderuk wrote: > Gave it a thorough read. Looks good, except for errstr not set in a couple > of places (see the diff attached). Thanks for the review. The comments about pg_hmac_ctx->data were wrong from the beginning, coming, I guess, from one of the earlier patch versions where this was discussed. So I have applied that independently. I have also spent a good amount of time on that to close the loop and make sure that no code paths are missing an error context, adjusted a couple of comments to explain more the role of *errstr in all the SCRAM routines, and finally applied it on HEAD. -- Michael signature.asc Description: PGP signature
Re: Improve error handling of HMAC computations and SCRAM
On 12.01.2022 14:32, Michael Paquier wrote:
On Wed, Jan 12, 2022 at 12:56:17PM +0900, Michael Paquier wrote:
Attached is a rebased patch for the HMAC portions, with a couple of
fixes I noticed while going through this stuff again (mostly around
SASLprep and pg_fe_scram_build_secret), and a fix for a conflict
coming from 9cb5518. psql's \password is wrong to assume that the
only error that can happen for scran-sha-256 is an OOM, but we'll get
there.
With an attachment, that's even better. (Thanks, Daniel.)
Gave it a thorough read. Looks good, except for errstr not set in a
couple of places (see the diff attached).
Didn't test it.
--
Sergey Shinderukhttps://postgrespro.com/diff --git a/src/common/hmac.c b/src/common/hmac.c
index 592f9b20a38..a27778e86b3 100644
--- a/src/common/hmac.c
+++ b/src/common/hmac.c
@@ -46,9 +46,7 @@ typedef enum pg_hmac_errno
PG_HMAC_ERROR_INTERNAL
} pg_hmac_errno;
-/*
- * Internal structure for pg_hmac_ctx->data with this implementation.
- */
+/* Internal pg_hmac_ctx structure */
struct pg_hmac_ctx
{
pg_cryptohash_ctx *hash;
diff --git a/src/common/hmac_openssl.c b/src/common/hmac_openssl.c
index c352f9db9e9..44f36d51dcb 100644
--- a/src/common/hmac_openssl.c
+++ b/src/common/hmac_openssl.c
@@ -60,9 +60,7 @@ typedef enum pg_hmac_errno
PG_HMAC_ERROR_OPENSSL
} pg_hmac_errno;
-/*
- * Internal structure for pg_hmac_ctx->data with this implementation.
- */
+/* Internal pg_hmac_ctx structure */
struct pg_hmac_ctx
{
HMAC_CTX *hmacctx;
diff --git a/src/common/scram-common.c b/src/common/scram-common.c
index 5f90397c66d..8896b1e73e4 100644
--- a/src/common/scram-common.c
+++ b/src/common/scram-common.c
@@ -44,7 +44,10 @@ scram_SaltedPassword(const char *password,
pg_hmac_ctx *hmac_ctx = pg_hmac_create(PG_SHA256);
if (hmac_ctx == NULL)
+ {
+ *errstr = pg_hmac_error(NULL); /* returns OOM */
return -1;
+ }
/*
* Iterate hash calculation of HMAC entry using given salt. This is
@@ -126,7 +129,10 @@ scram_ClientKey(const uint8 *salted_password, uint8
*result,
pg_hmac_ctx *ctx = pg_hmac_create(PG_SHA256);
if (ctx == NULL)
+ {
+ *errstr = pg_hmac_error(NULL); /* returns OOM */
return -1;
+ }
if (pg_hmac_init(ctx, salted_password, SCRAM_KEY_LEN) < 0 ||
pg_hmac_update(ctx, (uint8 *) "Client Key", strlen("Client
Key")) < 0 ||
Re: Improve error handling of HMAC computations and SCRAM
On Wed, Jan 12, 2022 at 12:56:17PM +0900, Michael Paquier wrote:
> Attached is a rebased patch for the HMAC portions, with a couple of
> fixes I noticed while going through this stuff again (mostly around
> SASLprep and pg_fe_scram_build_secret), and a fix for a conflict
> coming from 9cb5518. psql's \password is wrong to assume that the
> only error that can happen for scran-sha-256 is an OOM, but we'll get
> there.
With an attachment, that's even better. (Thanks, Daniel.)
--
Michael
From a6bcfefa9a8dd98bdc6f0e105f7b55dc8739c49e Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Wed, 12 Jan 2022 12:46:27 +0900
Subject: [PATCH v2] Improve HMAC error handling
---
src/include/common/hmac.h| 1 +
src/include/common/scram-common.h| 14 +++--
src/backend/libpq/auth-scram.c | 22 ---
src/common/hmac.c| 64
src/common/hmac_openssl.c| 90
src/common/scram-common.c| 47 +++
src/interfaces/libpq/fe-auth-scram.c | 67 +++--
src/interfaces/libpq/fe-auth.c | 10 ++--
src/interfaces/libpq/fe-auth.h | 3 +-
9 files changed, 269 insertions(+), 49 deletions(-)
diff --git a/src/include/common/hmac.h b/src/include/common/hmac.h
index cf7aa17be4..c18783fe11 100644
--- a/src/include/common/hmac.h
+++ b/src/include/common/hmac.h
@@ -25,5 +25,6 @@ extern int pg_hmac_init(pg_hmac_ctx *ctx, const uint8 *key, size_t len);
extern int pg_hmac_update(pg_hmac_ctx *ctx, const uint8 *data, size_t len);
extern int pg_hmac_final(pg_hmac_ctx *ctx, uint8 *dest, size_t len);
extern void pg_hmac_free(pg_hmac_ctx *ctx);
+extern const char *pg_hmac_error(pg_hmac_ctx *ctx);
#endif /* PG_HMAC_H */
diff --git a/src/include/common/scram-common.h b/src/include/common/scram-common.h
index d53b4fa7f5..d1f840c11c 100644
--- a/src/include/common/scram-common.h
+++ b/src/include/common/scram-common.h
@@ -47,12 +47,16 @@
#define SCRAM_DEFAULT_ITERATIONS 4096
extern int scram_SaltedPassword(const char *password, const char *salt,
- int saltlen, int iterations, uint8 *result);
-extern int scram_H(const uint8 *str, int len, uint8 *result);
-extern int scram_ClientKey(const uint8 *salted_password, uint8 *result);
-extern int scram_ServerKey(const uint8 *salted_password, uint8 *result);
+ int saltlen, int iterations, uint8 *result,
+ const char **errstr);
+extern int scram_H(const uint8 *str, int len, uint8 *result,
+ const char **errstr);
+extern int scram_ClientKey(const uint8 *salted_password, uint8 *result,
+ const char **errstr);
+extern int scram_ServerKey(const uint8 *salted_password, uint8 *result,
+ const char **errstr);
extern char *scram_build_secret(const char *salt, int saltlen, int iterations,
-const char *password);
+const char *password, const char **errstr);
#endif /* SCRAM_COMMON_H */
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 7c9dee70ce..ee7f52218a 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -465,6 +465,7 @@ pg_be_scram_build_secret(const char *password)
pg_saslprep_rc rc;
char saltbuf[SCRAM_DEFAULT_SALT_LEN];
char *result;
+ const char *errstr = NULL;
/*
* Normalize the password with SASLprep. If that doesn't work, because
@@ -482,7 +483,8 @@ pg_be_scram_build_secret(const char *password)
errmsg("could not generate random salt")));
result = scram_build_secret(saltbuf, SCRAM_DEFAULT_SALT_LEN,
-SCRAM_DEFAULT_ITERATIONS, password);
+SCRAM_DEFAULT_ITERATIONS, password,
+&errstr);
if (prep_password)
pfree(prep_password);
@@ -509,6 +511,7 @@ scram_verify_plain_password(const char *username, const char *password,
uint8 computed_key[SCRAM_KEY_LEN];
char *prep_password;
pg_saslprep_rc rc;
+ const char *errstr = NULL;
if (!parse_scram_secret(secret, &iterations, &encoded_salt,
stored_key, server_key))
@@ -539,10 +542,10 @@ scram_verify_plain_password(const char *username, const char *password,
/* Compute Server Key based on the user-supplied plaintext password */
if (scram_SaltedPassword(password, salt, saltlen, iterations,
- salted_password) < 0 ||
- scram_ServerKey(salted_password, computed_key) < 0)
+ salted_password, &errstr) < 0 ||
+ scram_ServerKey(salted_password, computed_key, &errstr) < 0)
{
- elog(ERROR, "could not compute server key");
+ elog(ERROR, "could not compute server key: %s", errstr);
}
if (prep_password)
@@ -1113,6 +1116,7 @@ verify_client_proof(scram_state *state)
uint8 client_StoredKey[SCRAM_KEY_LEN];
pg_hmac_ctx *ctx = pg_hmac_create(PG_SHA256);
int i;
+ const char *errstr = NULL;
/*
* Calculate ClientSignature. Note that we don't log directly a failure
@@ -1133,7 +1137,8 @@ verify_client_proof(scram_state *state)
strlen(state->client_final_message_without_proof)
Re: Improve error handling of HMAC computations and SCRAM
On Tue, Jan 11, 2022 at 11:08:59AM +0300, Sergey Shinderuk wrote: > Yeah, that's better. I thought "providing errors about an error" was a > typo, but now I see the same comment was committed in b69aba745. Is it just > me? :) It is not only you :) I have applied a fix to fix the comments on HEAD and REL_14_STABLE. Attached is a rebased patch for the HMAC portions, with a couple of fixes I noticed while going through this stuff again (mostly around SASLprep and pg_fe_scram_build_secret), and a fix for a conflict coming from 9cb5518. psql's \password is wrong to assume that the only error that can happen for scran-sha-256 is an OOM, but we'll get there. -- Michael signature.asc Description: PGP signature
Re: Improve error handling of HMAC computations and SCRAM
On 11.01.2022 10:57, Michael Paquier wrote: On Tue, Jan 11, 2022 at 10:50:50AM +0300, Sergey Shinderuk wrote: + * Returns a static string providing errors about an error that happened "errors about an error" looks odd. Sure, that could be reworded. What about "providing details about an error"? Yeah, that's better. I thought "providing errors about an error" was a typo, but now I see the same comment was committed in b69aba745. Is it just me? :) Thanks, -- Sergey Shinderukhttps://postgrespro.com/
Re: Improve error handling of HMAC computations and SCRAM
On Tue, Jan 11, 2022 at 10:50:50AM +0300, Sergey Shinderuk wrote: > A few comments after a quick glance... Thanks! > + * Returns a static string providing errors about an error that happened > > "errors about an error" looks odd. Sure, that could be reworded. What about "providing details about an error"? > We already have SSLerrmessage elsewhere and it's documented to never return > NULL. I find that confusing. This name is chosen on purpose. There could be some refactoring done with those things. > If I have two distinct pg_hmac_ctx's, are their errreason's idependent from > one another or do they really point to the same static buffer? Each errreason could be different, as each computation could fail for a different reason. If they fail for the same reason, they would point to the same error context strings. -- Michael signature.asc Description: PGP signature
Re: Improve error handling of HMAC computations and SCRAM
Hi,
On 11.01.2022 07:56, Michael Paquier wrote:
> Thoughts?
A few comments after a quick glance...
+ * Returns a static string providing errors about an error that happened
"errors about an error" looks odd.
+static const char *
+SSLerrmessage(unsigned long ecode)
+{
+ if (ecode == 0)
+ return NULL;
+
+ /*
+* This may return NULL, but we would fall back to a default error path
if
+* that were the case.
+*/
+ return ERR_reason_error_string(ecode);
+}
We already have SSLerrmessage elsewhere and it's documented to never
return NULL. I find that confusing.
If I have two distinct pg_hmac_ctx's, are their errreason's idependent
from one another or do they really point to the same static buffer?
Regards,
--
Sergey Shinderukhttps://postgrespro.com/
Improve error handling of HMAC computations and SCRAM
Hi all,
This is a follow-up of the work done in b69aba7 for cryptohashes, but
this time for HMAC. The main issue here is related to SCRAM, where we
have a lot of code paths that have no idea about what kind of failure
is happening when an error happens, and this exists since v10 where
SCRAM has been introduced, for some of them, frontend and backend
included. \password is one example.
The set of errors improved here would only trigger in scenarios that
are unlikely going to happen, like an OOM or an internal OpenSSL
error. It would be possible to create a HMAC from a MD5, which would
cause an error when compiling with OpenSSL and FIPS enabled, but the
only callers of the pg_hmac_* routines involve SHA-256 in core through
SCRAM, so I don't see much a point in backpatching any of the things
proposed here.
The attached patch creates a new routine call pg_hmac_error() that one
can use to grab details about the error that happened, in the same
fashion as what has been done for cryptohashes. The logic is not that
complicated, but note that the fallback HMAC implementation relies
itself on cryptohashes, so there are cases where we need to look at
the error from pg_cryptohash_error() and store it in the HMAC private
context.
Thoughts?
--
Michael
From 687dd48a8150fae4597b126d68f6758b52ff67cb Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Tue, 11 Jan 2022 13:47:06 +0900
Subject: [PATCH] Improve HMAC error handling
---
src/include/common/hmac.h| 1 +
src/include/common/scram-common.h| 14 +++--
src/backend/libpq/auth-scram.c | 22 ---
src/common/hmac.c| 64
src/common/hmac_openssl.c| 90
src/common/scram-common.c| 47 +++
src/interfaces/libpq/fe-auth-scram.c | 63 +--
src/interfaces/libpq/fe-auth.c | 17 --
src/interfaces/libpq/fe-auth.h | 3 +-
9 files changed, 271 insertions(+), 50 deletions(-)
diff --git a/src/include/common/hmac.h b/src/include/common/hmac.h
index cf7aa17be4..c18783fe11 100644
--- a/src/include/common/hmac.h
+++ b/src/include/common/hmac.h
@@ -25,5 +25,6 @@ extern int pg_hmac_init(pg_hmac_ctx *ctx, const uint8 *key, size_t len);
extern int pg_hmac_update(pg_hmac_ctx *ctx, const uint8 *data, size_t len);
extern int pg_hmac_final(pg_hmac_ctx *ctx, uint8 *dest, size_t len);
extern void pg_hmac_free(pg_hmac_ctx *ctx);
+extern const char *pg_hmac_error(pg_hmac_ctx *ctx);
#endif /* PG_HMAC_H */
diff --git a/src/include/common/scram-common.h b/src/include/common/scram-common.h
index d53b4fa7f5..d1f840c11c 100644
--- a/src/include/common/scram-common.h
+++ b/src/include/common/scram-common.h
@@ -47,12 +47,16 @@
#define SCRAM_DEFAULT_ITERATIONS 4096
extern int scram_SaltedPassword(const char *password, const char *salt,
- int saltlen, int iterations, uint8 *result);
-extern int scram_H(const uint8 *str, int len, uint8 *result);
-extern int scram_ClientKey(const uint8 *salted_password, uint8 *result);
-extern int scram_ServerKey(const uint8 *salted_password, uint8 *result);
+ int saltlen, int iterations, uint8 *result,
+ const char **errstr);
+extern int scram_H(const uint8 *str, int len, uint8 *result,
+ const char **errstr);
+extern int scram_ClientKey(const uint8 *salted_password, uint8 *result,
+ const char **errstr);
+extern int scram_ServerKey(const uint8 *salted_password, uint8 *result,
+ const char **errstr);
extern char *scram_build_secret(const char *salt, int saltlen, int iterations,
-const char *password);
+const char *password, const char **errstr);
#endif /* SCRAM_COMMON_H */
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 7c9dee70ce..ee7f52218a 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -465,6 +465,7 @@ pg_be_scram_build_secret(const char *password)
pg_saslprep_rc rc;
char saltbuf[SCRAM_DEFAULT_SALT_LEN];
char *result;
+ const char *errstr = NULL;
/*
* Normalize the password with SASLprep. If that doesn't work, because
@@ -482,7 +483,8 @@ pg_be_scram_build_secret(const char *password)
errmsg("could not generate random salt")));
result = scram_build_secret(saltbuf, SCRAM_DEFAULT_SALT_LEN,
-SCRAM_DEFAULT_ITERATIONS, password);
+SCRAM_DEFAULT_ITERATIONS, password,
+&errstr);
if (prep_password)
pfree(prep_password);
@@ -509,6 +511,7 @@ scram_verify_plain_password(const char *username, const char *password,
uint8 computed_key[SCRAM_KEY_LEN];
char *prep_password;
pg_saslprep_rc rc;
+ const char *errstr = NULL;
if (!parse_scram_secret(secret, &iterations, &encoded_salt,
stored_key, server_key))
@@ -539,10 +542,10 @@ scram_verify_plain_password(const char *username, const char *password,
/* Compute Server Key based on the user-supplied plaintext password */
if (scram_SaltedPas
