From: Jonathan S. Katz <jkatz@postgresql.org>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-bugs@lists.postgresql.org, Stephen Frost <sfrost@snowman.net>
Subject: Re: Possible to store invalid SCRAM-SHA-256 Passwords
Date: Mon, 22 Apr 2019 16:57:40 -0400
Message-ID: <32661faa-0254-d37e-7930-00d823f38f9f@postgresql.org> (raw)
In-Reply-To: <17ed1a12-14df-a501-8b58-f4d1eba49e3f@postgresql.org>
References: <016deb6b-1f0a-8e9f-1833-a8675b170aa9@postgresql.org>
<4240a41f-a957-fd7d-7c24-31683f259b9d@postgresql.org>
<20190422015051.GA3433@paquier.xyz>
<17ed1a12-14df-a501-8b58-f4d1eba49e3f@postgresql.org>
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
From 26e52e72010a4c25a69ff32f3a960298b3a99250 Mon Sep 17 00:00:00 2001
From: "Jonathan S. Katz" <jonathan.katz@excoventures.com>
Date: Sat, 20 Apr 2019 16:02:32 -0400
Subject: [PATCH] Modified SCRAM-SHA-256 validation check for get_password_type
The get_password_type function previously just checked to see if a password
started with "SCRAM-SHA-256$" to determine whether or not it was a
SCRAM-SHA-256 hashed password. However, this would cause passwords created
in this was to be unusuable.
This changes get_password_type methodology to actually perform SCRAM-SHA-256
verification on the password to see if it is a valid hash. If not, it will
proceed to the next password type check.
---
src/backend/libpq/auth-scram.c | 4 +---
src/backend/libpq/crypt.c | 9 ++++++++-
src/include/libpq/scram.h | 2 ++
src/test/regress/expected/password.out | 11 +++++++++++
src/test/regress/sql/password.sql | 7 +++++++
5 files changed, 29 insertions(+), 4 deletions(-)
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.cindex 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 *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);
@@ -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.cindex c34e4a3d85..db19f51c80 100644--- 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,15 @@ get_role_password(const char *role, char **logdetail)
PasswordType
get_password_type(const char *shadow_pass)
{
+ 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)
return PASSWORD_TYPE_MD5;
- if (strncmp(shadow_pass, "SCRAM-SHA-256$", strlen("SCRAM-SHA-256$")) == 0)+ if (strncmp(shadow_pass, "SCRAM-SHA-256$", strlen("SCRAM-SHA-256$")) == 0 &&+ 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/libpq/scram.h b/src/include/libpq/scram.hindex 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 *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.outindex 393d836ead..d40dcf9a43 100644--- a/src/test/regress/expected/password.out+++ b/src/test/regress/expected/password.out@@ -75,6 +75,16 @@ SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+
regress_passwd5 | md5e73a4b11df52a6068f8b39f90be36023
(5 rows)
+-- An invalid SCRAM-SHA-256 that is encrypted is streated as a SCRAM-SHA-256 password+CREATE ROLE regress_passwd6 PASSWORD 'SCRAM-SHA-256$1234';+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='regress_passwd6';+ rolname | rolpassword_masked +-----------------+---------------------------------------------------+ regress_passwd6 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>+(1 row)+
-- An empty password is not allowed, in any form
CREATE ROLE regress_passwd_empty PASSWORD '';
NOTICE: empty string is not a valid password, clearing password
@@ -93,6 +103,7 @@ DROP ROLE regress_passwd2;
DROP ROLE regress_passwd3;
DROP ROLE regress_passwd4;
DROP ROLE regress_passwd5;
+DROP ROLE regress_passwd6;
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.sqlindex 8f8252d127..b5a0a89182 100644--- a/src/test/regress/sql/password.sql+++ b/src/test/regress/sql/password.sql@@ -59,6 +59,12 @@ SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+
WHERE rolname LIKE 'regress_passwd%'
ORDER BY rolname, rolpassword;
+-- An invalid SCRAM-SHA-256 that is encrypted is streated as a SCRAM-SHA-256 password+CREATE ROLE regress_passwd6 PASSWORD 'SCRAM-SHA-256$1234';+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='regress_passwd6';+
-- An empty password is not allowed, in any form
CREATE ROLE regress_passwd_empty PASSWORD '';
ALTER ROLE regress_passwd_empty PASSWORD 'md585939a5ce845f1a1b620742e3c659e0a';
@@ -70,6 +76,7 @@ DROP ROLE regress_passwd2;
DROP ROLE regress_passwd3;
DROP ROLE regress_passwd4;
DROP ROLE regress_passwd5;
+DROP ROLE regress_passwd6;
DROP ROLE regress_passwd_empty;
-- all entries should have been removed
--
2.14.3 (Apple Git-98)
Attachments:
[text/plain] 0001-Modified-SCRAM-SHA-256-validation-check-for-get_pass_v2.patch (6.2K, ../32661faa-0254-d37e-7930-00d823f38f9f@postgresql.org/2-0001-Modified-SCRAM-SHA-256-validation-check-for-get_pass_v2.patch)
download | inline diff:
From 26e52e72010a4c25a69ff32f3a960298b3a99250 Mon Sep 17 00:00:00 2001
From: "Jonathan S. Katz" <jonathan.katz@excoventures.com>
Date: Sat, 20 Apr 2019 16:02:32 -0400
Subject: [PATCH] Modified SCRAM-SHA-256 validation check for get_password_type
The get_password_type function previously just checked to see if a password
started with "SCRAM-SHA-256$" to determine whether or not it was a
SCRAM-SHA-256 hashed password. However, this would cause passwords created
in this was to be unusuable.
This changes get_password_type methodology to actually perform SCRAM-SHA-256
verification on the password to see if it is a valid hash. If not, it will
proceed to the next password type check.
---
src/backend/libpq/auth-scram.c | 4 +---
src/backend/libpq/crypt.c | 9 ++++++++-
src/include/libpq/scram.h | 2 ++
src/test/regress/expected/password.out | 11 +++++++++++
src/test/regress/sql/password.sql | 7 +++++++
5 files changed, 29 insertions(+), 4 deletions(-)
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.cindex 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 *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);
@@ -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.cindex c34e4a3d85..db19f51c80 100644--- 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,15 @@ get_role_password(const char *role, char **logdetail)
PasswordType
get_password_type(const char *shadow_pass)
{
+ 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)
return PASSWORD_TYPE_MD5;
- if (strncmp(shadow_pass, "SCRAM-SHA-256$", strlen("SCRAM-SHA-256$")) == 0)+ if (strncmp(shadow_pass, "SCRAM-SHA-256$", strlen("SCRAM-SHA-256$")) == 0 &&+ 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/libpq/scram.h b/src/include/libpq/scram.hindex 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 *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.outindex 393d836ead..d40dcf9a43 100644--- a/src/test/regress/expected/password.out+++ b/src/test/regress/expected/password.out@@ -75,6 +75,16 @@ SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+
regress_passwd5 | md5e73a4b11df52a6068f8b39f90be36023
(5 rows)
+-- An invalid SCRAM-SHA-256 that is encrypted is streated as a SCRAM-SHA-256 password+CREATE ROLE regress_passwd6 PASSWORD 'SCRAM-SHA-256$1234';+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='regress_passwd6';+ rolname | rolpassword_masked +-----------------+---------------------------------------------------+ regress_passwd6 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>+(1 row)+
-- An empty password is not allowed, in any form
CREATE ROLE regress_passwd_empty PASSWORD '';
NOTICE: empty string is not a valid password, clearing password
@@ -93,6 +103,7 @@ DROP ROLE regress_passwd2;
DROP ROLE regress_passwd3;
DROP ROLE regress_passwd4;
DROP ROLE regress_passwd5;
+DROP ROLE regress_passwd6;
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.sqlindex 8f8252d127..b5a0a89182 100644--- a/src/test/regress/sql/password.sql+++ b/src/test/regress/sql/password.sql@@ -59,6 +59,12 @@ SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+
WHERE rolname LIKE 'regress_passwd%'
ORDER BY rolname, rolpassword;
+-- An invalid SCRAM-SHA-256 that is encrypted is streated as a SCRAM-SHA-256 password+CREATE ROLE regress_passwd6 PASSWORD 'SCRAM-SHA-256$1234';+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='regress_passwd6';+
-- An empty password is not allowed, in any form
CREATE ROLE regress_passwd_empty PASSWORD '';
ALTER ROLE regress_passwd_empty PASSWORD 'md585939a5ce845f1a1b620742e3c659e0a';
@@ -70,6 +76,7 @@ DROP ROLE regress_passwd2;
DROP ROLE regress_passwd3;
DROP ROLE regress_passwd4;
DROP ROLE regress_passwd5;
+DROP ROLE regress_passwd6;
DROP ROLE regress_passwd_empty;
-- all entries should have been removed
--
2.14.3 (Apple Git-98)
[application/pgp-signature] signature.asc (832B, ../32661faa-0254-d37e-7930-00d823f38f9f@postgresql.org/3-signature.asc)
download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: jkatz@postgresql.org, michael@paquier.xyz, sfrost@snowman.net
Subject: Re: Possible to store invalid SCRAM-SHA-256 Passwords
In-Reply-To: <32661faa-0254-d37e-7930-00d823f38f9f@postgresql.org>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox