pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedPossible to store invalid SCRAM-SHA-256 Passwords
27+ messages / 5 participants
[nested] [flat]
* Possible to store invalid SCRAM-SHA-256 Passwords
@ 2019-04-20 19:26 Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
0 siblings, 1 reply; 27+ messages in thread
From: Jonathan S. Katz @ 2019-04-20 19:26 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: Stephen Frost <sfrost@snowman.net>
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.
Jonathan
Attachments:
[application/pgp-signature] signature.asc (832B, ../../016deb6b-1f0a-8e9f-1833-a8675b170aa9@postgresql.org/2-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
@ 2019-04-20 20:12 ` Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 27+ messages in thread
From: Jonathan S. Katz @ 2019-04-20 20:12 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: Stephen Frost <sfrost@snowman.net>
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
From a8b999ed44fd636fce05bee8fcaffedb9b0fb9a7 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 ++
3 files changed, 11 insertions(+), 4 deletions(-)
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 *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.c
index 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.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 *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);
--
2.14.3 (Apple Git-98)
Attachments:
[text/plain] 0001-Modified-SCRAM-SHA-256-validation-check-for-get_pass.patch (3.6K, ../../4240a41f-a957-fd7d-7c24-31683f259b9d@postgresql.org/2-0001-Modified-SCRAM-SHA-256-validation-check-for-get_pass.patch)
download | inline diff:
From a8b999ed44fd636fce05bee8fcaffedb9b0fb9a7 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 ++
3 files changed, 11 insertions(+), 4 deletions(-)
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 *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.c
index 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.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 *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);
--
2.14.3 (Apple Git-98)
[application/pgp-signature] signature.asc (832B, ../../4240a41f-a957-fd7d-7c24-31683f259b9d@postgresql.org/3-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
@ 2019-04-22 01:50 ` Michael Paquier <michael@paquier.xyz>
2019-04-22 13:04 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 27+ messages in thread
From: Michael Paquier @ 2019-04-22 01:50 UTC (permalink / raw)
To: Jonathan S. Katz <jkatz@postgresql.org>; +Cc: pgsql-bugs@lists.postgresql.org, Stephen Frost <sfrost@snowman.net>
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20190422015051.GA3433@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
@ 2019-04-22 13:04 ` Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 20:57 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
1 sibling, 1 reply; 27+ messages in thread
From: Jonathan S. Katz @ 2019-04-22 13:04 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs@lists.postgresql.org, Stephen Frost <sfrost@snowman.net>
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../17ed1a12-14df-a501-8b58-f4d1eba49e3f@postgresql.org/2-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:04 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
@ 2019-04-22 20:57 ` Jonathan S. Katz <jkatz@postgresql.org>
0 siblings, 0 replies; 27+ messages in thread
From: Jonathan S. Katz @ 2019-04-22 20:57 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs@lists.postgresql.org, Stephen Frost <sfrost@snowman.net>
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.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 *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.c
index 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.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 *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 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.sql
index 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.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 *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.c
index 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.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 *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 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.sql
index 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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
@ 2019-04-22 13:42 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 13:52 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Stephen Frost <sfrost@snowman.net>
2019-04-22 14:20 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 22:32 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
1 sibling, 3 replies; 27+ messages in thread
From: Tom Lane @ 2019-04-22 13:42 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Jonathan S. Katz <jkatz@postgresql.org>; pgsql-bugs@lists.postgresql.org, Stephen Frost <sfrost@snowman.net>
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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
@ 2019-04-22 13:52 ` Stephen Frost <sfrost@snowman.net>
2019-04-22 21:49 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords raf@raf.org
2019-04-23 00:06 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2 siblings, 2 replies; 27+ messages in thread
From: Stephen Frost @ 2019-04-22 13:52 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Michael Paquier <michael@paquier.xyz>; Jonathan S. Katz <jkatz@postgresql.org>; pgsql-bugs@lists.postgresql.org
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
Attachments:
[application/pgp-signature] signature.asc (818B, ../../20190422135215.GG6197@tamriel.snowman.net/2-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 13:52 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Stephen Frost <sfrost@snowman.net>
@ 2019-04-22 21:49 ` raf@raf.org
2019-04-23 14:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Stephen Frost <sfrost@snowman.net>
1 sibling, 1 reply; 27+ messages in thread
From: raf@raf.org @ 2019-04-22 21:49 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
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.
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 13:52 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Stephen Frost <sfrost@snowman.net>
2019-04-22 21:49 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords raf@raf.org
@ 2019-04-23 14:42 ` Stephen Frost <sfrost@snowman.net>
2019-04-23 14:55 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 27+ messages in thread
From: Stephen Frost @ 2019-04-23 14:42 UTC (permalink / raw)
To: raf@raf.org; +Cc: pgsql-bugs@lists.postgresql.org
Greetings,
* raf@raf.org (raf@raf.org) wrote:
> 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.
>
> 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.
There isn't a way to make it unambiguous given that we accept
more-or-less anything as a plaintext password though, that would be the
issue here..
Thanks!
Stephen
Attachments:
[application/pgp-signature] signature.asc (818B, ../../20190423144159.GP6197@tamriel.snowman.net/2-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 13:52 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Stephen Frost <sfrost@snowman.net>
2019-04-22 21:49 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords raf@raf.org
2019-04-23 14:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Stephen Frost <sfrost@snowman.net>
@ 2019-04-23 14:55 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 27+ messages in thread
From: Tom Lane @ 2019-04-23 14:55 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: raf@raf.org; pgsql-bugs@lists.postgresql.org
Stephen Frost <sfrost@snowman.net> writes:
> * raf@raf.org (raf@raf.org) wrote:
>> 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.
> There isn't a way to make it unambiguous given that we accept
> more-or-less anything as a plaintext password though, that would be the
> issue here..
In practice, particularly with the extra validation we just added,
it seems vanishingly unlikely that anyone would choose a password
that just happened to look like one of the hashed formats.
If somebody intentionally chooses such a password, well, it's on
their heads whether the outcome is what they want.
regards, tom lane
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 13:52 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Stephen Frost <sfrost@snowman.net>
@ 2019-04-23 00:06 ` Michael Paquier <michael@paquier.xyz>
2019-04-23 14:43 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Stephen Frost <sfrost@snowman.net>
1 sibling, 1 reply; 27+ messages in thread
From: Michael Paquier @ 2019-04-23 00:06 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Jonathan S. Katz <jkatz@postgresql.org>; pgsql-bugs@lists.postgresql.org
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20190423000619.GD2712@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 13:52 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Stephen Frost <sfrost@snowman.net>
2019-04-23 00:06 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
@ 2019-04-23 14:43 ` Stephen Frost <sfrost@snowman.net>
0 siblings, 0 replies; 27+ messages in thread
From: Stephen Frost @ 2019-04-23 14:43 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Jonathan S. Katz <jkatz@postgresql.org>; pgsql-bugs@lists.postgresql.org
Greetings,
* Michael Paquier (michael@paquier.xyz) wrote:
> 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.
Yes, having an indepedent catalog table would have been a good approach
too, much better than where we're at now. I hope someone has time to
work on that for a future version.
Thanks!
Stephen
Attachments:
[application/pgp-signature] signature.asc (818B, ../../20190423144306.GQ6197@tamriel.snowman.net/2-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
@ 2019-04-22 14:20 ` Jonathan S. Katz <jkatz@postgresql.org>
2 siblings, 0 replies; 27+ messages in thread
From: Jonathan S. Katz @ 2019-04-22 14:20 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs@lists.postgresql.org, Stephen Frost <sfrost@snowman.net>
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../7e5b5f9e-6cde-b066-837d-9af22e7b1152@postgresql.org/2-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
@ 2019-04-22 22:32 ` Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 22:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 1 reply; 27+ messages in thread
From: Jonathan S. Katz @ 2019-04-22 22:32 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs@lists.postgresql.org, Stephen Frost <sfrost@snowman.net>
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../5a93333c-dde6-ee52-f870-4dccfc50eb6f@postgresql.org/2-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 22:32 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
@ 2019-04-22 22:42 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 23:36 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
0 siblings, 1 reply; 27+ messages in thread
From: Tom Lane @ 2019-04-22 22:42 UTC (permalink / raw)
To: Jonathan S. Katz <jkatz@postgresql.org>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-bugs@lists.postgresql.org, Stephen Frost <sfrost@snowman.net>
"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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 22:32 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 22:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
@ 2019-04-22 23:36 ` Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 23:55 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-23 01:01 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
0 siblings, 2 replies; 27+ messages in thread
From: Jonathan S. Katz @ 2019-04-22 23:36 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-bugs@lists.postgresql.org, Stephen Frost <sfrost@snowman.net>
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
From ab205e6fa82081d4b00fa8b67b0f33163f950cc4 Mon Sep 17 00:00:00 2001
From: "Jonathan S. Katz" <jonathan.katz@excoventures.com>
Date: Mon, 22 Apr 2019 19:27:15 -0400
Subject: [PATCH] Provide additional check to determine if a password is a
valid MD5 hash.
The present check in "get_password_type" only checks if a password contains
the "md5" prefix and is of length MD5_PASSWD_LEN. However, this mean that
there could be invalid md5 characters stored in the digest.
This fix explicitly checks to see if a password prefixed with "md5" actually
contains the valid set of characters that can be used in a MD5 hash.
---
src/backend/libpq/crypt.c | 3 ++-
src/include/common/md5.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index db19f51c80..5617862243 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -96,7 +96,8 @@ get_password_type(const char *shadow_pass)
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)
+ 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 &&
parse_scram_verifier(shadow_pass, &iterations, &encoded_salt, stored_key, server_key))
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
+#define MD5_PASSWD_CHARSET "0123456789abcdef"
#define MD5_PASSWD_LEN 35
extern bool pg_md5_hash(const void *buff, size_t len, char *hexsum);
--
2.14.3 (Apple Git-98)
Attachments:
[text/plain] 0001-Provide-additional-check-to-determine-if-a-password-.patch (1.8K, ../../15140775-08c3-45bd-6dab-fdcd290d1bf0@postgresql.org/2-0001-Provide-additional-check-to-determine-if-a-password-.patch)
download | inline diff:
From ab205e6fa82081d4b00fa8b67b0f33163f950cc4 Mon Sep 17 00:00:00 2001
From: "Jonathan S. Katz" <jonathan.katz@excoventures.com>
Date: Mon, 22 Apr 2019 19:27:15 -0400
Subject: [PATCH] Provide additional check to determine if a password is a
valid MD5 hash.
The present check in "get_password_type" only checks if a password contains
the "md5" prefix and is of length MD5_PASSWD_LEN. However, this mean that
there could be invalid md5 characters stored in the digest.
This fix explicitly checks to see if a password prefixed with "md5" actually
contains the valid set of characters that can be used in a MD5 hash.
---
src/backend/libpq/crypt.c | 3 ++-
src/include/common/md5.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index db19f51c80..5617862243 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -96,7 +96,8 @@ get_password_type(const char *shadow_pass)
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)
+ 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 &&
parse_scram_verifier(shadow_pass, &iterations, &encoded_salt, stored_key, server_key))
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
+#define MD5_PASSWD_CHARSET "0123456789abcdef"
#define MD5_PASSWD_LEN 35
extern bool pg_md5_hash(const void *buff, size_t len, char *hexsum);
--
2.14.3 (Apple Git-98)
[application/pgp-signature] signature.asc (832B, ../../15140775-08c3-45bd-6dab-fdcd290d1bf0@postgresql.org/3-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 22:32 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 22:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 23:36 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
@ 2019-04-22 23:55 ` Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 27+ messages in thread
From: Tom Lane @ 2019-04-22 23:55 UTC (permalink / raw)
To: Jonathan S. Katz <jkatz@postgresql.org>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-bugs@lists.postgresql.org, Stephen Frost <sfrost@snowman.net>
"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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 22:32 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 22:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 23:36 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
@ 2019-04-23 01:01 ` Michael Paquier <michael@paquier.xyz>
2019-04-23 01:10 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-23 01:26 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
1 sibling, 2 replies; 27+ messages in thread
From: Michael Paquier @ 2019-04-23 01:01 UTC (permalink / raw)
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>
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
Attachments:
[text/x-diff] scram-md5-invalid.patch (6.2K, ../../20190423010142.GE2712@paquier.xyz/2-scram-md5-invalid.patch)
download | inline diff:
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 *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.c
index c34e4a3d85..a025e6cf4f 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,16 @@ 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 &&
+ 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 (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/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
+#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 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.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 = '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+/=]+)\$([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 +79,9 @@ 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>
+(7 rows)
-- 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/password.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';
+-- 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+/=]+)\$([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 +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;
-- all entries should have been removed
[application/pgp-signature] signature.asc (832B, ../../20190423010142.GE2712@paquier.xyz/3-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 22:32 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 22:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 23:36 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-23 01:01 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
@ 2019-04-23 01:10 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-04-23 01:16 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
1 sibling, 1 reply; 27+ messages in thread
From: Tom Lane @ 2019-04-23 01:10 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Jonathan S. Katz <jkatz@postgresql.org>; pgsql-bugs@lists.postgresql.org, Stephen Frost <sfrost@snowman.net>
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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 22:32 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 22:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 23:36 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-23 01:01 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-23 01:10 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
@ 2019-04-23 01:16 ` Jonathan S. Katz <jkatz@postgresql.org>
2019-04-23 02:10 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 27+ messages in thread
From: Jonathan S. Katz @ 2019-04-23 01:16 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs@lists.postgresql.org, Stephen Frost <sfrost@snowman.net>
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../265876b8-1a72-89cd-bdcf-e641d93e166c@postgresql.org/2-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 22:32 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 22:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 23:36 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-23 01:01 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-23 01:10 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-23 01:16 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
@ 2019-04-23 02:10 ` Michael Paquier <michael@paquier.xyz>
2019-04-23 06:57 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 27+ messages in thread
From: Michael Paquier @ 2019-04-23 02:10 UTC (permalink / raw)
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>
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20190423021018.GF2712@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 22:32 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 22:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 23:36 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-23 01:01 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-23 01:10 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-23 01:16 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-23 02:10 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
@ 2019-04-23 06:57 ` Michael Paquier <michael@paquier.xyz>
2019-04-23 14:19 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
0 siblings, 1 reply; 27+ messages in thread
From: Michael Paquier @ 2019-04-23 06:57 UTC (permalink / raw)
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>
On Tue, Apr 23, 2019 at 11:10:18AM +0900, Michael Paquier wrote:
> 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.
The afternoon has been better. I have double-checked your patch and
committed it down to v10. Now, there are two things which may need
extra handling:
- Do we add a note in the release notes about that with a SQL query
checking the state of pg_authid?
- In ~9.6 we include in md5.h a macro which does not care about hex
characters in the MD5 hash. I think that we should fix that as well,
or perhaps that's not worth caring per the lack of complaints?
Attached is what would be needed.
--
Michael
Attachments:
[text/x-diff] md5-check-96-v1.patch (584B, ../../20190423065701.GL2712@paquier.xyz/2-md5-check-96-v1.patch)
download | inline diff:
diff --git a/src/include/libpq/md5.h b/src/include/libpq/md5.h
index f3eec8b4f4..fac2636c07 100644
--- a/src/include/libpq/md5.h
+++ b/src/include/libpq/md5.h
@@ -16,10 +16,12 @@
#ifndef PG_MD5_H
#define PG_MD5_H
+#define MD5_PASSWD_CHARSET "0123456789abcdef"
#define MD5_PASSWD_LEN 35
#define isMD5(passwd) (strncmp(passwd, "md5", 3) == 0 && \
- strlen(passwd) == MD5_PASSWD_LEN)
+ strlen(passwd) == MD5_PASSWD_LEN && \
+ strspn(passwd + 3, MD5_PASSWD_CHARSET) == MD5_PASSWD_LEN - 3)
extern bool pg_md5_hash(const void *buff, size_t len, char *hexsum);
[application/pgp-signature] signature.asc (832B, ../../20190423065701.GL2712@paquier.xyz/3-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 22:32 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 22:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 23:36 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-23 01:01 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-23 01:10 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-23 01:16 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-23 02:10 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-23 06:57 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
@ 2019-04-23 14:19 ` Jonathan S. Katz <jkatz@postgresql.org>
2019-04-23 23:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 27+ messages in thread
From: Jonathan S. Katz @ 2019-04-23 14:19 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs@lists.postgresql.org, Stephen Frost <sfrost@snowman.net>
On 4/23/19 2:57 AM, Michael Paquier wrote:
> On Tue, Apr 23, 2019 at 11:10:18AM +0900, Michael Paquier wrote:
>> 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.
>
> The afternoon has been better. I have double-checked your patch and
> committed it down to v10.
Thanks!
> Now, there are two things which may need
> extra handling:
> - Do we add a note in the release notes about that with a SQL query
> checking the state of pg_authid?
Hmm...well, let's sound it out:
- If you have an invalid SCRAM-SHA-256 password (e.g.
SCRAM-SHA-256$1234), you would have been unable to log in anyway, so in
all likelihood you would either have had an admin reset your password,
or you gave up.
- If you had a md5 hash with bogus characters in it, it'd be the above
as well
So likely it's been resolved in some way: the user has been issued a new
password or has given up on PostgreSQL
With that said, we could do something like:
"To determine if this release affects an of your users ability to log in
using either the SCRAM-SHA-256 on MD5 password based methods, you can
run the following query:
<query that finds the now invalid hashes>
We advise that you reset the passwords for these users.
"
> - In ~9.6 we include in md5.h a macro which does not care about hex
> characters in the MD5 hash. I think that we should fix that as well,
> or perhaps that's not worth caring per the lack of complaints?
> Attached is what would be needed.
+1 for fixing so its consistent (at least from a behavior standpoint).
I confirmed that it's in 9.5 & 9.4 as well.
Thanks,
Jonathan
Attachments:
[application/pgp-signature] signature.asc (832B, ../../43882bd7-a002-dd50-bf9a-401560285485@postgresql.org/2-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 22:32 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 22:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 23:36 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-23 01:01 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-23 01:10 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-23 01:16 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-23 02:10 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-23 06:57 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-23 14:19 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
@ 2019-04-23 23:12 ` Michael Paquier <michael@paquier.xyz>
2019-04-24 00:07 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 27+ messages in thread
From: Michael Paquier @ 2019-04-23 23:12 UTC (permalink / raw)
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>
On Tue, Apr 23, 2019 at 10:19:30AM -0400, Jonathan S. Katz wrote:
> - If you have an invalid SCRAM-SHA-256 password (e.g.
> SCRAM-SHA-256$1234), you would have been unable to log in anyway, so in
> all likelihood you would either have had an admin reset your password,
> or you gave up.
> - If you had a md5 hash with bogus characters in it, it'd be the above
> as well
>
> So likely it's been resolved in some way: the user has been issued a new
> password or has given up on PostgreSQL
>
> With that said, we could do something like:
>
> "To determine if this release affects an of your users ability to log in
> using either the SCRAM-SHA-256 on MD5 password based methods, you can
> run the following query:
s/an of/any of/.
> We advise that you reset the passwords for these users.
Sounds fine to me, thanks. I am not sure if we would want to have
something in the release notes, on a wiki page with the release notes
including a link to it, or just no direct mention in the release
notes. In the past, say for the issue with the incorrect VM page
references, we have been a wiki page with queries and such for
diagnostics.
> +1 for fixing so its consistent (at least from a behavior standpoint).
>
> I confirmed that it's in 9.5 & 9.4 as well.
Thanks for confirming, I am going to patch 9.4~9.6 with that.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20190423231216.GA1878@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 22:32 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 22:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 23:36 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-23 01:01 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-23 01:10 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-23 01:16 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-23 02:10 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-23 06:57 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-23 14:19 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-23 23:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
@ 2019-04-24 00:07 ` Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 27+ messages in thread
From: Michael Paquier @ 2019-04-24 00:07 UTC (permalink / raw)
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>
On Wed, Apr 24, 2019 at 08:12:16AM +0900, Michael Paquier wrote:
> Thanks for confirming, I am going to patch 9.4~9.6 with that.
And done.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20190424000726.GC1878@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 22:32 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 22:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 23:36 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-23 01:01 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
@ 2019-04-23 01:26 ` Jonathan S. Katz <jkatz@postgresql.org>
2019-04-23 01:28 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 27+ messages in thread
From: Jonathan S. Katz @ 2019-04-23 01:26 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs@lists.postgresql.org, Stephen Frost <sfrost@snowman.net>
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../9ce16763-836c-041e-06aa-af3e955444cd@postgresql.org/2-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Possible to store invalid SCRAM-SHA-256 Passwords
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-22 13:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 22:32 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 22:42 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 23:36 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-23 01:01 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Michael Paquier <michael@paquier.xyz>
2019-04-23 01:26 ` Re: Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
@ 2019-04-23 01:28 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 27+ messages in thread
From: Tom Lane @ 2019-04-23 01:28 UTC (permalink / raw)
To: Jonathan S. Katz <jkatz@postgresql.org>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-bugs@lists.postgresql.org, Stephen Frost <sfrost@snowman.net>
"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
^ permalink raw reply [nested|flat] 27+ messages in thread
end of thread, other threads:[~2019-04-24 00:07 UTC | newest]
Thread overview: 27+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-04-20 19:26 Possible to store invalid SCRAM-SHA-256 Passwords Jonathan S. Katz <jkatz@postgresql.org>
2019-04-20 20:12 ` Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 01:50 ` Michael Paquier <michael@paquier.xyz>
2019-04-22 13:04 ` Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 20:57 ` Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 13:42 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 13:52 ` Stephen Frost <sfrost@snowman.net>
2019-04-22 21:49 ` raf@raf.org
2019-04-23 14:42 ` Stephen Frost <sfrost@snowman.net>
2019-04-23 14:55 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-04-23 00:06 ` Michael Paquier <michael@paquier.xyz>
2019-04-23 14:43 ` Stephen Frost <sfrost@snowman.net>
2019-04-22 14:20 ` Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 22:32 ` Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 22:42 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-04-22 23:36 ` Jonathan S. Katz <jkatz@postgresql.org>
2019-04-22 23:55 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-04-23 01:01 ` Michael Paquier <michael@paquier.xyz>
2019-04-23 01:10 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-04-23 01:16 ` Jonathan S. Katz <jkatz@postgresql.org>
2019-04-23 02:10 ` Michael Paquier <michael@paquier.xyz>
2019-04-23 06:57 ` Michael Paquier <michael@paquier.xyz>
2019-04-23 14:19 ` Jonathan S. Katz <jkatz@postgresql.org>
2019-04-23 23:12 ` Michael Paquier <michael@paquier.xyz>
2019-04-24 00:07 ` Michael Paquier <michael@paquier.xyz>
2019-04-23 01:26 ` Jonathan S. Katz <jkatz@postgresql.org>
2019-04-23 01:28 ` Tom Lane <tgl@sss.pgh.pa.us>
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