[PATCH v2] auth: fix empty password authentication

[PATCH v2] auth: fix empty password authentication

[Vladimir Davydov]

We are supposed to authenticate guest user without a password. This
used to work before commit 076a842011e0 ("Permit empty passwords in
net.box"), when guest didn't have any password. Now it has an empty
password and the check in authenticate turns out to be broken, which
breaks assumptions made by certain connectors. This patch fixes the
check.

Closes #4327
---
https://github.com/tarantool/tarantool/issues/4327
https://github.com/tarantool/tarantool/tree/dv/gh-4327-fix-empty-password-auth

Changes in v2:
 - Don't change the way net.box treats absense of a password.
   Just fix the issue in question.

v1: https://www.freelists.org/post/tarantool-patches/PATCH-auth-fix-empty-password-authentication

 src/box/authentication.cc | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/src/box/authentication.cc b/src/box/authentication.cc
index 811974cb..b0459a5b 100644
--- a/src/box/authentication.cc
+++ b/src/box/authentication.cc
@@ -33,8 +33,13 @@
 #include "session.h"
 #include "msgpuck.h"
 #include "error.h"
+#include "third_party/base64.h"
 
-static char zero_hash[SCRAMBLE_SIZE];
+/**
+ * chap-sha1 of empty string, i.e.
+ * base64_encode(sha1(sha1(""), 0)
+ */
+static const char *CHAP_SHA1_EMPTY_PASSWORD = "vhvewKp0tNyweZQ+cFKAlsyphfg=";
 
 void
 authenticate(const char *user_name, uint32_t len, const char *salt,
@@ -52,10 +57,14 @@ authenticate(const char *user_name, uint32_t len, const char *salt,
 	 * pooling.
 	 */
 	part_count = mp_decode_array(&tuple);
-	if (part_count == 0 && user->def->uid == GUEST &&
-	    memcmp(user->def->hash2, zero_hash, SCRAMBLE_SIZE) == 0) {
-		/* No password is set for GUEST, OK. */
-		goto ok;
+	if (part_count == 0 && user->def->uid == GUEST) {
+		char hash2[SCRAMBLE_SIZE];
+		base64_decode(CHAP_SHA1_EMPTY_PASSWORD, SCRAMBLE_BASE64_SIZE,
+			      hash2, SCRAMBLE_SIZE);
+		if (memcmp(user->def->hash2, hash2, SCRAMBLE_SIZE) == 0) {
+			/* Empty password is set, OK. */
+			goto ok;
+		}
 	}
 
 	access_check_session_xc(user);
@@ -90,11 +99,11 @@ authenticate(const char *user_name, uint32_t len, const char *salt,
 			diag_raise();
 		tnt_raise(ClientError, ER_PASSWORD_MISMATCH, user->def->name);
 	}
+ok:
 	/* check and run auth triggers on success */
 	if (! rlist_empty(&session_on_auth) &&
 	    session_run_on_auth_triggers(&auth_res) != 0)
 		diag_raise();
-ok:
 	credentials_init(&session->credentials, user->auth_token,
 			 user->def->uid);
 }
-- 
2.11.0

Re: [PATCH v2] auth: fix empty password authentication

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/15 19:27]:
> We are supposed to authenticate guest user without a password. This
> used to work before commit 076a842011e0 ("Permit empty passwords in
> net.box"), when guest didn't have any password. Now it has an empty
> password and the check in authenticate turns out to be broken, which
> breaks assumptions made by certain connectors. This patch fixes the
> check.
> 
> Closes #4327
> ---
> https://github.com/tarantool/tarantool/issues/4327
> https://github.com/tarantool/tarantool/tree/dv/gh-4327-fix-empty-password-auth
> 
> Changes in v2:
>  - Don't change the way net.box treats absense of a password.
>    Just fix the issue in question.
> 
> v1: https://www.freelists.org/post/tarantool-patches/PATCH-auth-fix-empty-password-authentication
> 
>  src/box/authentication.cc | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/src/box/authentication.cc b/src/box/authentication.cc
> index 811974cb..b0459a5b 100644
> --- a/src/box/authentication.cc
> +++ b/src/box/authentication.cc
> @@ -33,8 +33,13 @@
>  #include "session.h"
>  #include "msgpuck.h"
>  #include "error.h"
> +#include "third_party/base64.h"
>  
> -static char zero_hash[SCRAMBLE_SIZE];
> +/**
> + * chap-sha1 of empty string, i.e.
> + * base64_encode(sha1(sha1(""), 0)
> + */
> +static const char *CHAP_SHA1_EMPTY_PASSWORD = "vhvewKp0tNyweZQ+cFKAlsyphfg=";

Do you insist on copy-pasting it from alter.cc?



>  
>  void
>  authenticate(const char *user_name, uint32_t len, const char *salt,
> @@ -52,10 +57,14 @@ authenticate(const char *user_name, uint32_t len, const char *salt,
>  	 * pooling.
>  	 */
>  	part_count = mp_decode_array(&tuple);
> -	if (part_count == 0 && user->def->uid == GUEST &&
> -	    memcmp(user->def->hash2, zero_hash, SCRAMBLE_SIZE) == 0) {
> -		/* No password is set for GUEST, OK. */
> -		goto ok;
> +	if (part_count == 0 && user->def->uid == GUEST) {
> +		char hash2[SCRAMBLE_SIZE];
> +		base64_decode(CHAP_SHA1_EMPTY_PASSWORD, SCRAMBLE_BASE64_SIZE,
> +			      hash2, SCRAMBLE_SIZE);
> +		if (memcmp(user->def->hash2, hash2, SCRAMBLE_SIZE) == 0) {
> +			/* Empty password is set, OK. */
> +			goto ok;
> +		}

I think we're still misunderstanding each other. Both zero hash
and empty string should work. We should become more permissive,
not less.


-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH v2] auth: fix empty password authentication

[Vladimir Davydov]

On Mon, Jul 15, 2019 at 10:52:05PM +0300, Konstantin Osipov wrote:
> > +/**
> > + * chap-sha1 of empty string, i.e.
> > + * base64_encode(sha1(sha1(""), 0)
> > + */
> > +static const char *CHAP_SHA1_EMPTY_PASSWORD = "vhvewKp0tNyweZQ+cFKAlsyphfg=";
> 
> Do you insist on copy-pasting it from alter.cc?

Thought you wouldn't notice :-)

> >  void
> >  authenticate(const char *user_name, uint32_t len, const char *salt,
> > @@ -52,10 +57,14 @@ authenticate(const char *user_name, uint32_t len, const char *salt,
> >  	 * pooling.
> >  	 */
> >  	part_count = mp_decode_array(&tuple);
> > -	if (part_count == 0 && user->def->uid == GUEST &&
> > -	    memcmp(user->def->hash2, zero_hash, SCRAMBLE_SIZE) == 0) {
> > -		/* No password is set for GUEST, OK. */
> > -		goto ok;
> > +	if (part_count == 0 && user->def->uid == GUEST) {
> > +		char hash2[SCRAMBLE_SIZE];
> > +		base64_decode(CHAP_SHA1_EMPTY_PASSWORD, SCRAMBLE_BASE64_SIZE,
> > +			      hash2, SCRAMBLE_SIZE);
> > +		if (memcmp(user->def->hash2, hash2, SCRAMBLE_SIZE) == 0) {
> > +			/* Empty password is set, OK. */
> > +			goto ok;
> > +		}
> 
> I think we're still misunderstanding each other. Both zero hash
> and empty string should work. We should become more permissive,
> not less.

Sorry for misunderstanding. Fixed. See the new patch below and on
the branch.

From c185a3872dc2cd86a383e72d9544ec5b97a9dc8d Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Date: Wed, 17 Jul 2019 14:41:26 +0300
Subject: [PATCH] auth: fix empty password authentication

We are supposed to authenticate guest user without a password. This
used to work before commit 076a842011e0 ("Permit empty passwords in
net.box"), when guest didn't have any password. Now it has an empty
password and the check in authenticate turns out to be broken, which
breaks assumptions made by certain connectors. This patch fixes the
check.

Closes #4327

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 1dbfe6b2..f98a77a5 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -58,12 +58,6 @@
 #include "sequence.h"
 #include "sql.h"
 
-/**
- * chap-sha1 of empty string, i.e.
- * base64_encode(sha1(sha1(""), 0)
- */
-#define CHAP_SHA1_EMPTY_PASSWORD "vhvewKp0tNyweZQ+cFKAlsyphfg="
-
 /* {{{ Auxiliary functions and methods. */
 
 static void
diff --git a/src/box/authentication.cc b/src/box/authentication.cc
index 811974cb..fdad7395 100644
--- a/src/box/authentication.cc
+++ b/src/box/authentication.cc
@@ -33,6 +33,7 @@
 #include "session.h"
 #include "msgpuck.h"
 #include "error.h"
+#include "third_party/base64.h"
 
 static char zero_hash[SCRAMBLE_SIZE];
 
@@ -52,10 +53,14 @@ authenticate(const char *user_name, uint32_t len, const char *salt,
 	 * pooling.
 	 */
 	part_count = mp_decode_array(&tuple);
-	if (part_count == 0 && user->def->uid == GUEST &&
-	    memcmp(user->def->hash2, zero_hash, SCRAMBLE_SIZE) == 0) {
-		/* No password is set for GUEST, OK. */
-		goto ok;
+	if (part_count == 0 && user->def->uid == GUEST) {
+		if (memcmp(user->def->hash2, zero_hash, SCRAMBLE_SIZE) == 0)
+			goto ok; /* no password is set, OK */
+		char hash2[SCRAMBLE_SIZE];
+		base64_decode(CHAP_SHA1_EMPTY_PASSWORD, SCRAMBLE_BASE64_SIZE,
+			      hash2, SCRAMBLE_SIZE);
+		if (memcmp(user->def->hash2, hash2, SCRAMBLE_SIZE) == 0)
+			goto ok; /* empty password is set, OK */
 	}
 
 	access_check_session_xc(user);
@@ -90,11 +95,11 @@ authenticate(const char *user_name, uint32_t len, const char *salt,
 			diag_raise();
 		tnt_raise(ClientError, ER_PASSWORD_MISMATCH, user->def->name);
 	}
+ok:
 	/* check and run auth triggers on success */
 	if (! rlist_empty(&session_on_auth) &&
 	    session_run_on_auth_triggers(&auth_res) != 0)
 		diag_raise();
-ok:
 	credentials_init(&session->credentials, user->auth_token,
 			 user->def->uid);
 }
diff --git a/src/box/user_def.c b/src/box/user_def.c
index 7d783771..4d9821a2 100644
--- a/src/box/user_def.c
+++ b/src/box/user_def.c
@@ -29,6 +29,9 @@
  * SUCH DAMAGE.
  */
 #include "user_def.h"
+
+const char *CHAP_SHA1_EMPTY_PASSWORD = "vhvewKp0tNyweZQ+cFKAlsyphfg=";
+
 const char *
 priv_name(user_access_t access)
 {
diff --git a/src/box/user_def.h b/src/box/user_def.h
index 8bf31c2f..98097c39 100644
--- a/src/box/user_def.h
+++ b/src/box/user_def.h
@@ -39,6 +39,11 @@
 extern "C" {
 #endif /* defined(__cplusplus) */
 
+/**
+ * chap-sha1 of empty string, i.e. base64_encode(sha1(sha1(""), 0)
+ */
+extern const char *CHAP_SHA1_EMPTY_PASSWORD;
+
 typedef uint16_t user_access_t;
 /**
  * Effective session user. A cache of user data

Re: [PATCH v2] auth: fix empty password authentication

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/17 14:54]:

lgtm

-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH v2] auth: fix empty password authentication

[Vladimir Davydov]

On Wed, Jul 17, 2019 at 05:28:37PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/17 14:54]:
> 
> lgtm

Pushed to master, 2.1, and 1.10.