Fix for CVE-2017-9868 for Windows.

eclipse · Jun 26, 2017 · 6e7d02b · 6e7d02b
1 parent 408972d
commit 6e7d02b
Show file tree

Hide file tree

Showing 9 changed files with 80 additions and 18 deletions.
diff --git a/ChangeLog.txt b/ChangeLog.txt
@@ -1,3 +1,12 @@
+1.4.13 - 20170xxx
+=================
+
+Security:
+- Fix CVE-2017-9868. The persistence file was readable by all local users,
+ potentially allowing sensitive information to be leaked.
+ This can also be fixed administratively, by restricting access to the
+ directory in which the persistence file is stored.
+
 Broker:
 - Fix for poor websockets performance.
 - Fix lazy bridges not timing out for idle_timeout. Closes #417.

diff --git a/lib/mosquitto.c b/lib/mosquitto.c
@@ -653,7 +653,7 @@ int mosquitto_tls_set(struct mosquitto *mosq, const char *cafile, const char *ca
  if(!mosq || (!cafile && !capath) || (certfile && !keyfile) || (!certfile && keyfile)) return MOSQ_ERR_INVAL;
 
  if(cafile){
- fptr = _mosquitto_fopen(cafile, "rt");
+ fptr = _mosquitto_fopen(cafile, "rt", false);
  if(fptr){
  fclose(fptr);
  }else{
@@ -680,7 +680,7 @@ int mosquitto_tls_set(struct mosquitto *mosq, const char *cafile, const char *ca
  }
 
  if(certfile){
- fptr = _mosquitto_fopen(certfile, "rt");
+ fptr = _mosquitto_fopen(certfile, "rt", false);
  if(fptr){
  fclose(fptr);
  }else{
@@ -704,7 +704,7 @@ int mosquitto_tls_set(struct mosquitto *mosq, const char *cafile, const char *ca
  }
 
  if(keyfile){
- fptr = _mosquitto_fopen(keyfile, "rt");
+ fptr = _mosquitto_fopen(keyfile, "rt", false);
  if(fptr){
  fclose(fptr);
  }else{

diff --git a/lib/util_mosq.c b/lib/util_mosq.c
@@ -18,7 +18,12 @@ and the Eclipse Distribution License is available at
 #include <string.h>
 
 #ifdef WIN32
-#include <winsock2.h>
+# include <winsock2.h>
+# include <aclapi.h>
+# include <io.h>
+# include <lmcons.h>
+#else
+# include <sys/stat.h>
 #endif
 
 
@@ -95,7 +100,7 @@ void _mosquitto_check_keepalive(struct mosquitto *mosq)
  /* Check if a lazy bridge should be timed out due to idle. */
  if(mosq->bridge && mosq->bridge->start_type == bst_lazy
  && mosq->sock != INVALID_SOCKET
- && now - mosq->next_msg_out + mosq->keepalive >= mosq->bridge->idle_timeout){
+ && now - mosq->next_msg_out - mosq->keepalive >= mosq->bridge->idle_timeout){
 
  _mosquitto_log_printf(NULL, MOSQ_LOG_NOTICE, "Bridge connection %s has exceeded idle timeout, disconnecting.", mosq->id);
  _mosquitto_socket_close(db, mosq);
@@ -338,7 +343,7 @@ int _mosquitto_hex2bin(const char *hex, unsigned char *bin, int bin_max_len)
 }
 #endif
 
-FILE *_mosquitto_fopen(const char *path, const char *mode)
+FILE *_mosquitto_fopen(const char *path, const char *mode, bool restrict_read)
 {
 #ifdef WIN32
  char buf[4096];
@@ -347,9 +352,60 @@ FILE *_mosquitto_fopen(const char *path, const char *mode)
  if(rc == 0 || rc > 4096){
  return NULL;
  }else{
- return fopen(buf, mode);
+ if (restrict_read) {
+ HANDLE hfile;
+ SECURITY_ATTRIBUTES sec;
+ EXPLICIT_ACCESS ea;
+ PACL pacl = NULL;
+ char username[UNLEN + 1];
+ int ulen = UNLEN;
+ SECURITY_DESCRIPTOR sd;
+
+ 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, 0,
+ &sec,
+ CREATE_NEW,
+ 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) {
+ umask(0700);
+ }
  return fopen(path, mode);
 #endif
 }

diff --git a/lib/util_mosq.h b/lib/util_mosq.h
@@ -32,7 +32,7 @@ void _mosquitto_check_keepalive(struct mosquitto_db *db, struct mosquitto *mosq)
 void _mosquitto_check_keepalive(struct mosquitto *mosq);
 #endif
 uint16_t _mosquitto_mid_generate(struct mosquitto *mosq);
-FILE *_mosquitto_fopen(const char *path, const char *mode);
+FILE *_mosquitto_fopen(const char *path, const char *mode, bool restrict_read);
 
 #ifdef REAL_WITH_TLS_PSK
 int _mosquitto_hex2bin(const char *hex, unsigned char *bin, int bin_max_len);

diff --git a/src/conf.c b/src/conf.c
@@ -1762,7 +1762,7 @@ int _config_read_file(struct mqtt3_config *config, bool reload, const char *file
  int rc;
  FILE *fptr = NULL;
 
- fptr = _mosquitto_fopen(file, "rt");
+ fptr = _mosquitto_fopen(file, "rt", false);
  if(!fptr){
  _mosquitto_log_printf(NULL, MOSQ_LOG_ERR, "Error: Unable to open config file %s\n", file);
  return 1;

diff --git a/src/logging.c b/src/logging.c
@@ -69,7 +69,7 @@ int mqtt3_log_init(struct mqtt3_config *config)
  if(drop_privileges(config, true)){
  return 1;
  }
- config->log_fptr = _mosquitto_fopen(config->log_file, "at");
+ config->log_fptr = _mosquitto_fopen(config->log_file, "at", true);
  if(!config->log_fptr){
  log_destinations = MQTT3_LOG_STDERR;
  log_priorities = MOSQ_LOG_ERR;

diff --git a/src/mosquitto.c b/src/mosquitto.c
@@ -269,7 +269,7 @@ int main(int argc, char *argv[])
  }
 
  if(config.daemon && config.pid_file){
- pid = _mosquitto_fopen(config.pid_file, "wt");
+ pid = _mosquitto_fopen(config.pid_file, "wt", false);
  if(pid){
  fprintf(pid, "%d", getpid());
  fclose(pid);

diff --git a/src/persist.c b/src/persist.c
@@ -402,12 +402,9 @@ int mqtt3_db_backup(struct mosquitto_db *db, bool shutdown)
  goto error;
  }
  }
-
- /* Set permissions to -rw------- */
- umask(0077);
 #endif
 
- db_fptr = _mosquitto_fopen(outfile, "wb");
+ db_fptr = _mosquitto_fopen(outfile, "wb", true);
  if(db_fptr == NULL){
  _mosquitto_log_printf(NULL, MOSQ_LOG_INFO, "Error saving in-memory database, unable to open %s for writing.", outfile);
  goto error;
@@ -824,7 +821,7 @@ int mqtt3_db_restore(struct mosquitto_db *db)
 
  db->msg_store_load = NULL;
 
- fptr = _mosquitto_fopen(db->config->persistence_filepath, "rb");
+ fptr = _mosquitto_fopen(db->config->persistence_filepath, "rb", false);
  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
@@ -352,7 +352,7 @@ static int _aclfile_parse(struct mosquitto_db *db)
  if(!db || !db->config) return MOSQ_ERR_INVAL;
  if(!db->config->acl_file) return MOSQ_ERR_SUCCESS;
 
- aclfile = _mosquitto_fopen(db->config->acl_file, "rt");
+ aclfile = _mosquitto_fopen(db->config->acl_file, "rt", false);
  if(!aclfile){
  _mosquitto_log_printf(NULL, MOSQ_LOG_ERR, "Error: Unable to open acl_file \"%s\".", db->config->acl_file);
  return 1;
@@ -511,7 +511,7 @@ static int _pwfile_parse(const char *file, struct _mosquitto_unpwd **root)
  int len;
  char *saveptr = NULL;
 
- pwfile = _mosquitto_fopen(file, "rt");
+ pwfile = _mosquitto_fopen(file, "rt", false);
  if(!pwfile){
  _mosquitto_log_printf(NULL, MOSQ_LOG_ERR, "Error: Unable to open pwfile \"%s\".", file);
  return 1;