diff --git a/ChangeLog.txt b/ChangeLog.txt index 6bb4cae9..c7146ff5 100644 --- a/ChangeLog.txt +++ b/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. diff --git a/src/security_default.c b/src/security_default.c index 26d8b59e..743020c1 100644 --- a/src/security_default.c +++ b/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 diff --git a/test/broker/09-pwfile-parse-invalid.py b/test/broker/09-pwfile-parse-invalid.py new file mode 100755 index 00000000..6a865234 --- /dev/null +++ b/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 http://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) diff --git a/test/broker/Makefile b/test/broker/Makefile index 08f1b0ff..3299d7db 100644 --- a/test/broker/Makefile +++ b/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 diff --git a/test/broker/ptest.py b/test/broker/ptest.py index a9d9a9a0..eb10895d 100755 --- a/test/broker/ptest.py +++ b/test/broker/ptest.py @@ -100,6 +100,7 @@ tests = [ (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'), diff --git a/test/mosq_test.py b/test/mosq_test.py index a28f2eb1..10323323 100644 --- a/test/mosq_test.py +++ b/test/mosq_test.py @@ -6,7 +6,7 @@ import struct 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: