On 12/07/2016 08:39 AM, Michael Paquier wrote:
On Tue, Nov 29, 2016 at 1:36 PM, Michael Paquier
<michael.paqu...@gmail.com> wrote:
Nothing more will likely happen in this CF, so I have moved it to
2017-01 with the same status of "Needs Review".
Attached is a new set of patches using the new routines
pg_backend_random() and pg_strong_random() to handle the randomness in
SCRAM:
- 0001 refactors the SHA2 routines. pgcrypto uses raw files from
src/common when compiling with this patch. That works on any platform,
and this is the simplified version of upthread.
- 0002 adds base64 routines to src/common.
- 0003 does some refactoring regarding the password encryption in
ALTER/CREATE USER queries.
- 0004 adds the clause PASSWORD (val USING method) in CREATE/ALTER USER.
- 0005 is the code patch for SCRAM. Note that this switches pgcrypto
to link to libpgcommon as SHA2 routines are used by the backend.
- 0006 adds some regression tests for passwords.
- 0007 adds some TAP tests for authentication.
This is added to the upcoming CF.
I spent a little time reading through this once again. Steady progress,
did some small fixes:
* Rewrote the nonce generation. In the server-side, it first generated a
string of ascii-printable characters, then base64-encoded them, which is
superfluous. Also, avoid calling pg_strong_random() one byte at a time,
for performance reasons.
* Added a more sophisticated fallback implementation in libpq, for the
--disable-strong-random cases, similar to pg_backend_random().
* No need to disallow SCRAM with db_user_namespace. It doesn't include
the username in the salt like MD5 does.
Attached those here, as add-on patches to your latest patch set. I'll
continue reviewing, but a couple of things caught my eye that you may
want to jump on, in the meanwhile:
On error messages, the spec says:
o e: This attribute specifies an error that occurred during
authentication exchange. It is sent by the server in its final
message and can help diagnose the reason for the authentication
exchange failure. On failed authentication, the entire server-
final-message is OPTIONAL; specifically, a server implementation
MAY conclude the SASL exchange with a failure without sending the
server-final-message. This results in an application-level error
response without an extra round-trip. If the server-final-message
is sent on authentication failure, then the "e" attribute MUST be
included.
Note that it says that the server can send the error message with the e=
attribute, in the *final message*. It's not a valid response in the
earlier state, before sending server-first-message. I think we need to
change the INIT state handling in pg_be_scram_exchange() to not send e=
messages to the client. On an error at that state, it needs to just bail
out without a message. The spec allows that. We can always log the
detailed reason in the server log, anyway.
As Peter E pointed out earlier, the documentation is lacking, on how to
configure MD5 and/or SCRAM. If you put "scram" as the authentication
method in pg_hba.conf, what does it mean? If you have a line for both
"scram" and "md5" in pg_hba.conf, with the same database/user/hostname
combo, what does that mean? Answer: The first one takes effect, the
second one has no effect. Yet the example in the docs now has that,
which is nonsense :-). Hopefully we'll have some kind of a "both"
option, before the release, but in the meanwhile, we need describe how
this works now in the docs.
- Heikki
>From 4d3a59ae1cb5742499c71b0c1e048d30dcef6836 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Wed, 7 Dec 2016 15:24:55 +0200
Subject: [PATCH 08/11] Rewrite nonce generation.
In the server, the nonce was generated using only ASCII-printable
characters, and the result was base64-encoded. The base64 encoding is
pointless, if we use only ASCII-printable chars to begin with.
Calling pg_strong_random() can be somewhat expensive, as with the
/dev/urandom implementation, it has to open the device, read the bytes,
and close, on every call. So avoid calling it in a loop, generating only
one byte in each call.
I went back to using base64-encoding method of turning the raw bytes into
the final nonce. That was more convenient than writing something that
encodes to the whole ASCII-printable range. That means that we're not using
the whole range of chars allowed in the nonce, but I believe that doesn't
make any difference. (Both the frontend and backend will still accept the
full range from the other side of the connection).
---
src/backend/libpq/auth-scram.c | 52 ++++++++-----------------------
src/include/common/scram-common.h | 6 +++-
src/include/libpq/libpq-be.h | 2 --
src/interfaces/libpq/fe-auth-scram.c | 60 ++++++++++--------------------------
4 files changed, 34 insertions(+), 86 deletions(-)
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 55c5efa..cda663b 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -72,7 +72,7 @@ typedef struct
/* Server-side status fields */
char *server_first_message;
- char *server_nonce; /* base64-encoded */
+ char *server_nonce;
char *server_signature;
} scram_state;
@@ -115,7 +115,6 @@ static bool verify_client_proof(scram_state *state);
static bool verify_final_nonce(scram_state *state);
static bool parse_scram_verifier(const char *verifier, char **salt,
int *iterations, char **stored_key, char **server_key);
-static void generate_nonce(char *out, int len);
/*
* build_error_message
@@ -237,29 +236,6 @@ check_client_data(void *opaque, char **logdetail)
char *passwd;
TimestampTz vuntil = 0;
bool vuntil_null;
- int count = 0;
-
- /* compute the salt to use for computing responses */
- while (count < sizeof(MyProcPort->SASLSalt))
- {
- char byte;
-
- if (!pg_backend_random(&byte, 1))
- {
- *logdetail = psprintf(_("Could not generate random salt"));
- return SASL_OTHER_ERROR;
- }
-
- /*
- * Only ASCII printable characters, except commas are accepted in
- * the nonce.
- */
- if (byte < '!' || byte > '~' || byte == ',')
- continue;
-
- MyProcPort->SASLSalt[count] = byte;
- count++;
- }
/*
* Fetch details about role needed for password checks.
@@ -450,7 +426,7 @@ scram_build_verifier(const char *username, const char *password,
if (iterations <= 0)
iterations = SCRAM_ITERATIONS_DEFAULT;
- generate_nonce(salt, SCRAM_SALT_LEN);
+ pg_backend_random(salt, SCRAM_SALT_LEN);
encoded_salt = palloc(pg_b64_enc_len(SCRAM_SALT_LEN) + 1);
encoded_len = pg_b64_encode(salt, SCRAM_SALT_LEN, encoded_salt);
@@ -806,9 +782,6 @@ verify_client_proof(scram_state *state)
static char *
build_server_first_message(scram_state *state)
{
- char nonce[SCRAM_NONCE_LEN];
- int encoded_len;
-
/*
* server-first-message =
* [reserved-mext ","] nonce "," salt ","
@@ -830,10 +803,19 @@ build_server_first_message(scram_state *state)
*
* r=fyko+d2lbbFgONRv9qkxdawL3rfcNHYJY1ZVvWVs7j,s=QSXCR+Q6sek8bf92,i=4096
*/
- generate_nonce(nonce, SCRAM_NONCE_LEN);
+
+ /*
+ * Per the spec, the nonce may consist of any printable ASCII characters.
+ * For convenience, however, we don't use the whole range available, rather,
+ * we generate some random bytes, and base64 encode them.
+ */
+ char raw_nonce[SCRAM_NONCE_LEN];
+ int encoded_len;
+
+ pg_backend_random(raw_nonce, SCRAM_NONCE_LEN);
state->server_nonce = palloc(pg_b64_enc_len(SCRAM_NONCE_LEN) + 1);
- encoded_len = pg_b64_encode(nonce, SCRAM_NONCE_LEN, state->server_nonce);
+ encoded_len = pg_b64_encode(raw_nonce, SCRAM_NONCE_LEN, state->server_nonce);
if (encoded_len < 0)
return NULL;
@@ -964,11 +946,3 @@ build_server_final_message(scram_state *state)
*/
return psprintf("v=%s", server_signature_base64);
}
-
-static void
-generate_nonce(char *result, int len)
-{
- /* Use the salt generated for SASL authentication */
- memset(result, 0, len);
- memcpy(result, MyProcPort->SASLSalt, Min(sizeof(MyProcPort->SASLSalt), len));
-}
diff --git a/src/include/common/scram-common.h b/src/include/common/scram-common.h
index e9028fb..34c6527 100644
--- a/src/include/common/scram-common.h
+++ b/src/include/common/scram-common.h
@@ -21,7 +21,11 @@
/* length of HMAC */
#define SHA256_HMAC_B PG_SHA256_BLOCK_LENGTH
-/* length of random nonce generated in the authentication exchange */
+/*
+ * Size of random nonce generated in the authentication exchange. This is
+ * in "raw" number of bytes, the actual nonces sent over the wire are
+ * encoded using only ASCII-printable characters.
+ */
#define SCRAM_NONCE_LEN 10
/* length of salt when generating new verifiers */
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 299aaca..66647ad 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -144,8 +144,6 @@ typedef struct Port
* Information that needs to be held during the authentication cycle.
*/
HbaLine *hba;
- char SASLSalt[10]; /* SASL password salt, size of
- * SCRAM_SALT_LEN */
/*
* Information that really has no business at all being in struct Port,
diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c
index a1dc7f1..12884e5 100644
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -61,7 +61,6 @@ static char *build_client_first_message(fe_scram_state *state,
static char *build_client_final_message(fe_scram_state *state,
PQExpBuffer errormessage);
static bool verify_server_proof(fe_scram_state *state);
-static bool generate_nonce(char *buf, int len);
static void calculate_client_proof(fe_scram_state *state,
const char *client_final_message_without_proof,
uint8 *result);
@@ -257,25 +256,35 @@ read_attr_value(char **input, char attr, PQExpBuffer errorMessage)
static char *
build_client_first_message(fe_scram_state *state, PQExpBuffer errormessage)
{
- char nonce[SCRAM_NONCE_LEN + 1];
+ char raw_nonce[SCRAM_NONCE_LEN + 1];
char *buf;
- char msglen;
+ char buflen;
+ int n;
+ int encoded_len;
- if (!generate_nonce(nonce, SCRAM_NONCE_LEN))
+ /* XXX: Check max length of username? */
+
+ /*
+ * Generate a "raw" nonce. This is converted to ASCII-printable form by
+ * base64-encoding it.
+ */
+ if (!pg_strong_random(raw_nonce, SCRAM_NONCE_LEN))
{
printfPQExpBuffer(errormessage, libpq_gettext("failed to generate nonce\n"));
return NULL;
}
/* Generate message */
- msglen = 5 + strlen(state->username) + 3 + strlen(nonce);
- buf = malloc(msglen + 1);
+ buflen = 5 + strlen(state->username) + 3 + pg_b64_enc_len(SCRAM_NONCE_LEN) + 1;
+ buf = malloc(buflen);
if (buf == NULL)
{
printfPQExpBuffer(errormessage, libpq_gettext("out of memory\n"));
return NULL;
}
- snprintf(buf, msglen + 1, "n,,n=%s,r=%s", state->username, nonce);
+ n = snprintf(buf, buflen, "n,,n=%s,r=", state->username);
+ encoded_len = pg_b64_encode(raw_nonce, SCRAM_NONCE_LEN, buf + n);
+ buf[n + encoded_len] = '\0';
state->client_first_message_bare = strdup(buf + 3);
if (!state->client_first_message_bare)
@@ -527,40 +536,3 @@ verify_server_proof(fe_scram_state *state)
return true;
}
-
-/*
- * Generate nonce with some randomness.
- * Returns true of nonce has been succesfully generated, and false
- * otherwise.
- */
-static bool
-generate_nonce(char *buf, int len)
-{
- int count = 0;
-
- /* compute the salt to use for computing responses */
- while (count < len)
- {
- char byte;
-
-#ifdef HAVE_STRONG_RANDOM
- if (!pg_strong_random(&byte, 1))
- return false;
-#else
- byte = random() % 256;
-#endif
-
- /*
- * Only ASCII printable characters, except commas are accepted in
- * the nonce.
- */
- if (byte < '!' || byte > '~' || byte == ',')
- continue;
-
- buf[count] = byte;
- count++;
- }
-
- buf[len] = '\0';
- return true;
-}
--
2.10.2
>From d969c8c48febae6ea27035fceccc160778ebb093 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Wed, 7 Dec 2016 17:12:52 +0200
Subject: [PATCH 09/11] Random number fixes.
* Check return value of pg_backend_random() and pg_strong_random()
* Add a fallback implementation for libpq, for !HAVE_STRONG_RANDOM
* Rename SCRAM_NONCE_LEN to SCRAM_RAW_NONCE_LEN, for clarity (unrelated to
the other changes in this commit, really)
---
src/backend/libpq/auth-scram.c | 22 ++++++++++---
src/include/common/scram-common.h | 4 +--
src/interfaces/libpq/Makefile | 13 ++++++--
src/interfaces/libpq/fe-auth-scram.c | 62 +++++++++++++++++++++++++++++++++---
4 files changed, 87 insertions(+), 14 deletions(-)
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index cda663b..6b129af 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -426,7 +426,13 @@ scram_build_verifier(const char *username, const char *password,
if (iterations <= 0)
iterations = SCRAM_ITERATIONS_DEFAULT;
- pg_backend_random(salt, SCRAM_SALT_LEN);
+ if (!pg_backend_random(salt, SCRAM_SALT_LEN))
+ {
+ ereport(LOG,
+ (errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg("could not generate random salt")));
+ return NULL;
+ }
encoded_salt = palloc(pg_b64_enc_len(SCRAM_SALT_LEN) + 1);
encoded_len = pg_b64_encode(salt, SCRAM_SALT_LEN, encoded_salt);
@@ -809,13 +815,19 @@ build_server_first_message(scram_state *state)
* For convenience, however, we don't use the whole range available, rather,
* we generate some random bytes, and base64 encode them.
*/
- char raw_nonce[SCRAM_NONCE_LEN];
+ char raw_nonce[SCRAM_RAW_NONCE_LEN];
int encoded_len;
- pg_backend_random(raw_nonce, SCRAM_NONCE_LEN);
+ if (!pg_backend_random(raw_nonce, SCRAM_RAW_NONCE_LEN))
+ {
+ ereport(LOG,
+ (errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg("could not generate random nonce")));
+ return NULL;
+ }
- state->server_nonce = palloc(pg_b64_enc_len(SCRAM_NONCE_LEN) + 1);
- encoded_len = pg_b64_encode(raw_nonce, SCRAM_NONCE_LEN, state->server_nonce);
+ state->server_nonce = palloc(pg_b64_enc_len(SCRAM_RAW_NONCE_LEN) + 1);
+ encoded_len = pg_b64_encode(raw_nonce, SCRAM_RAW_NONCE_LEN, state->server_nonce);
if (encoded_len < 0)
return NULL;
diff --git a/src/include/common/scram-common.h b/src/include/common/scram-common.h
index 34c6527..024059b 100644
--- a/src/include/common/scram-common.h
+++ b/src/include/common/scram-common.h
@@ -26,10 +26,10 @@
* in "raw" number of bytes, the actual nonces sent over the wire are
* encoded using only ASCII-printable characters.
*/
-#define SCRAM_NONCE_LEN 10
+#define SCRAM_RAW_NONCE_LEN 10
/* length of salt when generating new verifiers */
-#define SCRAM_SALT_LEN SCRAM_NONCE_LEN
+#define SCRAM_SALT_LEN 10
/* number of bytes used when sending iteration number during exchange */
#define SCRAM_ITERATION_LEN 10
diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile
index 460b0a1..4599b03 100644
--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -35,10 +35,17 @@ OBJS= fe-auth.o fe-auth-scram.o fe-connect.o fe-exec.o fe-misc.o fe-print.o fe-l
fe-protocol2.o fe-protocol3.o pqexpbuffer.o fe-secure.o \
libpq-events.o
# libpgport C files we always use
-OBJS += chklocale.o inet_net_ntop.o noblock.o pg_strong_random.o \
- pgstrcasecmp.o pqsignal.o thread.o
+OBJS += chklocale.o inet_net_ntop.o noblock.o pgstrcasecmp.o pqsignal.o \
+ thread.o
# libpgport C files that are needed if identified by configure
OBJS += $(filter crypt.o getaddrinfo.o getpeereid.o inet_aton.o open.o system.o snprintf.o strerror.o strlcpy.o win32error.o win32setlocale.o, $(LIBOBJS))
+
+ifeq ($(enable_strong_random), yes)
+OBJS += pg_strong_random.o
+else
+OBJS += erand48.o
+endif
+
# src/backend/utils/mb
OBJS += encnames.o wchar.o
# src/common
@@ -95,7 +102,7 @@ backend_src = $(top_srcdir)/src/backend
# For some libpgport modules, this only happens if configure decides
# the module is needed (see filter hack in OBJS, above).
-chklocale.c crypt.c getaddrinfo.c getpeereid.c inet_aton.c inet_net_ntop.c noblock.c open.c system.c pgsleep.c pg_strong_random.c pgstrcasecmp.c pqsignal.c snprintf.c strerror.c strlcpy.c thread.c win32error.c win32setlocale.c: % : $(top_srcdir)/src/port/%
+chklocale.c crypt.c erand48.c getaddrinfo.c getpeereid.c inet_aton.c inet_net_ntop.c noblock.c open.c system.c pgsleep.c pg_strong_random.c pgstrcasecmp.c pqsignal.c snprintf.c strerror.c strlcpy.c thread.c win32error.c win32setlocale.c: % : $(top_srcdir)/src/port/%
rm -f $@ && $(LN_S) $< .
ip.c md5.c: % : $(top_srcdir)/src/common/%
diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c
index 12884e5..12943ff 100644
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -18,6 +18,12 @@
#include "common/scram-common.h"
#include "fe-auth.h"
+/* These are needed for getpid(), in the fallback implementation */
+#ifndef HAVE_STRONG_RANDOM
+#include <sys/types.h>
+#include <unistd.h>
+#endif
+
/*
* Status of exchange messages used for SCRAM authentication via the
* SASL protocol.
@@ -250,13 +256,61 @@ read_attr_value(char **input, char attr, PQExpBuffer errorMessage)
return begin;
}
+static bool
+pg_frontend_random(char *dst, int len)
+{
+#ifdef HAVE_STRONG_RANDOM
+ return pg_strong_random(dst, len);
+#else
+ int i;
+ char *end = dst + len;
+
+ static unsigned short seed[3];
+ static int mypid = 0;
+
+ pglock_thread();
+
+ if (mypid != getpid())
+ {
+ struct timeval now;
+
+ gettimeofday(&now, NULL);
+
+ seed[0] = now.tv_sec ^ getpid();
+ seed[1] = (unsigned short) (now.tv_usec);
+ seed[2] = (unsigned short) (now.tv_usec >> 16);
+ }
+
+ for (i = 0; dst < end; i++)
+ {
+ uint32 r;
+ int j;
+
+ /*
+ * pg_jrand48 returns a 32-bit integer. Fill the next 4 bytes from it.
+ */
+ r = (uint32) pg_jrand48(seed);
+
+ for (j = 0; j < 4 && dst < end; j++)
+ {
+ *(dst++) = (char) (r & 0xFF);
+ r >>= 8;
+ }
+ }
+
+ pgunlock_thread();
+
+ return true;
+#endif
+}
+
/*
* Build the first exchange message sent by the client.
*/
static char *
build_client_first_message(fe_scram_state *state, PQExpBuffer errormessage)
{
- char raw_nonce[SCRAM_NONCE_LEN + 1];
+ char raw_nonce[SCRAM_RAW_NONCE_LEN + 1];
char *buf;
char buflen;
int n;
@@ -268,14 +322,14 @@ build_client_first_message(fe_scram_state *state, PQExpBuffer errormessage)
* Generate a "raw" nonce. This is converted to ASCII-printable form by
* base64-encoding it.
*/
- if (!pg_strong_random(raw_nonce, SCRAM_NONCE_LEN))
+ if (!pg_frontend_random(raw_nonce, SCRAM_RAW_NONCE_LEN))
{
printfPQExpBuffer(errormessage, libpq_gettext("failed to generate nonce\n"));
return NULL;
}
/* Generate message */
- buflen = 5 + strlen(state->username) + 3 + pg_b64_enc_len(SCRAM_NONCE_LEN) + 1;
+ buflen = 5 + strlen(state->username) + 3 + pg_b64_enc_len(SCRAM_RAW_NONCE_LEN) + 1;
buf = malloc(buflen);
if (buf == NULL)
{
@@ -283,7 +337,7 @@ build_client_first_message(fe_scram_state *state, PQExpBuffer errormessage)
return NULL;
}
n = snprintf(buf, buflen, "n,,n=%s,r=", state->username);
- encoded_len = pg_b64_encode(raw_nonce, SCRAM_NONCE_LEN, buf + n);
+ encoded_len = pg_b64_encode(raw_nonce, SCRAM_RAW_NONCE_LEN, buf + n);
buf[n + encoded_len] = '\0';
state->client_first_message_bare = strdup(buf + 3);
--
2.10.2
>From 7d2a08dd94a96d37aa5ac821fbb963ee03798f0b Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Wed, 7 Dec 2016 17:13:58 +0200
Subject: [PATCH 10/11] No need to disallow SCRAM with db_user_namespace.
---
src/backend/libpq/hba.c | 13 -------------
1 file changed, 13 deletions(-)
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 6fe79d7..f1e9a38 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -1183,19 +1183,6 @@ parse_hba_line(List *line, int line_num, char *raw_line)
}
parsedline->auth_method = uaMD5;
}
- else if (strcmp(token->string, "scram") == 0)
- {
- if (Db_user_namespace)
- {
- ereport(LOG,
- (errcode(ERRCODE_CONFIG_FILE_ERROR),
- errmsg("SCRAM authentication is not supported when \"db_user_namespace\" is enabled"),
- errcontext("line %d of configuration file \"%s\"",
- line_num, HbaFileName)));
- return NULL;
- }
- parsedline->auth_method = uaSASL;
- }
else if (strcmp(token->string, "pam") == 0)
#ifdef USE_PAM
parsedline->auth_method = uaPAM;
--
2.10.2
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
