From: Michael Paquier <michael@paquier.xyz>
To: Jonathan S. Katz <jkatz@postgresql.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs@lists.postgresql.org, Stephen Frost <sfrost@snowman.net>
Subject: Re: Possible to store invalid SCRAM-SHA-256 Passwords
Date: Tue, 23 Apr 2019 10:01:42 +0900
Message-ID: <20190423010142.GE2712@paquier.xyz> (raw)
In-Reply-To: <15140775-08c3-45bd-6dab-fdcd290d1bf0@postgresql.org>
References: <016deb6b-1f0a-8e9f-1833-a8675b170aa9@postgresql.org>
<4240a41f-a957-fd7d-7c24-31683f259b9d@postgresql.org>
<20190422015051.GA3433@paquier.xyz>
<3033.1555940554@sss.pgh.pa.us>
<5a93333c-dde6-ee52-f870-4dccfc50eb6f@postgresql.org>
<30284.1555972921@sss.pgh.pa.us>
<15140775-08c3-45bd-6dab-fdcd290d1bf0@postgresql.org>
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.cindex 3cbe902ace..4c7b149570 100644--- a/src/backend/libpq/auth-scram.c+++ b/src/backend/libpq/auth-scram.c@@ -161,8 +161,6 @@ static char *build_server_first_message(scram_state *state);
static char *build_server_final_message(scram_state *state);
static bool verify_client_proof(scram_state *state);
static bool verify_final_nonce(scram_state *state);
-static bool parse_scram_verifier(const char *verifier, int *iterations,- char **salt, uint8 *stored_key, uint8 *server_key);
static void mock_scram_verifier(const char *username, int *iterations,
char **salt, uint8 *stored_key, uint8 *server_key);
static bool is_scram_printable(char *p);
@@ -546,7 +544,7 @@ scram_verify_plain_password(const char *username, const char *password,
*
* Returns true if the SCRAM verifier has been parsed, and false otherwise.
*/
-static bool+bool
parse_scram_verifier(const char *verifier, int *iterations, char **salt,
uint8 *stored_key, uint8 *server_key)
{
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.cindex c34e4a3d85..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.hindex 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.hindex d7f4c094c9..bc297e2a33 100644--- a/src/include/libpq/scram.h+++ b/src/include/libpq/scram.h@@ -29,6 +29,8 @@ extern int pg_be_scram_exchange(void *opaq, const char *input, int inputlen,
/* Routines to handle and check SCRAM-SHA-256 verifier */
extern char *pg_be_scram_build_verifier(const char *password);
+extern bool parse_scram_verifier(const char *verifier, int *iterations, char **salt,+ uint8 *stored_key, uint8 *server_key);
extern bool scram_verify_plain_password(const char *username,
const char *password, const char *verifier);
diff --git a/src/test/regress/expected/password.out b/src/test/regress/expected/password.outindex 393d836ead..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.sqlindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: michael@paquier.xyz, jkatz@postgresql.org, tgl@sss.pgh.pa.us, sfrost@snowman.net
Subject: Re: Possible to store invalid SCRAM-SHA-256 Passwords
In-Reply-To: <20190423010142.GE2712@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox