Fix and tests for security bug #543401.

ChangeLog.txt

@@ -1,6 +1,17 @@
 1.5.6 - 201901xx
 ================
 
+Security:
+- CVE-2018-xxxxx: If Mosquitto is configured to use a password file for
+ authentication, any malformed data in the password file will be treated as
+ valid. This typically means that the malformed data becomes a username and no
+ password. If this occurs, clients can circumvent authentication and get access
+ to the broker by using the malformed username. In particular, a blank line
+ will be treated as a valid empty username. Other security measures are
+ unaffected. Users who have only used the mosquitto_passwd utility to create
+ and modify their password files are unaffected by this vulnerability.
+ Affects version 1.0 to 1.5.5 inclusive.
+
 Broker:
 - Fixed comment handling for config options that have optional arguments.
 - Improved documentation around bridge topic remapping.

src/security_default.c

@@ -623,6 +623,9 @@ static int pwfile__parse(const char *file, struct mosquitto__unpwd **root)
 
  while(!feof(pwfile)){
  if(fgets(buf, 256, pwfile)){
+ if(buf[0] == '#') continue;
+ if(!strchr(buf, ':')) continue;
+
  username = strtok_r(buf, ":", &saveptr);
  if(username){
  unpwd = mosquitto__calloc(1, sizeof(struct mosquitto__unpwd));
@@ -655,8 +658,13 @@ static int pwfile__parse(const char *file, struct mosquitto__unpwd **root)
  unpwd->password[len-1] = '\0';
  len = strlen(unpwd->password);
  }
+
+ HASH_ADD_KEYPTR(hh, *root, unpwd->username, strlen(unpwd->username), unpwd);
+ }else{
+ log__printf(NULL, MOSQ_LOG_NOTICE, "Warning: Invalid line in password file '%s': %s", file, buf);
+ mosquitto__free(unpwd->username);
+ mosquitto__free(unpwd);
  }
- HASH_ADD_KEYPTR(hh, *root, unpwd->username, strlen(unpwd->username), unpwd);
  }
  }
  }
@@ -693,34 +701,39 @@ static int unpwd__file_parse(struct mosquitto__unpwd **unpwd, const char *passwo
  token = strtok(NULL, "$");
  if(token){
  rc = base64__decode(token, &salt, &salt_len);
- if(rc){
- log__printf(NULL, MOSQ_LOG_ERR, "Error: Unable to decode password salt for user %s.", u->username);
- return MOSQ_ERR_INVAL;
- }
- u->salt = salt;
- u->salt_len = salt_len;
- token = strtok(NULL, "$");
- if(token){
- rc = base64__decode(token, &password, &password_len);
- if(rc){
- log__printf(NULL, MOSQ_LOG_ERR, "Error: Unable to decode password for user %s.", u->username);
- return MOSQ_ERR_INVAL;
+ if(rc == MOSQ_ERR_SUCCESS && salt_len == 12){
+ u->salt = salt;
+ u->salt_len = salt_len;
+ token = strtok(NULL, "$");
+ if(token){
+ rc = base64__decode(token, &password, &password_len);
+ if(rc == MOSQ_ERR_SUCCESS && password_len == 64){
+ mosquitto__free(u->password);
+ u->password = (char *)password;
+ u->password_len = password_len;
+ }else{
+ log__printf(NULL, MOSQ_LOG_ERR, "Error: Unable to decode password for user %s, removing entry.", u->username);
+ HASH_DEL(*unpwd, u);
+ }
+ }else{
+ log__printf(NULL, MOSQ_LOG_ERR, "Error: Invalid password hash for user %s, removing entry.", u->username);
+ HASH_DEL(*unpwd, u);
  }
- mosquitto__free(u->password);
- u->password = (char *)password;
- u->password_len = password_len;
  }else{
- log__printf(NULL, MOSQ_LOG_ERR, "Error: Invalid password hash for user %s.", u->username);
- return MOSQ_ERR_INVAL;
+ log__printf(NULL, MOSQ_LOG_ERR, "Error: Unable to decode password salt for user %s, removing entry.", u->username);
+ HASH_DEL(*unpwd, u);
  }
  }else{
- log__printf(NULL, MOSQ_LOG_ERR, "Error: Invalid password hash for user %s.", u->username);
- return MOSQ_ERR_INVAL;
+ log__printf(NULL, MOSQ_LOG_ERR, "Error: Invalid password hash for user %s, removing entry.", u->username);
+ HASH_DEL(*unpwd, u);
  }
  }else{
- log__printf(NULL, MOSQ_LOG_ERR, "Error: Invalid password hash for user %s.", u->username);
- return MOSQ_ERR_INVAL;
+ log__printf(NULL, MOSQ_LOG_ERR, "Error: Invalid password hash for user %s, removing entry.", u->username);
+ HASH_DEL(*unpwd, u);
  }
+ }else{
+ log__printf(NULL, MOSQ_LOG_ERR, "Error: Missing password hash for user %s, removing entry.", u->username);
+ HASH_DEL(*unpwd, u);
  }
  }
 #endif

test/broker/09-pwfile-parse-invalid.py

@@ -0,0 +1,169 @@
+#!/usr/bin/env python
+
+# Test for CVE-2018-xxxxx.
+
+import inspect, os, sys
+# From https://stackoverflow.com/questions/279237/python-import-a-module-from-a-folder
+cmd_subfolder = os.path.realpath(os.path.abspath(os.path.join(os.path.split(inspect.getfile( inspect.currentframe() ))[0],"..")))
+if cmd_subfolder not in sys.path:
+ sys.path.insert(0, cmd_subfolder)
+
+import mosq_test
+import signal
+
+def write_config(filename, port, per_listener):
+ with open(filename, 'w') as f:
+ f.write("per_listener_settings %s\n" % (per_listener))
+ f.write("port %d\n" % (port))
+ f.write("password_file %s\n" % (filename.replace('.conf', '.pwfile')))
+ f.write("allow_anonymous false")
+
+def write_pwfile(filename, bad_line1, bad_line2):
+ with open(filename, 'w') as f:
+ if bad_line1 is not None:
+ f.write('%s\n' % (bad_line1))
+ # Username test, password test
+ f.write('test:$6$njERlZMi/7DzNB9E$iiavfuXvUm8iyDZArTy7smTxh07GXXOrOsqxfW6gkOYVXHGk+W+i/8d3xDxrMwEPygEBhoA8A/gjQC0N2M4Lkw==\n')
+ # Username empty, password 0 length
+ f.write('empty:$6$o+53eGXtmlfHeYrg$FY7X9DNQ4uU1j0NiPmGOOSU05ZSzhqNmNhXIof/0nLpVb1zDhcRHdaC72E3YryH7dtTiG/r6jH6C8J+30cZBgA==\n')
+ if bad_line2 is not None:
+ f.write('%s\n' % (bad_line2))
+
+def do_test(port, connack_rc, username, password):
+ rc = 1
+ keepalive = 60
+ connect_packet = mosq_test.gen_connect("username-password-check", keepalive=keepalive, username=username, password=password)
+ connack_packet = mosq_test.gen_connack(rc=connack_rc)
+
+ try:
+ sock = mosq_test.do_client_connect(connect_packet, connack_packet, port=port)
+ rc = 0
+ sock.close()
+ finally:
+ if rc:
+ exit(rc)
+
+
+def username_password_tests(port):
+ broker = mosq_test.start_broker(filename=os.path.basename(__file__), use_conf=True, port=port)
+
+ try:
+ do_test(port, connack_rc=0, username='test', password='test')
+ do_test(port, connack_rc=5, username='test', password='bad')
+ do_test(port, connack_rc=5, username='test', password='')
+ do_test(port, connack_rc=5, username='test', password=None)
+ do_test(port, connack_rc=5, username='empty', password='test')
+ do_test(port, connack_rc=5, username='empty', password='bad')
+ do_test(port, connack_rc=0, username='empty', password='')
+ do_test(port, connack_rc=5, username='empty', password=None)
+ do_test(port, connack_rc=5, username='bad', password='test')
+ do_test(port, connack_rc=5, username='bad', password='bad')
+ do_test(port, connack_rc=5, username='bad', password='')
+ do_test(port, connack_rc=5, username='bad', password=None)
+ do_test(port, connack_rc=5, username='', password='test')
+ do_test(port, connack_rc=5, username='', password='bad')
+ do_test(port, connack_rc=5, username='', password='')
+ do_test(port, connack_rc=5, username='', password=None)
+ do_test(port, connack_rc=5, username=None, password='test')
+ do_test(port, connack_rc=5, username=None, password='bad')
+ do_test(port, connack_rc=5, username=None, password='')
+ do_test(port, connack_rc=5, username=None, password=None)
+ except ValueError:
+ pass
+ finally:
+ broker.terminate()
+ broker.wait()
+
+
+def all_tests(port):
+ # Valid file, single user
+ write_pwfile(pw_file, bad_line1=None, bad_line2=None)
+ username_password_tests(port)
+
+ # Invalid file, first line blank
+ write_pwfile(pw_file, bad_line1='', bad_line2=None)
+ username_password_tests(port)
+
+ # Invalid file, last line blank
+ write_pwfile(pw_file, bad_line1=None, bad_line2='')
+ username_password_tests(port)
+
+ # Invalid file, first and last line blank
+ write_pwfile(pw_file, bad_line1='', bad_line2='')
+ username_password_tests(port)
+
+ # Invalid file, first line 'comment'
+ write_pwfile(pw_file, bad_line1='#comment', bad_line2=None)
+ username_password_tests(port)
+
+ # Invalid file, last line 'comment'
+ write_pwfile(pw_file, bad_line1=None, bad_line2='#comment')
+ username_password_tests(port)
+
+ # Invalid file, first and last line 'comment'
+ write_pwfile(pw_file, bad_line1='#comment', bad_line2='#comment')
+ username_password_tests(port)
+
+ # Invalid file, first line blank and last line 'comment'
+ write_pwfile(pw_file, bad_line1='', bad_line2='#comment')
+ username_password_tests(port)
+
+ # Invalid file, first line incomplete
+ write_pwfile(pw_file, bad_line1='bad:', bad_line2=None)
+ username_password_tests(port)
+
+ # Invalid file, first line incomplete, but with "password"
+ write_pwfile(pw_file, bad_line1='bad:bad', bad_line2=None)
+ username_password_tests(port)
+
+ # Invalid file, first line incomplete, partial password hash
+ write_pwfile(pw_file, bad_line1='bad:$', bad_line2=None)
+ username_password_tests(port)
+
+ # Invalid file, first line incomplete, partial password hash
+ write_pwfile(pw_file, bad_line1='bad:$6', bad_line2=None)
+ username_password_tests(port)
+
+ # Invalid file, first line incomplete, partial password hash
+ write_pwfile(pw_file, bad_line1='bad:$6$', bad_line2=None)
+ username_password_tests(port)
+
+ # Valid file, first line incomplete, has valid salt but no password hash
+ write_pwfile(pw_file, bad_line1='bad:$6$njERlZMi/7DzNB9E', bad_line2=None)
+ username_password_tests(port)
+
+ # Valid file, first line incomplete, has valid salt but no password hash
+ write_pwfile(pw_file, bad_line1='bad:$6$njERlZMi/7DzNB9E$', bad_line2=None)
+ username_password_tests(port)
+
+ # Valid file, first line has invalid hash designator
+ write_pwfile(pw_file, bad_line1='bad:$5$njERlZMi/7DzNB9E$iiavfuXvUm8iyDZArTy7smTxh07GXXOrOsqxfW6gkOYVXHGk+W+i/8d3xDxrMwEPygEBhoA8A/gjQC0N2M4Lkw==', bad_line2=None)
+ username_password_tests(port)
+
+ # Invalid file, missing username but valid password hash
+ write_pwfile(pw_file, bad_line1=':$6$njERlZMi/7DzNB9E$iiavfuXvUm8iyDZArTy7smTxh07GXXOrOsqxfW6gkOYVXHGk+W+i/8d3xDxrMwEPygEBhoA8A/gjQC0N2M4Lkw==', bad_line2=None)
+ username_password_tests(port)
+
+ # Valid file, valid username but password salt not base64
+ write_pwfile(pw_file, bad_line1='bad:$6$njER{ZMi/7DzNB9E$iiavfuXvUm8iyDZArTy7smTxh07GXXOrOsqxfW6gkOYVXHGk+W+i/8d3xDxrMwEPygEBhoA8A/gjQC0N2M4Lkw==', bad_line2=None)
+ username_password_tests(port)
+
+ # Valid file, valid username but password hash not base64
+ write_pwfile(pw_file, bad_line1='bad:$6$njERlZMi/7DzNB9E$iiavfuXv{}8iyDZArTy7smTxh07GXXOrOsqxfW6gkOYVXHGk+W+i/8d3xDxrMwEPygEBhoA8A/gjQC0N2M4Lkw==', bad_line2=None)
+ username_password_tests(port)
+
+
+
+port = mosq_test.get_port()
+conf_file = os.path.basename(__file__).replace('.py', '.conf')
+pw_file = os.path.basename(__file__).replace('.py', '.pwfile')
+
+try:
+ write_config(conf_file, port, "false")
+ all_tests(port)
+
+ write_config(conf_file, port, "true")
+ all_tests(port)
+finally:
+ os.remove(conf_file)
+ os.remove(pw_file)

test/broker/Makefile

@@ -128,6 +128,7 @@ endif
  ./09-plugin-auth-defer-unpwd-fail.py
  ./09-plugin-auth-msg-params.py
  ./09-plugin-auth-context-params.py
+ ./09-pwfile-parse-invalid.py
 
 10 :
  ./10-listener-mount-point.py

test/broker/ptest.py

@@ -100,6 +100,7 @@
  (1, './09-plugin-auth-defer-unpwd-fail.py'),
  (1, './09-plugin-auth-msg-params.py'),
  (1, './09-plugin-auth-context-params.py'),
+ (1, './09-pwfile-parse-invalid.py'),
 
  (2, './10-listener-mount-point.py'),
 

test/mosq_test.py

@@ -6,7 +6,7 @@
 import sys
 import time
 
-def start_broker(filename, cmd=None, port=0, use_conf=False):
+def start_broker(filename, cmd=None, port=0, use_conf=False, expect_fail=False):
  delay = 0.1
 
  if use_conf == True:
@@ -43,7 +43,11 @@ def start_broker(filename, cmd=None, port=0, use_conf=False):
  c.close()
  time.sleep(delay)
  return broker
- raise IOError
+
+ if expect_fail == False:
+ raise IOError
+ else:
+ return None
 
 def start_client(filename, cmd, env, port=1888):
  if cmd is None: