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

On 4/22/19 9:42 AM, Tom Lane 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 ...

OK, so I have something that sort of works, i.e:

if (strncmp(shadow_pass, "md5", 3) == 0 &&
    strlen(shadow_pass) == MD5_PASSWD_LEN &&
    strspn(shadow_pass, MD5_PASSWD_CHARSET) == MD5_PASSWD_LEN
)

where MD5_PASSWD_CHARSET = "mabcdef0123456789"

...but you may notice something: the CHARSET contains an "m" as we store
that "md5" prefix on the md5 hashed passwords.

So this leads to a few options:

1. Make a copy of the shadow password w/o the "md5" prefixed to it then
then perform the strspn sans "m" in the char set.

2. Count how many times "m" appears in the shadow password. If count >
1, then it's clearly an invalid md5 hash.

3. Leave the proposed check as is, knowing that someone could have a
hash like "md51234567890123456789012345678901m" that is borked.

4. Do nothing.

If there is a concise way to do #2 I like that approach; I'm not sure if
we have any helpers to perform counts like that or if I have to write my
own.

My preference is to go down path #2, otherwise I'd vote for #4 given the
(hopefully) planned deprecation.

Thanks,

Jonathan