From e5aa843ec71cae7fba216dbb499cf44357bbdd2b Mon Sep 17 00:00:00 2001
From: "Roger A. Light" <roger@atchoo.org>
Date: Sun, 13 Jul 2014 21:20:18 +0100
Subject: [PATCH] Don't allow access to clients when authenticating if a
 security plugin returns an application error. Fixes bug #1340782.

Thanks to Charlie Davis.
---
 ChangeLog.txt            |  2 ++
 src/bridge.c             | 16 ++++++++++++----
 src/read_handle_server.c | 32 +++++++++++++++++++++++---------
 src/subs.c               |  4 ++--
 4 files changed, 39 insertions(+), 15 deletions(-)

diff --git a/ChangeLog.txt b/ChangeLog.txt
index bf07cb2a..312b3194 100644
--- a/ChangeLog.txt
+++ b/ChangeLog.txt
@@ -10,6 +10,8 @@ Clients:
 ================
 
 Broker:
+- Don't allow access to clients when authenticating if a security plugin
+  returns an application error. Fixes bug #1340782.
 - Ensure that bridges verify certificates by default when using TLS.
 - Fix possible crash when using pattern ACLs that do not include a %u and
   clients that connect without a username.
diff --git a/src/bridge.c b/src/bridge.c
index 83048a2f..095a15ea 100644
--- a/src/bridge.c
+++ b/src/bridge.c
@@ -163,11 +163,19 @@ int mqtt3_bridge_connect(struct mosquitto_db *db, struct mosquitto *context)
 	}
 
 	rc = mosquitto_unpwd_check(db, context->bridge->local_username, context->bridge->local_password);
-	if(rc == MOSQ_ERR_AUTH){
-		_mosquitto_log_printf(NULL, MOSQ_LOG_ERR, "Bridge %s failed authentication on local broker.", context->id);
-		return rc;
+	switch(rc){
+		case MOSQ_ERR_SUCCESS:
+			break;
+		case MOSQ_ERR_AUTH:
+			_mosquitto_log_printf(NULL, MOSQ_LOG_ERR, "Bridge %s failed authentication on local broker.", context->id);
+			return rc;
+		case MOSQ_ERR_UNKNOWN:
+			_mosquitto_log_printf(NULL, MOSQ_LOG_ERR, "Bridge %s returned application error in authorisation.", context->id);
+			return rc;
+		default:
+			_mosquitto_log_printf(NULL, MOSQ_LOG_ERR, "Unknown error in authentication for bridge %s.", context->id);
+			return rc;
 	}
-	rc = 0;
 
 	/* Delete all local subscriptions even for clean_session==false. We don't
 	 * remove any messages and the next loop carries out the resubscription
diff --git a/src/read_handle_server.c b/src/read_handle_server.c
index 5488ac67..fc491d3a 100644
--- a/src/read_handle_server.c
+++ b/src/read_handle_server.c
@@ -344,13 +344,20 @@ int mqtt3_handle_connect(struct mosquitto_db *db, struct mosquitto *context)
 #endif /* WITH_TLS */
 		if(username_flag){
 			rc = mosquitto_unpwd_check(db, username, password);
-			if(rc == MOSQ_ERR_AUTH){
-				_mosquitto_send_connack(context, CONNACK_REFUSED_BAD_USERNAME_PASSWORD);
-				mqtt3_context_disconnect(db, context);
-				rc = MOSQ_ERR_SUCCESS;
-				goto handle_connect_error;
-			}else if(rc == MOSQ_ERR_INVAL){
-				goto handle_connect_error;
+			switch(rc){
+				case MOSQ_ERR_SUCCESS:
+					break;
+				case MOSQ_ERR_AUTH:
+					_mosquitto_send_connack(context, CONNACK_REFUSED_BAD_USERNAME_PASSWORD);
+					mqtt3_context_disconnect(db, context);
+					rc = MOSQ_ERR_SUCCESS;
+					goto handle_connect_error;
+					break;
+				default:
+					mqtt3_context_disconnect(db, context);
+					rc = MOSQ_ERR_SUCCESS;
+					goto handle_connect_error;
+					break;
 			}
 			context->username = username;
 			context->password = password;
@@ -629,8 +636,15 @@ int mqtt3_handle_subscribe(struct mosquitto_db *db, struct mosquitto *context)
 
 			if(context->protocol == mosq_p_mqtt311){
 				rc = mosquitto_acl_check(db, context, sub, MOSQ_ACL_READ);
-				if(rc == MOSQ_ERR_ACL_DENIED){
-					qos = 0x80;
+				switch(rc){
+					case MOSQ_ERR_SUCCESS:
+						break;
+					case MOSQ_ERR_ACL_DENIED:
+						qos = 0x80;
+						break;
+					default:
+						_mosquitto_free(sub);
+						return rc;
 				}
 			}
 
diff --git a/src/subs.c b/src/subs.c
index 6194059f..991d2856 100644
--- a/src/subs.c
+++ b/src/subs.c
@@ -92,7 +92,7 @@ static int _subs_process(struct mosquitto_db *db, struct _mosquitto_subhier *hie
 			hier->retained = NULL;
 		}
 	}
-	while(source_id && leaf){
+	while(source_id && leaf && ){
 		if(leaf->context->is_bridge && !strcmp(leaf->context->id, source_id)){
 			leaf = leaf->next;
 			continue;
@@ -131,7 +131,7 @@ static int _subs_process(struct mosquitto_db *db, struct _mosquitto_subhier *hie
 			}
 			if(mqtt3_db_message_insert(db, leaf->context, mid, mosq_md_out, msg_qos, client_retain, stored) == 1) rc = 1;
 		}else{
-			rc = 1;
+			return 1; /* Application error */
 		}
 		leaf = leaf->next;
 	}