Improve password file parsing in the broker and mosqitto_passwd.

Closes #1584. Thanks to panava.

ChangeLog.txt

@@ -10,6 +10,7 @@ Broker:
 - Fix config->user not being freed on exit. Closes #1564.
 - Fix trailing whitespace not being trimmed on acl users. Closes #1539.
 - Fix `bind_interface` not working for the default listener. Closes #1533.
+- Improve password file parsing in the broker and mosqitto_passwd. Closes #1584.
 
 Library:
 - Set minimum keepalive argument to `mosquitto_connect*()` to be 5 seconds.

lib/CMakeLists.txt

@@ -25,6 +25,7 @@ set(C_SRC
 	loop.c
 	memory_mosq.c memory_mosq.h
 	messages_mosq.c messages_mosq.h
+	misc_mosq.c misc_mosq.h
 	mosquitto.c mosquitto.h
 	mosquitto_internal.h
 	mqtt_protocol.h

lib/Makefile

@@ -21,6 +21,7 @@ MOSQ_OBJS=mosquitto.o \
 		  loop.o \
 		  memory_mosq.o \
 		  messages_mosq.o \
+		  misc_mosq.o \
 		  net_mosq_ocsp.o \
 		  net_mosq.o \
 		  options.o \
@@ -153,6 +154,9 @@ messages_mosq.o : messages_mosq.c messages_mosq.h
 memory_mosq.o : memory_mosq.c memory_mosq.h
 	${CROSS_COMPILE}$(CC) $(LIB_CPPFLAGS) $(LIB_CFLAGS) -c $< -o $@
 
+misc_mosq.o : misc_mosq.c misc_mosq.h
+	${CROSS_COMPILE}$(CC) $(LIB_CPPFLAGS) $(LIB_CFLAGS) -c $< -o $@
+
 net_mosq_ocsp.o : net_mosq_ocsp.c net_mosq.h
 	${CROSS_COMPILE}$(CC) $(LIB_CPPFLAGS) $(LIB_CFLAGS) -c $< -o $@
 

lib/misc_mosq.c

@@ -0,0 +1,177 @@
+/*
+Copyright (c) 2009-2019 Roger Light <roger@atchoo.org>
+
+All rights reserved. This program and the accompanying materials
+are made available under the terms of the Eclipse Public License v1.0
+and Eclipse Distribution License v1.0 which accompany this distribution.
+
+The Eclipse Public License is available at
+   http://www.eclipse.org/legal/epl-v10.html
+and the Eclipse Distribution License is available at
+  http://www.eclipse.org/org/documents/edl-v10.php.
+
+Contributors:
+   Roger Light - initial implementation and documentation.
+*/
+
+/* This contains general purpose utility functions that are not specific to
+ * Mosquitto/MQTT features. */
+
+#include "config.h"
+
+#include <ctype.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#ifdef WIN32
+#  include <winsock2.h>
+#  include <aclapi.h>
+#  include <io.h>
+#  include <lmcons.h>
+#else
+#  include <sys/stat.h>
+#endif
+
+
+FILE *mosquitto__fopen(const char *path, const char *mode, bool restrict_read)
+{
+#ifdef WIN32
+	char buf[4096];
+	int rc;
+	rc = ExpandEnvironmentStrings(path, buf, 4096);
+	if(rc == 0 || rc > 4096){
+		return NULL;
+	}else{
+		if (restrict_read) {
+			HANDLE hfile;
+			SECURITY_ATTRIBUTES sec;
+			EXPLICIT_ACCESS ea;
+			PACL pacl = NULL;
+			char username[UNLEN + 1];
+			int ulen = UNLEN;
+			SECURITY_DESCRIPTOR sd;
+			DWORD dwCreationDisposition;
+
+			switch(mode[0]){
+				case 'a':
+					dwCreationDisposition = OPEN_ALWAYS;
+					break;
+				case 'r':
+					dwCreationDisposition = OPEN_EXISTING;
+					break;
+				case 'w':
+					dwCreationDisposition = CREATE_ALWAYS;
+					break;
+				default:
+					return NULL;
+			}
+
+			GetUserName(username, &ulen);
+			if (!InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION)) {
+				return NULL;
+			}
+			BuildExplicitAccessWithName(&ea, username, GENERIC_ALL, SET_ACCESS, NO_INHERITANCE);
+			if (SetEntriesInAcl(1, &ea, NULL, &pacl) != ERROR_SUCCESS) {
+				return NULL;
+			}
+			if (!SetSecurityDescriptorDacl(&sd, TRUE, pacl, FALSE)) {
+				LocalFree(pacl);
+				return NULL;
+			}
+
+			sec.nLength = sizeof(SECURITY_ATTRIBUTES);
+			sec.bInheritHandle = FALSE;
+			sec.lpSecurityDescriptor = &sd;
+
+			hfile = CreateFile(buf, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ,
+				&sec,
+				dwCreationDisposition,
+				FILE_ATTRIBUTE_NORMAL,
+				NULL);
+
+			LocalFree(pacl);
+
+			int fd = _open_osfhandle((intptr_t)hfile, 0);
+			if (fd < 0) {
+				return NULL;
+			}
+
+			FILE *fptr = _fdopen(fd, mode);
+			if (!fptr) {
+				_close(fd);
+				return NULL;
+			}
+			return fptr;
+
+		}else {
+			return fopen(buf, mode);
+		}
+	}
+#else
+	if (restrict_read) {
+		FILE *fptr;
+		mode_t old_mask;
+
+		old_mask = umask(0077);
+		fptr = fopen(path, mode);
+		umask(old_mask);
+
+		return fptr;
+	}else{
+		return fopen(path, mode);
+	}
+#endif
+}
+
+
+char *misc__trimblanks(char *str)
+{
+	char *endptr;
+
+	if(str == NULL) return NULL;
+
+	while(isspace(str[0])){
+		str++;
+	}
+	endptr = &str[strlen(str)-1];
+	while(endptr > str && isspace(endptr[0])){
+		endptr[0] = '\0';
+		endptr--;
+	}
+	return str;
+}
+
+
+char *fgets_extending(char **buf, int *buflen, FILE *stream)
+{
+	char *rc;
+	char endchar;
+	int offset = 0;
+	char *newbuf;
+
+	if(stream == NULL || buf == NULL || buflen == NULL || *buflen < 1){
+		return NULL;
+	}
+
+	do{
+		rc = fgets(&((*buf)[offset]), (*buflen)-offset, stream);
+		if(feof(stream)){
+			return rc;
+		}
+
+		endchar = (*buf)[strlen(*buf)-1];
+		if(endchar == '\n'){
+			return rc;
+		}
+		/* No EOL char found, so extend buffer */
+		offset = (*buflen)-1;
+		*buflen += 1000;
+		newbuf = realloc(*buf, *buflen);
+		if(!newbuf){
+			return NULL;
+		}
+		*buf = newbuf;
+	}while(1);
+}

lib/misc_mosq.h

@@ -0,0 +1,25 @@
+/*
+Copyright (c) 2009-2019 Roger Light <roger@atchoo.org>
+
+All rights reserved. This program and the accompanying materials
+are made available under the terms of the Eclipse Public License v1.0
+and Eclipse Distribution License v1.0 which accompany this distribution.
+ 
+The Eclipse Public License is available at
+   http://www.eclipse.org/legal/epl-v10.html
+and the Eclipse Distribution License is available at
+  http://www.eclipse.org/org/documents/edl-v10.php.
+ 
+Contributors:
+   Roger Light - initial implementation and documentation.
+*/
+#ifndef MISC_MOSQ_H
+#define MISC_MOSQ_H
+
+#include <stdbool.h>
+
+FILE *mosquitto__fopen(const char *path, const char *mode, bool restrict_read);
+char *misc__trimblanks(char *str);
+char *fgets_extending(char **buf, int *buflen, FILE *stream);
+
+#endif

lib/options.c

@@ -32,6 +32,7 @@ Contributors:
 #include "mosquitto.h"
 #include "mosquitto_internal.h"
 #include "memory_mosq.h"
+#include "misc_mosq.h"
 #include "mqtt_protocol.h"
 #include "util_mosq.h"
 #include "will_mosq.h"

lib/util_mosq.c

@@ -203,96 +203,6 @@ int mosquitto__hex2bin(const char *hex, unsigned char *bin, int bin_max_len)
 }
 #endif
 
-FILE *mosquitto__fopen(const char *path, const char *mode, bool restrict_read)
-{
-#ifdef WIN32
-	char buf[4096];
-	int rc;
-	rc = ExpandEnvironmentStrings(path, buf, 4096);
-	if(rc == 0 || rc > 4096){
-		return NULL;
-	}else{
-		if (restrict_read) {
-			HANDLE hfile;
-			SECURITY_ATTRIBUTES sec;
-			EXPLICIT_ACCESS ea;
-			PACL pacl = NULL;
-			char username[UNLEN + 1];
-			int ulen = UNLEN;
-			SECURITY_DESCRIPTOR sd;
-			DWORD dwCreationDisposition;
-
-			switch(mode[0]){
-				case 'a':
-					dwCreationDisposition = OPEN_ALWAYS;
-					break;
-				case 'r':
-					dwCreationDisposition = OPEN_EXISTING;
-					break;
-				case 'w':
-					dwCreationDisposition = CREATE_ALWAYS;
-					break;
-				default:
-					return NULL;
-			}
-
-			GetUserName(username, &ulen);
-			if (!InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION)) {
-				return NULL;
-			}
-			BuildExplicitAccessWithName(&ea, username, GENERIC_ALL, SET_ACCESS, NO_INHERITANCE);
-			if (SetEntriesInAcl(1, &ea, NULL, &pacl) != ERROR_SUCCESS) {
-				return NULL;
-			}
-			if (!SetSecurityDescriptorDacl(&sd, TRUE, pacl, FALSE)) {
-				LocalFree(pacl);
-				return NULL;
-			}
-
-			sec.nLength = sizeof(SECURITY_ATTRIBUTES);
-			sec.bInheritHandle = FALSE;
-			sec.lpSecurityDescriptor = &sd;
-
-			hfile = CreateFile(buf, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ,
-				&sec,
-				dwCreationDisposition,
-				FILE_ATTRIBUTE_NORMAL,
-				NULL);
-
-			LocalFree(pacl);
-
-			int fd = _open_osfhandle((intptr_t)hfile, 0);
-			if (fd < 0) {
-				return NULL;
-			}
-
-			FILE *fptr = _fdopen(fd, mode);
-			if (!fptr) {
-				_close(fd);
-				return NULL;
-			}
-			return fptr;
-
-		}else {
-			return fopen(buf, mode);
-		}
-	}
-#else
-	if (restrict_read) {
-		FILE *fptr;
-		mode_t old_mask;
-
-		old_mask = umask(0077);
-		fptr = fopen(path, mode);
-		umask(old_mask);
-
-		return fptr;
-	}else{
-		return fopen(path, mode);
-	}
-#endif
-}
-
 void util__increment_receive_quota(struct mosquitto *mosq)
 {
 	if(mosq->msgs_in.inflight_quota < mosq->msgs_in.inflight_maximum){
@@ -383,21 +293,3 @@ enum mosquitto_client_state mosquitto__get_state(struct mosquitto *mosq)
 
 	return state;
 }
-
-
-char *util__trimblanks(char *str)
-{
-	char *endptr;
-
-	if(str == NULL) return NULL;
-
-	while(isspace(str[0])){
-		str++;
-	}
-	endptr = &str[strlen(str)-1];
-	while(endptr > str && isspace(endptr[0])){
-		endptr[0] = '\0';
-		endptr--;
-	}
-	return str;
-}

lib/util_mosq.h

@@ -31,7 +31,6 @@ int mosquitto__check_keepalive(struct mosquitto_db *db, struct mosquitto *mosq);
 int mosquitto__check_keepalive(struct mosquitto *mosq);
 #endif
 uint16_t mosquitto__mid_generate(struct mosquitto *mosq);
-FILE *mosquitto__fopen(const char *path, const char *mode, bool restrict_read);
 
 int mosquitto__set_state(struct mosquitto *mosq, enum mosquitto_client_state state);
 enum mosquitto_client_state mosquitto__get_state(struct mosquitto *mosq);
@@ -49,5 +48,4 @@ void util__decrement_receive_quota(struct mosquitto *mosq);
 void util__decrement_send_quota(struct mosquitto *mosq);
 
 
-char *util__trimblanks(char *str);
 #endif

src/CMakeLists.txt

@@ -27,6 +27,7 @@ set (MOSQ_SRCS
 	../lib/memory_mosq.c ../lib/memory_mosq.h
 	mosquitto.c
 	mosquitto_broker.h mosquitto_broker_internal.h
+	../lib/misc_mosq.c ../lib/misc_mosq.h
 	net.c
 	../lib/net_mosq_ocsp.c ../lib/net_mosq.c ../lib/net_mosq.h
 	../lib/packet_datatypes.c
@@ -188,7 +189,7 @@ install(TARGETS mosquitto RUNTIME DESTINATION "${CMAKE_INSTALL_SBINDIR}")
 install(FILES mosquitto_broker.h mosquitto_plugin.h DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}")
 
 if (WITH_TLS)
-	add_executable(mosquitto_passwd mosquitto_passwd.c)
+	add_executable(mosquitto_passwd mosquitto_passwd.c ../lib/misc_mosq.c)
 	target_link_libraries(mosquitto_passwd ${OPENSSL_LIBRARIES})
 	install(TARGETS mosquitto_passwd RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}")
 endif (WITH_TLS)

src/Makefile

@@ -31,6 +31,7 @@ OBJS=	mosquitto.o \
 		logging.o \
 		loop.o \
 		memory_mosq.o \
+		misc_mosq.o \
 		net.o \
 		net_mosq.o \
 		net_mosq_ocsp.o \
@@ -143,6 +144,9 @@ loop.o : loop.c mosquitto_broker_internal.h
 memory_mosq.o : ../lib/memory_mosq.c ../lib/memory_mosq.h
 	${CROSS_COMPILE}${CC} $(BROKER_CPPFLAGS) $(BROKER_CFLAGS) -c $< -o $@
 
+misc_mosq.o : ../lib/misc_mosq.c ../lib/misc_mosq.h
+	${CROSS_COMPILE}${CC} $(BROKER_CPPFLAGS) $(BROKER_CFLAGS) -c $< -o $@
+
 net.o : net.c mosquitto_broker_internal.h
 	${CROSS_COMPILE}${CC} $(BROKER_CPPFLAGS) $(BROKER_CFLAGS) -c $< -o $@
 
@@ -260,11 +264,11 @@ will_delay.o : will_delay.c mosquitto_broker_internal.h
 will_mosq.o : ../lib/will_mosq.c ../lib/will_mosq.h
 	${CROSS_COMPILE}${CC} $(BROKER_CPPFLAGS) $(BROKER_CFLAGS) -c $< -o $@
 
-mosquitto_passwd : mosquitto_passwd.o
+mosquitto_passwd : mosquitto_passwd.o misc_mosq.o
 	${CROSS_COMPILE}${CC} ${LDFLAGS} $^ -o $@ $(PASSWD_LDADD)
 
 mosquitto_passwd.o : mosquitto_passwd.c
-	${CROSS_COMPILE}${CC} -I.. $(CPPFLAGS) $(CFLAGS) -c $< -o $@
+	${CROSS_COMPILE}${CC} -I.. -I../lib $(CPPFLAGS) $(CFLAGS) -c $< -o $@
 
 plugin_defer.so : plugin_defer.c mosquitto_plugin.h mosquitto_broker.h mosquitto_broker_internal.h
 	${CROSS_COMPILE}${CC} -I. -I../lib -fPIC -shared $< -o $@

src/conf.c

@@ -42,6 +42,7 @@ Contributors:
 
 #include "mosquitto_broker_internal.h"
 #include "memory_mosq.h"
+#include "misc_mosq.h"
 #include "tls_mosq.h"
 #include "util_mosq.h"
 #include "mqtt_protocol.h"
@@ -71,35 +72,6 @@ static int config__read_file(struct mosquitto__config *config, bool reload, cons
 static int config__check(struct mosquitto__config *config);
 static void config__cleanup_plugins(struct mosquitto__config *config);
 
-static char *fgets_extending(char **buf, int *buflen, FILE *stream)
-{
-	char *rc;
-	char endchar;
-	int offset = 0;
-	char *newbuf;
-
-	do{
-		rc = fgets(&((*buf)[offset]), *buflen-offset, stream);
-		if(feof(stream)){
-			return rc;
-		}
-
-		endchar = (*buf)[strlen(*buf)-1];
-		if(endchar == '\n'){
-			return rc;
-		}
-		/* No EOL char found, so extend buffer */
-		offset = *buflen-1;
-		*buflen += 1000;
-		newbuf = realloc(*buf, *buflen);
-		if(!newbuf){
-			return NULL;
-		}
-		*buf = newbuf;
-	}while(1);
-}
-
-
 static void conf__set_cur_security_options(struct mosquitto__config *config, struct mosquitto__listener *cur_listener, struct mosquitto__security_options **security_options)
 {
 	if(config->per_listener_settings){
@@ -2381,7 +2353,7 @@ static int conf__parse_string(char **token, const char *name, char **value, char
 			return MOSQ_ERR_INVAL;
 		}
 		/* Deal with multiple spaces at the beginning of the string. */
-		*token = util__trimblanks(*token);
+		*token = misc__trimblanks(*token);
 		if(strlen(*token) == 0){
 			log__printf(NULL, MOSQ_LOG_ERR, "Error: Empty %s value in configuration.", name);
 			return MOSQ_ERR_INVAL;

src/logging.c

@@ -29,6 +29,7 @@ Contributors:
 
 #include "mosquitto_broker_internal.h"
 #include "memory_mosq.h"
+#include "misc_mosq.h"
 #include "util_mosq.h"
 
 extern struct mosquitto_db int_db;

src/mosquitto.c

@@ -51,6 +51,7 @@ Contributors:
 
 #include "mosquitto_broker_internal.h"
 #include "memory_mosq.h"
+#include "misc_mosq.h"
 #include "util_mosq.h"
 
 struct mosquitto_db int_db;

src/persist_read.c

@@ -34,6 +34,7 @@ Contributors:
 #include "memory_mosq.h"
 #include "persist.h"
 #include "time_mosq.h"
+#include "misc_mosq.h"
 #include "util_mosq.h"
 
 static uint32_t db_version;

src/persist_write.c

@@ -33,6 +33,7 @@ Contributors:
 #include "memory_mosq.h"
 #include "persist.h"
 #include "time_mosq.h"
+#include "misc_mosq.h"
 #include "util_mosq.h"
 
 static int persist__client_messages_save(struct mosquitto_db *db, FILE *db_fptr, struct mosquitto *context, struct mosquitto_client_msg *queue)

test/unit/Makefile

@@ -10,16 +10,17 @@ LDADD:=$(LDADD) -lcunit
 TEST_OBJS = test.o \
 			datatype_read.o \
 			datatype_write.o \
+			misc_trim_test.o \
 			property_add.o \
 			property_read.o \
 			property_user_read.o \
 			property_write.o \
 			stubs.o \
 			util_topic_test.o \
-			util_trim_test.o \
 			utf8.o
 
 LIB_OBJS = memory_mosq.o \
+		   misc_mosq.o \
 		   packet_datatypes.o \
 		   property_mosq.o \
 		   util_mosq.o \
@@ -32,6 +33,7 @@ PERSIST_READ_TEST_OBJS = \
 
 PERSIST_READ_OBJS = \
 		memory_mosq.o \
+		misc_mosq.o \
 		packet_datatypes.o \
 		persist_read.o \
 		persist_read_v234.o \
@@ -47,6 +49,7 @@ PERSIST_WRITE_TEST_OBJS = \
 PERSIST_WRITE_OBJS = \
 		database.o \
 		memory_mosq.o \
+		misc_mosq.o \
 		packet_datatypes.o \
 		persist_read.o \
 		persist_read_v234.o \
@@ -78,6 +81,9 @@ database.o : ../../src/database.c
 memory_mosq.o : ../../lib/memory_mosq.c
 	$(CROSS_COMPILE)$(CC) $(CPPFLAGS) $(CFLAGS) -c -o $@ $^
 
+misc_mosq.o : ../../lib/misc_mosq.c
+	$(CROSS_COMPILE)$(CC) $(CPPFLAGS) $(CFLAGS) -c -o $@ $^
+
 packet_datatypes.o : ../../lib/packet_datatypes.c
 	$(CROSS_COMPILE)$(CC) $(CPPFLAGS) $(CFLAGS) -c -o $@ $^
 

test/unit/misc_trim_test.c

@@ -1,14 +1,14 @@
 #include <CUnit/CUnit.h>
 #include <CUnit/Basic.h>
 
-#include <util_mosq.h>
+#include <misc_mosq.h>
 
 
 static void rtrim_helper(const char *expected, char *buf)
 {
 	char *res;
 
-	res = util__trimblanks(buf);
+	res = misc__trimblanks(buf);
 	CU_ASSERT_PTR_NOT_NULL(res);
 	if(res){
 		CU_ASSERT_EQUAL(strlen(buf), strlen(res));
@@ -22,7 +22,7 @@ static void ltrim_helper(const char *expected, char *buf)
 {
 	char *res;
 
-	res = util__trimblanks(buf);
+	res = misc__trimblanks(buf);
 	CU_ASSERT_PTR_NOT_NULL(res);
 	if(res){
 		CU_ASSERT_EQUAL(strlen(expected), strlen(res));
@@ -35,7 +35,7 @@ static void TEST_null_input(void)
 {
 	char *res;
 
-	res = util__trimblanks(NULL);
+	res = misc__trimblanks(NULL);
 	CU_ASSERT_PTR_NULL(res);
 }
 
@@ -46,7 +46,7 @@ static void TEST_empty_input(void)
 	char *res;
 
 	memset(buf, 0, sizeof(buf));
-	res = util__trimblanks(buf);
+	res = misc__trimblanks(buf);
 	CU_ASSERT_PTR_NOT_NULL(res);
 	if(res){
 		CU_ASSERT_STRING_EQUAL(res, "");
@@ -156,13 +156,13 @@ static void TEST_btrim(void)
  * TEST SUITE SETUP
  * ======================================================================== */
 
-int init_util_trim_tests(void)
+int init_misc_trim_tests(void)
 {
 	CU_pSuite test_suite = NULL;
 
-	test_suite = CU_add_suite("Util string trim", NULL, NULL);
+	test_suite = CU_add_suite("Misc string trim", NULL, NULL);
 	if(!test_suite){
-		printf("Error adding CUnit util string trim test suite.\n");
+		printf("Error adding CUnit Misc string trim test suite.\n");
 		return 1;
 	}
 
@@ -175,7 +175,7 @@ int init_util_trim_tests(void)
 			|| !CU_add_test(test_suite, "Both trim", TEST_btrim)
 			){
 
-		printf("Error adding util topic CUnit tests.\n");
+		printf("Error adding Misc topic CUnit tests.\n");
 		return 1;
 	}
 

test/unit/test.c

@@ -12,7 +12,7 @@ int init_property_user_read_tests(void);
 int init_property_write_tests(void);
 int init_utf8_tests(void);
 int init_util_topic_tests(void);
-int init_util_trim_tests(void);
+int init_misc_trim_tests(void);
 
 int main(int argc, char *argv[])
 {
@@ -32,7 +32,7 @@ int main(int argc, char *argv[])
 			|| init_property_user_read_tests()
 			|| init_property_write_tests()
 			|| init_util_topic_tests()
-			|| init_util_trim_tests()
+			|| init_misc_trim_tests()
 			){
 
         CU_cleanup_registry();