Fix and tests for CVE-2018-12546.

ChangeLog.txt

@@ -17,6 +17,12 @@ Security:
  is not a useful configuration, this behaviour is unexpected and could lead
  to access being incorrectly granted in some circumstances. This is now
  fixed. Affects versions 1.0 to 1.5.5 inclusive.
+- Fix CVE-2018-12546. If a client publishes a retained message to a topic that
+ they have access to, and then their access to that topic is revoked, the
+ retained message will still be delivered to future subscribers. This
+ behaviour may be undesirable in some applications, so a configuration option
+ `check_retain_source` has been introduced to enforce checking of the
+ retained message source on publish.
 
 Broker:
 - Fixed comment handling for config options that have optional arguments.

man/mosquitto.conf.5.xml

@@ -294,6 +294,24 @@
  <para>Reloaded on reload signal.</para>
  </listitem>
  </varlistentry>
+ <varlistentry>
+ <term><option>check_retain_source</option> [ true | false ]</term>
+ <listitem>
+ <para>This option affects the scenario when a client
+ subscribes to a topic that has retained messages. It is
+ possible that the client that published the retained
+ message to the topic had access at the time they
+ published, but that access has been subsequently
+ removed. If <option>check_retain_source</option> is set
+ to true, the default, the source of a retained message
+ will be checked for access rights before it is
+ republished. When set to false, no check will be made
+ and the retained message will always be
+ published.</para>
+ <para>This option applies globally, regardless of the
+ <option>per_listener_settings</option> option.</para>
+ </listitem>
+ </varlistentry>
  <varlistentry>
  <term><option>clientid_prefixes</option> <replaceable>prefix</replaceable></term>
  <listitem>

mosquitto.conf

@@ -148,6 +148,15 @@
 # setting behaviour from previous versions of mosquitto.
 #per_listener_settings false
 
+# This option affects the scenario when a client subscribes to a topic that has
+# retained messages. It is possible that the client that published the retained
+# message to the topic had access at the time they published, but that access
+# has been subsequently removed. If check_retain_source is set to true, the
+# default, the source of a retained message will be checked for access rights
+# before it is republished. When set to false, no check will be made and the
+# retained message will always be published. This affects all listeners.
+#check_retain_source true
+
 
 # =================================================================
 # Default listener

src/conf.c

@@ -1093,6 +1093,9 @@ int config__read_file_core(struct mosquitto__config *config, bool reload, struct
 #else
  log__printf(NULL, MOSQ_LOG_WARNING, "Warning: TLS support not available.");
 #endif
+ }else if(!strcmp(token, "check_retain_source")){
+ conf__set_cur_security_options(config, cur_listener, &cur_security_options);
+ if(conf__parse_bool(&token, "check_retain_source", &config->check_retain_source, saveptr)) return MOSQ_ERR_INVAL;
  }else if(!strcmp(token, "ciphers")){
 #ifdef WITH_TLS
  if(reload) continue; // Listeners not valid for reloading.

src/database.c

@@ -200,6 +200,7 @@ void db__msg_store_remove(struct mosquitto_db *db, struct mosquitto_msg_store *s
  db->msg_store_bytes -= store->payloadlen;
 
  mosquitto__free(store->source_id);
+ mosquitto__free(store->source_username);
  if(store->dest_ids){
  for(i=0; i<store->dest_id_count; i++){
  mosquitto__free(store->dest_ids[i]);
@@ -587,26 +588,27 @@ int db__messages_easy_queue(struct mosquitto_db *db, struct mosquitto *context,
  }
  memcpy(UHPA_ACCESS(payload_uhpa, payloadlen), payload, payloadlen);
 
+ if(db__message_store(db, context, 0, topic_heap, qos, payloadlen, &payload_uhpa, retain, &stored, 0)) return 1;
+
  if(context && context->id){
  source_id = context->id;
  }else{
  source_id = "";
  }
- if(db__message_store(db, source_id, 0, topic_heap, qos, payloadlen, &payload_uhpa, retain, &stored, 0)) return 1;
 
  return sub__messages_queue(db, source_id, topic_heap, qos, retain, &stored);
 }
 
 /* This function requires topic to be allocated on the heap. Once called, it owns topic and will free it on error. Likewise payload. */
-int db__message_store(struct mosquitto_db *db, const char *source, uint16_t source_mid, char *topic, int qos, uint32_t payloadlen, mosquitto__payload_uhpa *payload, int retain, struct mosquitto_msg_store **stored, dbid_t store_id)
+int db__message_store(struct mosquitto_db *db, const struct mosquitto *source, uint16_t source_mid, char *topic, int qos, uint32_t payloadlen, mosquitto__payload_uhpa *payload, int retain, struct mosquitto_msg_store **stored, dbid_t store_id)
 {
  struct mosquitto_msg_store *temp = NULL;
  int rc = MOSQ_ERR_SUCCESS;
 
  assert(db);
  assert(stored);
 
- temp = mosquitto__malloc(sizeof(struct mosquitto_msg_store));
+ temp = mosquitto__calloc(1, sizeof(struct mosquitto_msg_store));
  if(!temp){
  log__printf(NULL, MOSQ_LOG_ERR, "Error: Out of memory.");
  rc = MOSQ_ERR_NOMEM;
@@ -617,8 +619,8 @@ int db__message_store(struct mosquitto_db *db, const char *source, uint16_t sour
  temp->payload.ptr = NULL;
 
  temp->ref_count = 0;
- if(source){
- temp->source_id = mosquitto__strdup(source);
+ if(source && source->id){
+ temp->source_id = mosquitto__strdup(source->id);
  }else{
  temp->source_id = mosquitto__strdup("");
  }
@@ -627,6 +629,17 @@ int db__message_store(struct mosquitto_db *db, const char *source, uint16_t sour
  rc = MOSQ_ERR_NOMEM;
  goto error;
  }
+
+ if(source && source->username){
+ temp->source_username = mosquitto__strdup(source->username);
+ if(!temp->source_username){
+ rc = MOSQ_ERR_NOMEM;
+ goto error;
+ }
+ }
+ if(source){
+ temp->source_listener = source->listener;
+ }
  temp->source_mid = source_mid;
  temp->mid = 0;
  temp->qos = qos;
@@ -659,6 +672,7 @@ int db__message_store(struct mosquitto_db *db, const char *source, uint16_t sour
  mosquitto__free(topic);
  if(temp){
  mosquitto__free(temp->source_id);
+ mosquitto__free(temp->source_username);
  mosquitto__free(temp->topic);
  mosquitto__free(temp);
  }

src/db_dump/Makefile

@@ -1,6 +1,6 @@
 include ../../config.mk
 
-CFLAGS_FINAL=${CFLAGS} -I.. -I../../lib -I../..
+CFLAGS_FINAL=${CFLAGS} -I.. -I../../lib -I../.. -I../deps
 
 .PHONY: all clean reallyclean
 

src/db_dump/db_dump.c

@@ -83,7 +83,9 @@ struct db_msg
  uint8_t qos, retain;
  uint8_t *payload;
  char *source_id;
+ char *source_username;
  char *topic;
+ uint16_t source_port;
 };
 
 static uint32_t db_version;
@@ -177,6 +179,8 @@ print_db_msg(struct db_msg *msg, int length)
  printf("\tLength: %d\n", length);
  printf("\tStore ID: %" PRIu64 "\n", msg->store_id);
  printf("\tSource ID: %s\n", msg->source_id);
+ printf("\tSource Username: %s\n", msg->source_username);
+ printf("\tSource Port: %d\n", msg->source_port);
  printf("\tSource MID: %d\n", msg->source_mid);
  printf("\tMID: %d\n", msg->mid);
  printf("\tTopic: %s\n", msg->topic);
@@ -194,26 +198,49 @@ print_db_msg(struct db_msg *msg, int length)
 }
 
 
+static int persist__read_string(FILE *db_fptr, char **str)
+{
+ uint16_t i16temp;
+ uint16_t slen;
+ char *s = NULL;
+
+ if(fread(&i16temp, 1, sizeof(uint16_t), db_fptr) != sizeof(uint16_t)){
+ return MOSQ_ERR_INVAL;
+ }
+
+ slen = ntohs(i16temp);
+ if(slen){
+ s = mosquitto__malloc(slen+1);
+ if(!s){
+ fclose(db_fptr);
+ fprintf(stderr, "Error: Out of memory.\n");
+ return MOSQ_ERR_NOMEM;
+ }
+ if(fread(s, 1, slen, db_fptr) != slen){
+ mosquitto__free(s);
+ fprintf(stderr, "Error: %s.\n", strerror(errno));
+ return MOSQ_ERR_INVAL;
+ }
+ s[slen] = '\0';
+ }
+
+ *str = s;
+ return MOSQ_ERR_SUCCESS;
+}
+
+
 static int db__client_chunk_restore(struct mosquitto_db *db, FILE *db_fd, struct db_client *client)
 {
- uint16_t i16temp, slen;
+ uint16_t i16temp;
  int rc = 0;
  struct client_chunk *cc;
 
- read_e(db_fd, &i16temp, sizeof(uint16_t));
- slen = ntohs(i16temp);
- if(!slen){
+ rc = persist__read_string(db_fd, &client->client_id);
+ if(rc){
  fprintf(stderr, "Error: Corrupt persistent database.");
  fclose(db_fd);
- return 1;
+ return rc;
  }
- client->client_id = calloc(slen+1, sizeof(char));
- if(!client->client_id){
- fclose(db_fd);
- fprintf(stderr, "Error: Out of memory.");
- return 1;
- }
- read_e(db_fd, client->client_id, slen);
 
  read_e(db_fd, &i16temp, sizeof(uint16_t));
  client->last_mid = ntohs(i16temp);
@@ -245,24 +272,17 @@ static int db__client_chunk_restore(struct mosquitto_db *db, FILE *db_fd, struct
 static int db__client_msg_chunk_restore(struct mosquitto_db *db, FILE *db_fd, uint32_t length, struct db_client_msg *msg)
 {
  dbid_t i64temp;
- uint16_t i16temp, slen;
+ uint16_t i16temp;
  struct client_chunk *cc;
  struct msg_store_chunk *msc;
+ int rc;
 
- read_e(db_fd, &i16temp, sizeof(uint16_t));
- slen = ntohs(i16temp);
- if(!slen){
+ rc = persist__read_string(db_fd, &msg->client_id);
+ if(rc){
  fprintf(stderr, "Error: Corrupt persistent database.");
  fclose(db_fd);
- return 1;
+ return rc;
  }
- msg->client_id = calloc(slen+1, sizeof(char));
- if(!msg->client_id){
- fclose(db_fd);
- fprintf(stderr, "Error: Out of memory.");
- return 1;
- }
- read_e(db_fd, msg->client_id, slen);
 
  read_e(db_fd, &i64temp, sizeof(dbid_t));
  msg->store_id = i64temp;
@@ -301,58 +321,48 @@ static int db__msg_store_chunk_restore(struct mosquitto_db *db, FILE *db_fd, uin
 {
  dbid_t i64temp;
  uint32_t i32temp;
- uint16_t i16temp, slen;
+ uint16_t i16temp;
  int rc = 0;
  struct msg_store_chunk *mcs;
 
  read_e(db_fd, &i64temp, sizeof(dbid_t));
  msg->store_id = i64temp;
 
- read_e(db_fd, &i16temp, sizeof(uint16_t));
- slen = ntohs(i16temp);
- if(slen){
- msg->source_id = calloc(slen+1, sizeof(char));
- if(!msg->source_id){
- fclose(db_fd);
- fprintf(stderr, "Error: Out of memory.");
- return 1;
- }
- if(fread(msg->source_id, 1, slen, db_fd) != slen){
+ rc = persist__read_string(db_fd, &msg->source_id);
+ if(rc){
+ fprintf(stderr, "Error: Corrupt persistent database.");
+ fclose(db_fd);
+ return rc;
+ }
+ if(db_version == 4){
+ rc = persist__read_string(db_fd, &msg->source_username);
+ if(rc){
  fprintf(stderr, "Error: %s.", strerror(errno));
  fclose(db_fd);
  free(msg->source_id);
+ free(msg->topic);
+ free(msg->payload);
+ free(msg->source_id);
  return 1;
  }
+ read_e(db_fd, &i16temp, sizeof(uint16_t));
+ msg->source_port = ntohs(i16temp);
  }
+
+
  read_e(db_fd, &i16temp, sizeof(uint16_t));
  msg->source_mid = ntohs(i16temp);
 
  read_e(db_fd, &i16temp, sizeof(uint16_t));
  msg->mid = ntohs(i16temp);
 
- read_e(db_fd, &i16temp, sizeof(uint16_t));
- slen = ntohs(i16temp);
- if(slen){
- msg->topic = calloc(slen+1, sizeof(char));
- if(!msg->topic){
- fclose(db_fd);
- free(msg->source_id);
- fprintf(stderr, "Error: Out of memory.");
- return 1;
- }
- if(fread(msg->topic, 1, slen, db_fd) != slen){
- fprintf(stderr, "Error: %s.", strerror(errno));
- fclose(db_fd);
- free(msg->source_id);
- free(msg->topic);
- return 1;
- }
- }else{
- fprintf(stderr, "Error: Invalid msg_store chunk when restoring persistent database.");
+ rc = persist__read_string(db_fd, &msg->topic);
+ if(rc){
+ fprintf(stderr, "Error: Corrupt persistent database.");
  fclose(db_fd);
- free(msg->source_id);
- return 1;
+ return rc;
  }
+
  read_e(db_fd, &msg->qos, sizeof(uint8_t));
  read_e(db_fd, &msg->retain, sizeof(uint8_t));
 
@@ -415,29 +425,23 @@ static int db__retain_chunk_restore(struct mosquitto_db *db, FILE *db_fd)
 
 static int db__sub_chunk_restore(struct mosquitto_db *db, FILE *db_fd, uint32_t length, struct db_sub *sub)
 {
- uint16_t i16temp, slen;
  int rc = 0;
  struct client_chunk *cc;
 
- read_e(db_fd, &i16temp, sizeof(uint16_t));
- slen = ntohs(i16temp);
- sub->client_id = calloc(slen+1, sizeof(char));
- if(!sub->client_id){
+ rc = persist__read_string(db_fd, &sub->client_id);
+ if(rc){
+ fprintf(stderr, "Error: Corrupt persistent database.");
  fclose(db_fd);
- fprintf(stderr, "Error: Out of memory.");
- return 1;
+ return rc;
  }
- read_e(db_fd, sub->client_id, slen);
- read_e(db_fd, &i16temp, sizeof(uint16_t));
- slen = ntohs(i16temp);
- sub->topic = calloc(slen+1, sizeof(char));
- if(!sub->topic){
+
+ rc = persist__read_string(db_fd, &sub->topic);
+ if(rc){
+ fprintf(stderr, "Error: Corrupt persistent database.");
  fclose(db_fd);
- fprintf(stderr, "Error: Out of memory.");
- free(sub->client_id);
- return 1;
+ return rc;
  }
- read_e(db_fd, sub->topic, slen);
+
  read_e(db_fd, &sub->qos, sizeof(uint8_t));
 
  if(client_stats){