Fix detection of passwords hashed with MD5 or SCRAM-SHA-256

This commit fixes a couple of issues related to the way password
verifiers hashed with MD5 or SCRAM-SHA-256 are detected, leading to
being able to store in catalogs passwords which do not follow the
supported hash formats:
- A MD5-hashed entry was checked based on if its header uses "md5" and
if the string length matches what is expected.  Unfortunately the code
never checked if the hash only used hexadecimal characters, as reported
by Tom Lane.
- A SCRAM-hashed entry was checked based on only its header, which
should be "SCRAM-SHA-256$", but it never checked for any fields
afterwards, as reported by Jonathan Katz.

Backpatch down to v10, which is where SCRAM has been introduced, and
where password verifiers in plain format have been removed.

Author: Jonathan Katz
Reviewed-by: Tom Lane, Michael Paquier
Discussion: https://postgr.es/m/016deb6b-1f0a-8e9f-1833-a8675b170aa9@postgresql.org
Backpatch-through: 10

diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index d69d7dde06a6ac136fd36c92b8e5fb07ab8efac8..ccc49c1fea2f537ceb1a185f77081a13239fc2c5 100644 (file)
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -149,8 +149,6 @@ static char *build_server_first_message(scram_state *state);
 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);
@@ -476,7 +474,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 1715c5246225662788f6dd19dbc40404ff3aee14..e39fdacc4919692a39321fdd8f3ea70e5830ebcd 100644 (file)
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -20,6 +20,7 @@
 
 #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,17 @@ get_role_password(const char *role, char **logdetail)
 PasswordType
 get_password_type(const char *shadow_pass)
 {
-       if (strncmp(shadow_pass, "md5", 3) == 0 && strlen(shadow_pass) == 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) == 0 &&
+               strlen(shadow_pass) == MD5_PASSWD_LEN &&
+               strspn(shadow_pass + 3, MD5_PASSWD_CHARSET) == MD5_PASSWD_LEN - 3)
                return PASSWORD_TYPE_MD5;
-       if (strncmp(shadow_pass, "SCRAM-SHA-256$", strlen("SCRAM-SHA-256$")) == 0)
+       if (parse_scram_verifier(shadow_pass, &iterations, &encoded_salt,
+                                                        stored_key, 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 ccaaeddbf4e9afeefe05740d6647a002ac29e746..c9874bd82042626f631c7d5ddc96cbf5936c1607 100644 (file)
--- a/src/include/common/md5.h
+++ b/src/include/common/md5.h
@@ -16,6 +16,7 @@
 #ifndef PG_MD5_H
 #define PG_MD5_H
 
+#define MD5_PASSWD_CHARSET     "0123456789abcdef"
 #define MD5_PASSWD_LEN 35
 
 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 61ceeeeb2769cdec543e42fdfa314720d80f8fe1..97765b0081aa58dcb5545cfac6e60050056cd2fd 100644 (file)
--- a/src/include/libpq/scram.h
+++ b/src/include/libpq/scram.h
@@ -28,6 +28,8 @@ extern int pg_be_scram_exchange(void *opaq, char *input, int inputlen,
 
 /* 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, char **salt,
+                                        uint8 *stored_key, uint8 *server_key);
 extern bool scram_verify_plain_password(const char *username,
                                                        const char *password, const char *verifier);
 

diff --git a/src/test/regress/expected/password.out b/src/test/regress/expected/password.out
index 393d836eada1cdf4b30892ef73ea2ab374a65b2e..971e290a3210a6f0ec6c0d01f9ad70e9a65a0e77 100644 (file)
--- a/src/test/regress/expected/password.out
+++ b/src/test/regress/expected/password.out
@@ -62,6 +62,15 @@ SET password_encryption = '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';
+-- These may look like valid MD5 verifiers, but they are not, so they
+-- should be hashed with SCRAM-SHA-256.
+-- trailing garbage at the end
+CREATE ROLE regress_passwd7 PASSWORD 'md5012345678901234567890123456789zz';
+-- invalid length
+CREATE ROLE regress_passwd8 PASSWORD 'md501234567890123456789012345678901zz';
 SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+/=]+)\$([a-zA-Z0-9+=/]+):([a-zA-Z0-9+/=]+)', '\1$\2:<salt>$<storedkey>:<serverkey>') as rolpassword_masked
     FROM pg_authid
     WHERE rolname LIKE 'regress_passwd%'
@@ -73,7 +82,10 @@ SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\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>
+ regress_passwd8 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
+(8 rows)
 
 -- An empty password is not allowed, in any form
 CREATE ROLE regress_passwd_empty PASSWORD '';
@@ -93,6 +105,9 @@ 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_passwd8;
 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/password.sql
index 8f8252d127f0bae3f063d0de5178cdedf873e22c..89b6d4b278dfaa162bb89be475151b6523ec1e4e 100644 (file)
--- a/src/test/regress/sql/password.sql
+++ b/src/test/regress/sql/password.sql
@@ -54,6 +54,16 @@ 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';
+-- These may look like valid MD5 verifiers, but they are not, so they
+-- should be hashed with SCRAM-SHA-256.
+-- trailing garbage at the end
+CREATE ROLE regress_passwd7 PASSWORD 'md5012345678901234567890123456789zz';
+-- invalid length
+CREATE ROLE regress_passwd8 PASSWORD 'md501234567890123456789012345678901zz';
+
 SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+/=]+)\$([a-zA-Z0-9+=/]+):([a-zA-Z0-9+/=]+)', '\1$\2:<salt>$<storedkey>:<serverkey>') as rolpassword_masked
     FROM pg_authid
     WHERE rolname LIKE 'regress_passwd%'
@@ -70,6 +80,9 @@ 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_passwd8;
 DROP ROLE regress_passwd_empty;
 
 -- all entries should have been removed