Possible to store invalid SCRAM-SHA-256 Passwords

On 4/20/19 3:26 PM, Jonathan S. Katz wrote:

Hi,

With some guidance from Stephen, I've discovered some scenarios where
one can store invalid SCRAM-SHA-256 passwords.

Scenario #1: Directly from CREATE/ALTER ROLE

for example on PostgreSQL 11:

CREATE ROLE test1 PASSWORD 'SCRAM-SHA-256$1234' LOGIN;

In the logs, one sees:

2019-04-20 18:36:07.883 UTC [22251] postgres@postgres LOG: invalid
SCRAM verifier for user "test1"
2019-04-20 18:36:07.883 UTC [22251] postgres@postgres STATEMENT:
CREATE USER test1 PASSWORD 'SCRAM-SHA-256$1234' LOGIN;

pg_authid contains:

-[ RECORD 1 ]--+-------------------
rolname | test1
rolcanlogin | t
rolpassword | SCRAM-SHA-256$1234

and when I try to login with the password "SCRAM-SHA-256$1234" e.g.

psql -U test1 postgres

psql: FATAL: password authentication failed for user "test1"
FATAL: password authentication failed for user "test1"

Scenario #2: On an upgrade from PG < 10 => PG >= 10

On a PostgreSQL 9.6.12, I created a user as the following:

CREATE ROLE test2 WITH UNENCRYPTED PASSWORD 'SCRAM-SHA-256$1234' LOGIN;

with pg_authid contents:

-[ RECORD 1 ]--+-------------------
rolname | test2
rolcanlogin | t
rolpassword | SCRAM-SHA-256$1234

And was able to **successfully login.**

I installed PostgreSQL 11 and upgrading from 9.6.12 => 11.2

When I attempt to login as test2, I get the following error:

psql: FATAL: password authentication failed for user "tester"
FATAL: password authentication failed for user "tester"

While my hunch is that Scenario #2 is less likely to happen in the wild,
Scenario #1 is a real possibility. Examples, a non-libpq passed driver
wants to send a hashed password directly to a server and has a mistake
in the algorithm, or a program calls "ALTER ROLE" and modifies a
password with an invalid SCRAM-SHA-256 hash in it, etc.

Attached is a patch that I believe resolves this.

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.

Thoughts?

Jonathan

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.

Since v10, we don't allow the storage of plain verifiers so if a
string does not match what we think is a correct SCRAM or MD5
verifier, then it should be processed according to
password_encryption when storing the verifier or processed according
to the auth protocol with the HBA entry matching. Your patch looks
fine to me, I would have just added a test case in password.sql (no
need to send a new patch I can take care of it).

Any objections to back-patch that stuff to v10?
--
Michael

On 4/21/19 9:50 PM, Michael Paquier wrote:

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.

Since v10, we don't allow the storage of plain verifiers so if a
string does not match what we think is a correct SCRAM or MD5
verifier, then it should be processed according to
password_encryption when storing the verifier or processed according
to the auth protocol with the HBA entry matching. Your patch looks
fine to me, I would have just added a test case in password.sql (no
need to send a new patch I can take care of it).

Thanks for verifying. I'm happy to add the test case -- I first wanted
to ensure I was on the right track.

Any objections to back-patch that stuff to v10?

+1; I did not try it out, but am very confident that scenario #2 would
demonstrate the bug exists in 10 as well.

Thanks,

Jonathan

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 ...

regards, tom lane

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

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 ...

Yeah, but at least if people look back at it as a reference for how to
properly(?) validate various password hashes it would look better. If it
was any higher effort, I'd say "no" given the status of md5, but this is
fairly innocuous.

I'm happy to put something together in a separate patch. I can take a
look later today.

Jonathan

On 4/22/19 9:04 AM, Jonathan S. Katz wrote:

On 4/21/19 9:50 PM, Michael Paquier wrote:

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.

Since v10, we don't allow the storage of plain verifiers so if a
string does not match what we think is a correct SCRAM or MD5
verifier, then it should be processed according to
password_encryption when storing the verifier or processed according
to the auth protocol with the HBA entry matching. Your patch looks
fine to me, I would have just added a test case in password.sql (no
need to send a new patch I can take care of it).

Thanks for verifying. I'm happy to add the test case -- I first wanted
to ensure I was on the right track.

Attached is modified patch with tests included. For the patch against
HEAD, I only test to demonstrate that storing an invalid SCRAM-SHA-256
lookalike hash is stored as an actual SCRAM-SHA-256 password, given we
do not allow storing passwords UNENCRYPTED anymore (yay!). I followed
the testing format that was used in the other cases in the file.

What I would love to add as a test would be a function that checks
whether or not a user/password combo of something like
"jkatz/SCRAM-SHA-256$1234" where the password is stored as plaintext
would validate, but I don't believe we expose anything that does that in
the SQL API. So I feel this is just a light sanity check.

Thanks,

Jonathan

Stephen Frost wrote:

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.

Thanks!

Stephen

I don't think there's anything wrong with prefixing a
password hash with an identifier for the password
hashing scheme (and any parameters for that scheme).
This is done all the time in many systems. It just has
to be unambiguoous.

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

"Jonathan S. Katz" <jkatz@postgresql.org> writes:

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.

Yeah, that's silly; why not

strspn(shadow_pass + 3, MD5_PASSWD_CHARSET) == MD5_PASSWD_LEN - 3

It's not like this code isn't very well aware of the first 3 characters
being not like the others.

regards, tom lane

On 4/22/19 6:42 PM, Tom Lane wrote:

"Jonathan S. Katz" <jkatz@postgresql.org> writes:

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.

Yeah, that's silly; why not

strspn(shadow_pass + 3, MD5_PASSWD_CHARSET) == MD5_PASSWD_LEN - 3

It's not like this code isn't very well aware of the first 3 characters
being not like the others.

I like that :) Please see attached patch, which is diff'd from the one
upthread.

I tested using the following:

/* Log in with "abc" */
CREATE ROLE test1 PASSWORD 'md5cdde562ece166a02f5392b656dcf2502' LOGIN;
/* Logs in with "md5cdde562ece166a02f5392b656dcf250g" */
CREATE ROLE test2 PASSWORD 'md5cdde562ece166a02f5392b656dcf250g' LOGIN;
/* Logs in with "md5cdde562ece166a02f5392b656dcf250m" */
CREATE ROLE test3 PASSWORD 'md5cdde562ece166a02f5392b656dcf250m' LOGIN;

I debated adding a test...without being able to simulate a log in, I
don't know if it tests much other than "yes, you can store an invalid
md5 hash and it treats it as plaintext."

Thanks,

Jonathan

"Jonathan S. Katz" <jkatz@postgresql.org> writes:

I debated adding a test...without being able to simulate a log in, I
don't know if it tests much other than "yes, you can store an invalid
md5 hash and it treats it as plaintext."

You could probably build something using TAP testing, cf
src/test/authentication/t/001_password.pl

But I really doubt it's worth the trouble ...

regards, tom lane

On Mon, Apr 22, 2019 at 09:52:15AM -0400, Stephen Frost wrote:

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).

I am not sure that this would have been more work for ALTER TABLE as
we could have relied on just password_encryption to do the work as we
do now. The reluctance was to have more additional columns in
pg_authid as far as I recall, and I sided with having a separate
catalog, and more independent verifier type checks in the catalogs, as
you may recall, which would have also eased password rollups for a
given role.
--
Michael

On Mon, Apr 22, 2019 at 07:36:45PM -0400, Jonathan S. Katz wrote:

On 4/22/19 6:42 PM, Tom Lane wrote:

Yeah, that's silly; why not

strspn(shadow_pass + 3, MD5_PASSWD_CHARSET) == MD5_PASSWD_LEN - 3

It's not like this code isn't very well aware of the first 3 characters
being not like the others.

I like that :) Please see attached patch, which is diff'd from the one
upthread.

That's exactly what I would have done for that. However...

+   if (strncmp(shadow_pass, "md5", 3) == 0 && strlen(shadow_pass) == MD5_PASSWD_LEN &&
+       strspn(shadow_pass + 3, MD5_PASSWD_CHARSET) == MD5_PASSWD_LEN - 3)
                return PASSWORD_TYPE_MD5;
There is no point for the second strlen() check, as strspn does the
same work.

Also, the extra SELECT query with regexp_replace() is a bit overkill
for the purpose, and copying again a copy of the regexp around is no
fun.

In short, I would simplify things as the attached. What do you think?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

There is no point for the second strlen() check, as strspn does the
same work.

Um, no --- the strspn call will count the number of bytes of hex
data, but without also checking strlen, you don't know that there's
not non-hex trailing junk.

regards, tom lane

On 4/22/19 9:10 PM, Tom Lane wrote:

Michael Paquier <michael@paquier.xyz> writes:

There is no point for the second strlen() check, as strspn does the
same work.

Um, no --- the strspn call will count the number of bytes of hex
data, but without also checking strlen, you don't know that there's
not non-hex trailing junk.

+1; that's why I left the comparison in.

(e.g. "md512345678901234567890123456789012zzz" would pass without strlen).

Jonathan

On 4/22/19 9:01 PM, Michael Paquier wrote:

Also, the extra SELECT query with regexp_replace() is a bit overkill
for the purpose, and copying again a copy of the regexp around is no
fun.

In short, I would simplify things as the attached. What do you think?

Outside of the strlen comments, I do like the changes to the tests, +1

I did purposely keep the SCRAM + MD5 changes as separate patches as I
felt they were both atomic, but I have no strong opinions on that.

Thanks,

Jonathan

"Jonathan S. Katz" <jkatz@postgresql.org> writes:

I did purposely keep the SCRAM + MD5 changes as separate patches as I
felt they were both atomic, but I have no strong opinions on that.

FWIW, it looks like one patch to me: tighten our checks on whether
supposedly-encoded passwords fit the encoding rules.

regards, tom lane

On Mon, Apr 22, 2019 at 09:16:49PM -0400, Jonathan S. Katz wrote:

+1; that's why I left the comparison in.

(e.g. "md512345678901234567890123456789012zzz" would pass without strlen).

That's a hard morning... Yes you are right and I can see the failure.
By the way, grouping everything in one patch looks more adapted to me
as this tightens all the checks for the different verifier types.
--
Michael