Re: Possible to store invalid SCRAM-SHA-256 Passwords

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Michael Paquier <michael@paquier.xyz> writes:
> > On Sat, Apr 20, 2019 at 04:12:56PM -0400, Jonathan S. Katz wrote:
> >> I modified the "get_password_type" function to perform a SCRAM
> >> verification to see if it is a properly hashed SCRAM password. If it is,
> >> we treat the password as a SCRAM hashed one. Otherwise, we proceed to
> >> the next step, which is to treat it as a plainly stored one.
>
> > Any objections to back-patch that stuff to v10?
>
> Patch looks OK as far as it goes, but ISTM it raises an obvious
> question: shouldn't the immediately-preceding test to see if a
> password is MD5 also be trying harder?  Currently it only checks
> the length, but verifying that the rest is valid hex data would
> go far towards preventing the same set of problems for MD5.
>
> You might argue that MD5 is deprecated and we shouldn't expend
> any effort on it, but a simple strspn check would only require
> about one more line ...

I agree we should also handle md5 better.  I realize this needs to be
back-patched and so we have to deal with the existing catalog structure,
but this really screams out, in my mind anyway, that we shouldn't have
ever tried to just stash the password-encoding-type into the password
field and that we should have pulled it out into its own column, so that
we aren't having to guess about things as important as a password.

I recall having exactly that debate when SCRAM was being worked on and
the push-back basically being that it was more work and we'd have to
have additional syntax for ALTER USER, et al.  I wish I had had more
time to spend on that discussion.  Water under the bridge now, but
hopefully we learn from this and maybe someone refactors how this works
sometime soon (or, at least, whenever we add the next password
encoding).

Thanks!

Stephen