Thread: 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
Attachment
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 Shinderuk https://postgrespro.com/
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
Attachment
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 Shinderuk https://postgrespro.com/
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
Attachment
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
Attachment
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 Shinderuk https://postgrespro.com/
Attachment
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
Attachment
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 Shinderuk https://postgrespro.com/