Date: Tue, 23 Apr 2019 10:01:42 +0900
From: Michael Paquier <michael@paquier.xyz>
To: "Jonathan S. Katz" <jkatz@postgresql.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-bugs@lists.postgresql.org,
	Stephen Frost <sfrost@snowman.net>
Subject: Re: Possible to store invalid SCRAM-SHA-256 Passwords
Message-ID: <20190423010142.GE2712@paquier.xyz>
References: <016deb6b-1f0a-8e9f-1833-a8675b170aa9@postgresql.org>
 <4240a41f-a957-fd7d-7c24-31683f259b9d@postgresql.org>
 <20190422015051.GA3433@paquier.xyz>
 <3033.1555940554@sss.pgh.pa.us>
 <5a93333c-dde6-ee52-f870-4dccfc50eb6f@postgresql.org>
 <30284.1555972921@sss.pgh.pa.us>
 <15140775-08c3-45bd-6dab-fdcd290d1bf0@postgresql.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="qxfKREH7IwbezJ+T"
Content-Disposition: inline
In-Reply-To: <15140775-08c3-45bd-6dab-fdcd290d1bf0@postgresql.org>
Precedence: bulk


--qxfKREH7IwbezJ+T
Content-Type: multipart/mixed; boundary="KJY2Ze80yH5MUxol"
Content-Disposition: inline


--KJY2Ze80yH5MUxol
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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
>>=20
>>      strspn(shadow_pass + 3, MD5_PASSWD_CHARSET) =3D=3D MD5_PASSWD_LEN -=
 3
>>=20
>> It's not like this code isn't very well aware of the first 3 characters
>> being not like the others.
>=20
> 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) =3D=3D 0 && strlen(shadow_pass) =3D=
=3D MD5_PASSWD_LEN &&
+       strspn(shadow_pass + 3, MD5_PASSWD_CHARSET) =3D=3D 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

--KJY2Ze80yH5MUxol
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="scram-md5-invalid.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 3cbe902ace..4c7b149570 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -161,8 +161,6 @@ static char *build_server_first_message(scram_state *st=
ate);
 static char *build_server_final_message(scram_state *state);
 static bool verify_client_proof(scram_state *state);
 static bool verify_final_nonce(scram_state *state);
-static bool parse_scram_verifier(const char *verifier, int *iterations,
-					 char **salt, uint8 *stored_key, uint8 *server_key);
 static void mock_scram_verifier(const char *username, int *iterations,
 					char **salt, uint8 *stored_key, uint8 *server_key);
 static bool is_scram_printable(char *p);
@@ -546,7 +544,7 @@ scram_verify_plain_password(const char *username, const=
 char *password,
  *
  * Returns true if the SCRAM verifier has been parsed, and false otherwise.
  */
-static bool
+bool
 parse_scram_verifier(const char *verifier, int *iterations, char **salt,
 					 uint8 *stored_key, uint8 *server_key)
 {
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index c34e4a3d85..a025e6cf4f 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -20,6 +20,7 @@
=20
 #include "catalog/pg_authid.h"
 #include "common/md5.h"
+#include "common/scram-common.h"
 #include "libpq/crypt.h"
 #include "libpq/scram.h"
 #include "miscadmin.h"
@@ -90,9 +91,16 @@ get_role_password(const char *role, char **logdetail)
 PasswordType
 get_password_type(const char *shadow_pass)
 {
-	if (strncmp(shadow_pass, "md5", 3) =3D=3D 0 && strlen(shadow_pass) =3D=3D=
 MD5_PASSWD_LEN)
+	char	   *encoded_salt;
+	int			iterations;
+	uint8		stored_key[SCRAM_KEY_LEN];
+	uint8		server_key[SCRAM_KEY_LEN];
+
+	if (strncmp(shadow_pass, "md5", 3) =3D=3D 0 &&
+				strspn(shadow_pass + 3, MD5_PASSWD_CHARSET) =3D=3D MD5_PASSWD_LEN - 3)
 		return PASSWORD_TYPE_MD5;
-	if (strncmp(shadow_pass, "SCRAM-SHA-256$", strlen("SCRAM-SHA-256$")) =3D=
=3D 0)
+	if (strncmp(shadow_pass, "SCRAM-SHA-256$", strlen("SCRAM-SHA-256$")) =3D=
=3D 0 &&
+				parse_scram_verifier(shadow_pass, &iterations, &encoded_salt, stored_k=
ey, server_key))
 		return PASSWORD_TYPE_SCRAM_SHA_256;
 	return PASSWORD_TYPE_PLAINTEXT;
 }
diff --git a/src/include/common/md5.h b/src/include/common/md5.h
index cd54c874b1..80b7475f40 100644
--- a/src/include/common/md5.h
+++ b/src/include/common/md5.h
@@ -16,6 +16,7 @@
 #ifndef PG_MD5_H
 #define PG_MD5_H
=20
+#define MD5_PASSWD_CHARSET	"0123456789abcdef"
 #define MD5_PASSWD_LEN	35
=20
 extern bool pg_md5_hash(const void *buff, size_t len, char *hexsum);
diff --git a/src/include/libpq/scram.h b/src/include/libpq/scram.h
index d7f4c094c9..bc297e2a33 100644
--- a/src/include/libpq/scram.h
+++ b/src/include/libpq/scram.h
@@ -29,6 +29,8 @@ extern int pg_be_scram_exchange(void *opaq, const char *i=
nput, int inputlen,
=20
 /* Routines to handle and check SCRAM-SHA-256 verifier */
 extern char *pg_be_scram_build_verifier(const char *password);
+extern bool parse_scram_verifier(const char *verifier, int *iterations, ch=
ar **salt,
+					 uint8 *stored_key, uint8 *server_key);
 extern bool scram_verify_plain_password(const char *username,
 							const char *password, const char *verifier);
=20
diff --git a/src/test/regress/expected/password.out b/src/test/regress/expe=
cted/password.out
index 393d836ead..cad5866e82 100644
--- a/src/test/regress/expected/password.out
+++ b/src/test/regress/expected/password.out
@@ -62,6 +62,12 @@ SET password_encryption =3D 'scram-sha-256';
 ALTER ROLE  regress_passwd4 PASSWORD 'foo';
 -- already encrypted with MD5, use as it is
 CREATE ROLE regress_passwd5 PASSWORD 'md5e73a4b11df52a6068f8b39f90be36023';
+-- This looks like a valid SCRAM-SHA-256 verifier, but it is not
+-- so it should be hashed with SCRAM-SHA-256.
+CREATE ROLE regress_passwd6 PASSWORD 'SCRAM-SHA-256$1234';
+-- This may look like a valid MD5 verifier, but it is not, so it
+-- should be hashed with SCRAM-SHA-256.
+CREATE ROLE regress_passwd7 PASSWORD 'md5012345678901234567890123456789zz';
 SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA=
-Z0-9+/=3D]+)\$([a-zA-Z0-9+=3D/]+):([a-zA-Z0-9+/=3D]+)', '\1$\2:<salt>$<sto=
redkey>:<serverkey>') as rolpassword_masked
     FROM pg_authid
     WHERE rolname LIKE 'regress_passwd%'
@@ -73,7 +79,9 @@ SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-2=
56)\$(\d+):([a-zA-Z0-9+
  regress_passwd3 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
  regress_passwd4 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
  regress_passwd5 | md5e73a4b11df52a6068f8b39f90be36023
-(5 rows)
+ regress_passwd6 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
+ regress_passwd7 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
+(7 rows)
=20
 -- An empty password is not allowed, in any form
 CREATE ROLE regress_passwd_empty PASSWORD '';
@@ -93,6 +101,8 @@ DROP ROLE regress_passwd2;
 DROP ROLE regress_passwd3;
 DROP ROLE regress_passwd4;
 DROP ROLE regress_passwd5;
+DROP ROLE regress_passwd6;
+DROP ROLE regress_passwd7;
 DROP ROLE regress_passwd_empty;
 -- all entries should have been removed
 SELECT rolname, rolpassword
diff --git a/src/test/regress/sql/password.sql b/src/test/regress/sql/passw=
ord.sql
index 8f8252d127..d81c1c7186 100644
--- a/src/test/regress/sql/password.sql
+++ b/src/test/regress/sql/password.sql
@@ -54,6 +54,13 @@ ALTER ROLE  regress_passwd4 PASSWORD 'foo';
 -- already encrypted with MD5, use as it is
 CREATE ROLE regress_passwd5 PASSWORD 'md5e73a4b11df52a6068f8b39f90be36023';
=20
+-- This looks like a valid SCRAM-SHA-256 verifier, but it is not
+-- so it should be hashed with SCRAM-SHA-256.
+CREATE ROLE regress_passwd6 PASSWORD 'SCRAM-SHA-256$1234';
+-- This may look like a valid MD5 verifier, but it is not, so it
+-- should be hashed with SCRAM-SHA-256.
+CREATE ROLE regress_passwd7 PASSWORD 'md5012345678901234567890123456789zz';
+
 SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA=
-Z0-9+/=3D]+)\$([a-zA-Z0-9+=3D/]+):([a-zA-Z0-9+/=3D]+)', '\1$\2:<salt>$<sto=
redkey>:<serverkey>') as rolpassword_masked
     FROM pg_authid
     WHERE rolname LIKE 'regress_passwd%'
@@ -70,6 +77,8 @@ DROP ROLE regress_passwd2;
 DROP ROLE regress_passwd3;
 DROP ROLE regress_passwd4;
 DROP ROLE regress_passwd5;
+DROP ROLE regress_passwd6;
+DROP ROLE regress_passwd7;
 DROP ROLE regress_passwd_empty;
=20
 -- all entries should have been removed

--KJY2Ze80yH5MUxol--

--qxfKREH7IwbezJ+T
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAly+Y/YACgkQnvQgOdby
QH0oGhAAmh2nCysQbkUUNBIYvpfhsVBuCMvMZIMKo4eUt0Z2Y/HR3jLuYhwHh0uZ
dqYV4i3wjWm4L+YBvXuOX5g3knb3Tl/gzeoqKkYk/+ySKvsMfghjM1KC8mh+vWC7
h+RGUekTI5P6mi+JcNllCBAOTdHozTRMpwYhpH1mYMYXXSsJpZICkdhUgoW+n5Wn
txVFd9fntsnhAyxl41ouV9pgZgjh45mh+crd5jO4AK0xa0uhFahnv45ocKxWHVdb
4JtHEx0H7oGMmcZWoz1Fv0k6hHcj5H4slzE0ZLZJRZCZ1pa5MVioxXuIpsVMjEx5
TaPswpgPdiPIghNFDunfId+NIlmcZBwKyKBP8MvGKZoTgBDB1+nrPKqQeP6NNwha
Jmnl0oUxkfT8lAM+VsuRPhQ14PkAq1x/JVbx36hor1k1OE4N1XDmegymBX1B26JN
a4Z2yVxMRmA18sBquM56rw3r1XNDpgJD0USt+QYWmF6wE/ptTS0L2Je3wbyjhLY6
Nou4EpYZ0EzOhlNe+ngGK5JIj8Cf7LdmuHBYTHbIT62A6Ao7udXz655PEGIQ5/c7
3rtktdsxhG6C5NfunXdVDp5/x+r0QA0pwjW9W866HeA3k+FnFcy9vAeVCXVOz8pG
Godbis5A3pOZgZW9Phet4rpBlw+FjtS7PCKw4Rnjhkb4VpHJoN4=
=8KuP
-----END PGP SIGNATURE-----

--qxfKREH7IwbezJ+T--