From 4ca294fd9c2d460deb15ae85ccd891178df05704 Mon Sep 17 00:00:00 2001 From: "Roger A. Light" Date: Thu, 8 Jun 2023 21:02:29 +0100 Subject: [PATCH] Warn on lax permissions on sensitive files. - Broker will log warnings if sensitive files are world readable/writable, or if the owner/group is not the same as the user/group the broker is running as. In future versions the broker will refuse to open these files. --- ChangeLog.txt | 3 + apps/mosquitto_ctrl/CMakeLists.txt | 1 + apps/mosquitto_ctrl/Makefile | 1 + apps/mosquitto_ctrl/dynsec.c | 5 +- apps/mosquitto_passwd/mosquitto_passwd.c | 6 +- lib/misc_mosq.c | 56 +++++++ plugins/dynamic-security/plugin.c | 188 ++++++++++++++++++++++- src/net.c | 3 +- src/persist_read.c | 2 +- src/security_default.c | 4 +- 10 files changed, 258 insertions(+), 11 deletions(-) diff --git a/ChangeLog.txt b/ChangeLog.txt index 655ddd07..8e619c0c 100644 --- a/ChangeLog.txt +++ b/ChangeLog.txt @@ -11,6 +11,9 @@ Broker: on start after restoring from persistence. Closes #2634. - Fix connections being limited to 2048 on Windows. The limit is now 8192, where supported. Closes #2732. +- Broker will log warnings if sensitive files are world readable/writable, or + if the owner/group is not the same as the user/group the broker is running + as. In future versions the broker will refuse to open these files. Client library: - Use CLOCK_BOOTTIME when available, to keep track of time. This solves the diff --git a/apps/mosquitto_ctrl/CMakeLists.txt b/apps/mosquitto_ctrl/CMakeLists.txt index d4466fe5..26dbe8d6 100644 --- a/apps/mosquitto_ctrl/CMakeLists.txt +++ b/apps/mosquitto_ctrl/CMakeLists.txt @@ -17,6 +17,7 @@ if (WITH_TLS AND CJSON_FOUND) dynsec_role.c ../mosquitto_passwd/get_password.c ../mosquitto_passwd/get_password.h ../../lib/memory_mosq.c ../../lib/memory_mosq.h + ../../lib/misc_mosq.c ../../lib/misc_mosq.h ../../src/memory_public.c options.c ../../src/password_mosq.c ../../src/password_mosq.h diff --git a/apps/mosquitto_ctrl/Makefile b/apps/mosquitto_ctrl/Makefile index 502f0dac..cf9ac0dc 100644 --- a/apps/mosquitto_ctrl/Makefile +++ b/apps/mosquitto_ctrl/Makefile @@ -23,6 +23,7 @@ OBJS= mosquitto_ctrl.o \ get_password.o \ memory_mosq.o \ memory_public.o \ + misc_mosq.o \ options.o \ password_mosq.o diff --git a/apps/mosquitto_ctrl/dynsec.c b/apps/mosquitto_ctrl/dynsec.c index c74147b6..8e5dd726 100644 --- a/apps/mosquitto_ctrl/dynsec.c +++ b/apps/mosquitto_ctrl/dynsec.c @@ -30,6 +30,7 @@ Contributors: #include "mosquitto.h" #include "password_mosq.h" #include "get_password.h" +#include "misc_mosq.h" void dynsec__print_usage(void) { @@ -738,7 +739,7 @@ static int dynsec_init(int argc, char *argv[]) admin_password = password; } - fptr = fopen(filename, "rb"); + fptr = mosquitto__fopen(filename, "rb", true); if(fptr){ fclose(fptr); fprintf(stderr, "dynsec init: '%s' already exists. Remove the file or use a different location..\n", filename); @@ -753,7 +754,7 @@ static int dynsec_init(int argc, char *argv[]) json_str = cJSON_Print(tree); cJSON_Delete(tree); - fptr = fopen(filename, "wb"); + fptr = mosquitto__fopen(filename, "wb", true); if(fptr){ fprintf(fptr, "%s", json_str); free(json_str); diff --git a/apps/mosquitto_passwd/mosquitto_passwd.c b/apps/mosquitto_passwd/mosquitto_passwd.c index f5b2470e..0a5ba722 100644 --- a/apps/mosquitto_passwd/mosquitto_passwd.c +++ b/apps/mosquitto_passwd/mosquitto_passwd.c @@ -374,7 +374,7 @@ static int create_backup(const char *backup_file, FILE *fptr) { FILE *fbackup; - fbackup = fopen(backup_file, "wt"); + fbackup = mosquitto__fopen(backup_file, "wt", true); if(!fbackup){ fprintf(stderr, "Error creating backup password file \"%s\", not continuing.\n", backup_file); return 1; @@ -599,7 +599,7 @@ int main(int argc, char *argv[]) } password_cmd = password; } - fptr = fopen(password_file, "wt"); + fptr = mosquitto__fopen(password_file, "wt", true); if(!fptr){ fprintf(stderr, "Error: Unable to open file %s for writing. %s.\n", password_file, strerror(errno)); free(password_file); @@ -610,7 +610,7 @@ int main(int argc, char *argv[]) fclose(fptr); return rc; }else{ - fptr = fopen(password_file, "r+t"); + fptr = mosquitto__fopen(password_file, "r+t", true); if(!fptr){ fprintf(stderr, "Error: Unable to open password file %s. %s.\n", password_file, strerror(errno)); free(password_file); diff --git a/lib/misc_mosq.c b/lib/misc_mosq.c index 2529d794..3ee6fc79 100644 --- a/lib/misc_mosq.c +++ b/lib/misc_mosq.c @@ -38,6 +38,8 @@ Contributors: # define PATH_MAX MAX_PATH #else # include +# include +# include # include #endif @@ -149,6 +151,60 @@ FILE *mosquitto__fopen(const char *path, const char *mode, bool restrict_read) return NULL; } + if(restrict_read){ + if(statbuf.st_mode & S_IRWXO){ +#ifdef WITH_BROKER + log__printf(NULL, MOSQ_LOG_WARNING, +#else + fprintf(stderr, +#endif + "Warning: File %s has world readable permissions. Future versions will refuse to load this file.", + path); +#if 0 + return NULL; +#endif + } + if(statbuf.st_uid != getuid()){ + char buf[4096]; + struct passwd pw, *result; + + getpwuid_r(getuid(), &pw, buf, sizeof(buf), &result); + if(result){ +#ifdef WITH_BROKER + log__printf(NULL, MOSQ_LOG_WARNING, +#else + fprintf(stderr, +#endif + "Warning: File %s owner is not %s. Future versions will refuse to load this file.", + path, result->pw_name); + } +#if 0 + // Future version + return NULL; +#endif + } + if(statbuf.st_gid != getgid()){ + char buf[4096]; + struct group grp, *result; + + getgrgid_r(getgid(), &grp, buf, sizeof(buf), &result); + if(result){ +#ifdef WITH_BROKER + log__printf(NULL, MOSQ_LOG_WARNING, +#else + fprintf(stderr, +#endif + "Warning: File %s group is not %s. Future versions will refuse to load this file.", + path, result->gr_name); + } +#if 0 + // Future version + return NULL +#endif + } + } + + if(!S_ISREG(statbuf.st_mode) && !S_ISLNK(statbuf.st_mode)){ #ifdef WITH_BROKER log__printf(NULL, MOSQ_LOG_ERR, "Error: %s is not a file.", path); diff --git a/plugins/dynamic-security/plugin.c b/plugins/dynamic-security/plugin.c index 59aca8cd..7a1119c7 100644 --- a/plugins/dynamic-security/plugin.c +++ b/plugins/dynamic-security/plugin.c @@ -41,6 +41,190 @@ static mosquitto_plugin_id_t *plg_id = NULL; static char *config_file = NULL; struct dynsec__acl_default_access default_access = {false, false, false, false}; +#ifdef WIN32 +# include +# include +# include +# include +# include +# define PATH_MAX MAX_PATH +#else +# include +# include +# include +# include +#endif +/* Temporary - remove in 2.1 */ +FILE *mosquitto__fopen(const char *path, const char *mode, bool restrict_read) +{ +#ifdef WIN32 + char buf[4096]; + int rc; + int flags = 0; + + rc = ExpandEnvironmentStringsA(path, buf, 4096); + if(rc == 0 || rc > 4096){ + return NULL; + }else{ + if (restrict_read) { + HANDLE hfile; + SECURITY_ATTRIBUTES sec; + EXPLICIT_ACCESS_A ea; + PACL pacl = NULL; + char username[UNLEN + 1]; + DWORD ulen = UNLEN; + SECURITY_DESCRIPTOR sd; + DWORD dwCreationDisposition; + int fd; + FILE *fptr; + + switch(mode[0]){ + case 'a': + dwCreationDisposition = OPEN_ALWAYS; + flags = _O_APPEND; + break; + case 'r': + dwCreationDisposition = OPEN_EXISTING; + flags = _O_RDONLY; + break; + case 'w': + dwCreationDisposition = CREATE_ALWAYS; + break; + default: + return NULL; + } + + GetUserNameA(username, &ulen); + if (!InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION)) { + return NULL; + } + BuildExplicitAccessWithNameA(&ea, username, GENERIC_ALL, SET_ACCESS, NO_INHERITANCE); + if (SetEntriesInAclA(1, &ea, NULL, &pacl) != ERROR_SUCCESS) { + return NULL; + } + if (!SetSecurityDescriptorDacl(&sd, TRUE, pacl, FALSE)) { + LocalFree(pacl); + return NULL; + } + + memset(&sec, 0, sizeof(sec)); + sec.nLength = sizeof(SECURITY_ATTRIBUTES); + sec.bInheritHandle = FALSE; + sec.lpSecurityDescriptor = &sd; + + hfile = CreateFileA(buf, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ, + &sec, + dwCreationDisposition, + FILE_ATTRIBUTE_NORMAL, + NULL); + + LocalFree(pacl); + + fd = _open_osfhandle((intptr_t)hfile, flags); + if (fd < 0) { + return NULL; + } + + fptr = _fdopen(fd, mode); + if (!fptr) { + _close(fd); + return NULL; + } + if(mode[0] == 'a'){ + fseek(fptr, 0, SEEK_END); + } + return fptr; + + }else { + return fopen(buf, mode); + } + } +#else + FILE *fptr; + struct stat statbuf; + + if (restrict_read) { + mode_t old_mask; + + old_mask = umask(0077); + fptr = fopen(path, mode); + umask(old_mask); + }else{ + fptr = fopen(path, mode); + } + if(!fptr) return NULL; + + if(fstat(fileno(fptr), &statbuf) < 0){ + fclose(fptr); + return NULL; + } + + if(restrict_read){ + if(statbuf.st_mode & S_IRWXO){ +#ifdef WITH_BROKER + log__printf(NULL, MOSQ_LOG_WARNING, +#else + fprintf(stderr, +#endif + "Warning: File %s has world readable permissions. Future versions will refuse to load this file.", + path); +#if 0 + return NULL; +#endif + } + if(statbuf.st_uid != getuid()){ + char buf[4096]; + struct passwd pw, *result; + + getpwuid_r(getuid(), &pw, buf, sizeof(buf), &result); + if(result){ +#ifdef WITH_BROKER + log__printf(NULL, MOSQ_LOG_WARNING, +#else + fprintf(stderr, +#endif + "Warning: File %s owner is not %s. Future versions will refuse to load this file.", + path, result->pw_name); + } +#if 0 + // Future version + return NULL; +#endif + } + if(statbuf.st_gid != getgid()){ + char buf[4096]; + struct group grp, *result; + + getgrgid_r(getgid(), &grp, buf, sizeof(buf), &result); + if(result){ +#ifdef WITH_BROKER + log__printf(NULL, MOSQ_LOG_WARNING, +#else + fprintf(stderr, +#endif + "Warning: File %s group is not %s. Future versions will refuse to load this file.", + path, result->gr_name); + } +#if 0 + // Future version + return NULL +#endif + } + } + + + if(!S_ISREG(statbuf.st_mode) && !S_ISLNK(statbuf.st_mode)){ +#ifdef WITH_BROKER + log__printf(NULL, MOSQ_LOG_ERR, "Error: %s is not a file.", path); +#endif + fclose(fptr); + return NULL; + } + return fptr; +#endif +} + + void dynsec__command_reply(cJSON *j_responses, struct mosquitto *context, const char *command, const char *error, const char *correlation_data) { cJSON *j_response; @@ -359,7 +543,7 @@ static int dynsec__config_load(void) /* Load from file */ errno = 0; - fptr = fopen(config_file, "rb"); + fptr = mosquitto__fopen(config_file, "rb", true); if(fptr == NULL){ mosquitto_log_printf(MOSQ_LOG_ERR, "Error loading Dynamic security plugin config: File is not readable - check permissions.\n"); return MOSQ_ERR_ERRNO; @@ -460,7 +644,7 @@ void dynsec__config_save(void) } snprintf(file_path, file_path_len, "%s.new", config_file); - fptr = fopen(file_path, "wt"); + fptr = mosquitto__fopen(file_path, "wt", true); if(fptr == NULL){ mosquitto_free(json_str); mosquitto_free(file_path); diff --git a/src/net.c b/src/net.c index 7ae33cbc..51452307 100644 --- a/src/net.c +++ b/src/net.c @@ -56,6 +56,7 @@ Contributors: #include "mosquitto_broker_internal.h" #include "mqtt_protocol.h" #include "memory_mosq.h" +#include "misc_mosq.h" #include "net_mosq.h" #include "util_mosq.h" @@ -416,7 +417,7 @@ int net__tls_server_ctx(struct mosquitto__listener *listener) #endif if(listener->dhparamfile){ - dhparamfile = fopen(listener->dhparamfile, "r"); + dhparamfile = mosquitto__fopen(listener->dhparamfile, "r", true); if(!dhparamfile){ log__printf(NULL, MOSQ_LOG_ERR, "Error loading dhparamfile \"%s\".", listener->dhparamfile); return MOSQ_ERR_TLS; diff --git a/src/persist_read.c b/src/persist_read.c index 107e8602..14a2f4f0 100644 --- a/src/persist_read.c +++ b/src/persist_read.c @@ -427,7 +427,7 @@ int persist__restore(void) db.msg_store_load = NULL; - fptr = mosquitto__fopen(db.config->persistence_filepath, "rb", false); + fptr = mosquitto__fopen(db.config->persistence_filepath, "rb", true); if(fptr == NULL) return MOSQ_ERR_SUCCESS; rlen = fread(&header, 1, 15, fptr); if(rlen == 0){ diff --git a/src/security_default.c b/src/security_default.c index 4fc6a78b..015ce01b 100644 --- a/src/security_default.c +++ b/src/security_default.c @@ -530,7 +530,7 @@ static int aclfile__parse(struct mosquitto__security_options *security_opts) return MOSQ_ERR_NOMEM; } - aclfptr = mosquitto__fopen(security_opts->acl_file, "rt", false); + aclfptr = mosquitto__fopen(security_opts->acl_file, "rt", true); if(!aclfptr){ mosquitto__free(buf); log__printf(NULL, MOSQ_LOG_ERR, "Error: Unable to open acl_file \"%s\".", security_opts->acl_file); @@ -755,7 +755,7 @@ static int pwfile__parse(const char *file, struct mosquitto__unpwd **root) return MOSQ_ERR_NOMEM; } - pwfile = mosquitto__fopen(file, "rt", false); + pwfile = mosquitto__fopen(file, "rt", true); if(!pwfile){ log__printf(NULL, MOSQ_LOG_ERR, "Error: Unable to open pwfile \"%s\".", file); mosquitto__free(buf);