On Sat, 2021-02-27 at 17:02 -0500, Jeff Janes wrote:
> Note that in one case you do get the "does not match" line. That is
> if the user has a scram password assigned and the hba specifies
> plain-text 'password' as the method. So if the absence of the DETAIL
> is intentional, it is not internally consistent.
Agreed.
> The attached patch fixes the issue. I don't know if this is the
> correct location to be installing the message,
> maybe verify_client_proof should be doing it instead.
Hmm, I agree that the current location doesn't seem quite right. If
someone adds a new way to exit the SCRAM loop on failure, they'd
probably need to partially unwind this change to avoid printing the
wrong detail message. But in my opinion, verify_client_proof()
shouldn't be concerned with logging details...
What would you think about adding the additional detail right after
verify_client_proof() fails? I.e.
> if (!verify_client_proof(state) || state->doomed)
> {
> /* <add logdetail here, if not doomed or already set> */
> result = SASL_EXCHANGE_FAILURE;
> break;
> }
That way the mismatched-password detail is linked directly to the
mismatched-password logic.
Other notes:
- Did you have any thoughts around adding a regression test, since this
would be an easy thing to break in the future?
- I was wondering about timing attacks against the psprintf() call to
construct the logdetail string, but it looks like the other authn code
paths aren't worried about that.
--Jacob
