From b4dfeb3767b595e3247640b7e8558935eb55d2e4 Mon Sep 17 00:00:00 2001 From: "Roger A. Light" Date: Tue, 21 May 2019 23:56:22 +0100 Subject: [PATCH] Fix MQTT v5 clients not being able to specify a password without a username. Thanks to Erik Moqvist. Closes #1274. --- ChangeLog.txt | 2 ++ client/client_shared.c | 7 ++----- client/pub_client.c | 2 +- client/rr_client.c | 2 +- client/sub_client.c | 2 +- lib/options.c | 21 ++++++++++++++------- lib/send_connect.c | 29 +++++++++++++++++++--------- man/mosquitto_pub.1.xml | 10 ++++------ man/mosquitto_rr.1.xml | 10 ++++------ man/mosquitto_sub.1.xml | 10 ++++------ src/handle_connect.c | 42 ++++++++++++++++++++--------------------- 11 files changed, 73 insertions(+), 64 deletions(-) diff --git a/ChangeLog.txt b/ChangeLog.txt index e721d48a92..c64373ea34 100644 --- a/ChangeLog.txt +++ b/ChangeLog.txt @@ -28,6 +28,8 @@ Clients: - Fix mosquitto_pub exiting with error code 0 when an error occurred. Closes #1285. - Fix mosquitto_pub not using the `-c` option. Closes #1273. +- Fix MQTT v5 clients not being able to specify a password without a username. + Closes #1274. 1.6.2 - 20190430 diff --git a/client/client_shared.c b/client/client_shared.c index 0205ce8d66..ede8baa1d9 100644 --- a/client/client_shared.c +++ b/client/client_shared.c @@ -325,9 +325,6 @@ int client_config_load(struct mosq_config *cfg, int pub_or_sub, int argc, char * fprintf(stderr, "Error: Will retain given, but no will topic given.\n"); return 1; } - if(cfg->password && !cfg->username){ - err_printf(cfg, "Warning: Not using password since username not set.\n"); - } #ifdef WITH_TLS if((cfg->certfile && !cfg->keyfile) || (cfg->keyfile && !cfg->certfile)){ fprintf(stderr, "Error: Both certfile and keyfile must be provided if one of them is set.\n"); @@ -1084,8 +1081,8 @@ int client_opts_set(struct mosquitto *mosq, struct mosq_config *cfg) } cfg->will_props = NULL; - if(cfg->username && mosquitto_username_pw_set(mosq, cfg->username, cfg->password)){ - err_printf(cfg, "Error: Problem setting username and password.\n"); + if((cfg->username || cfg->password) && mosquitto_username_pw_set(mosq, cfg->username, cfg->password)){ + err_printf(cfg, "Error: Problem setting username and/or password.\n"); mosquitto_lib_cleanup(); return 1; } diff --git a/client/pub_client.c b/client/pub_client.c index 40310225e1..f5521eac96 100644 --- a/client/pub_client.c +++ b/client/pub_client.c @@ -329,7 +329,7 @@ void print_usage(void) mosquitto_lib_version(&major, &minor, &revision); printf("mosquitto_pub is a simple mqtt client that will publish a message on a single topic and exit.\n"); printf("mosquitto_pub version %s running on libmosquitto %d.%d.%d.\n\n", VERSION, major, minor, revision); - printf("Usage: mosquitto_pub {[-h host] [-p port] [-u username [-P password]] -t topic | -L URL}\n"); + printf("Usage: mosquitto_pub {[-h host] [-p port] [-u username] [-P password] -t topic | -L URL}\n"); printf(" {-f file | -l | -n | -m message}\n"); printf(" [-c] [-k keepalive] [-q qos] [-r] [--repeat N] [--repeat-delay time]\n"); #ifdef WITH_SRV diff --git a/client/rr_client.c b/client/rr_client.c index cbd32d5eb8..6444c28e09 100644 --- a/client/rr_client.c +++ b/client/rr_client.c @@ -156,7 +156,7 @@ void print_usage(void) printf(" Defaults to MQTT v5, where the Request-Response feature will be used, but v3.1.1 can also be used\n"); printf(" with v3.1.1 brokers.\n"); printf("mosquitto_rr version %s running on libmosquitto %d.%d.%d.\n\n", VERSION, major, minor, revision); - printf("Usage: mosquitto_rr {[-h host] [-p port] [-u username [-P password]] -t topic | -L URL} -e response-topic\n"); + printf("Usage: mosquitto_rr {[-h host] [-p port] [-u username] [-P password] -t topic | -L URL} -e response-topic\n"); printf(" [-c] [-k keepalive] [-q qos] [-R]\n"); printf(" [-F format]\n"); #ifndef WIN32 diff --git a/client/sub_client.c b/client/sub_client.c index a10757c6ab..f57a48cce5 100644 --- a/client/sub_client.c +++ b/client/sub_client.c @@ -168,7 +168,7 @@ void print_usage(void) mosquitto_lib_version(&major, &minor, &revision); printf("mosquitto_sub is a simple mqtt client that will subscribe to a set of topics and print all messages it receives.\n"); printf("mosquitto_sub version %s running on libmosquitto %d.%d.%d.\n\n", VERSION, major, minor, revision); - printf("Usage: mosquitto_sub {[-h host] [-p port] [-u username [-P password]] -t topic | -L URL [-t topic]}\n"); + printf("Usage: mosquitto_sub {[-h host] [-p port] [-u username] [-P password] -t topic | -L URL [-t topic]}\n"); printf(" [-c] [-k keepalive] [-q qos]\n"); printf(" [-C msg_count] [-E] [-R] [--retained-only] [--remove-retained] [-T filter_out] [-U topic ...]\n"); printf(" [-F format]\n"); diff --git a/lib/options.c b/lib/options.c index 005b781018..06c5de00b6 100644 --- a/lib/options.c +++ b/lib/options.c @@ -69,6 +69,12 @@ int mosquitto_username_pw_set(struct mosquitto *mosq, const char *username, cons { if(!mosq) return MOSQ_ERR_INVAL; + if(mosq->protocol == mosq_p_mqtt311 || mosq->protocol == mosq_p_mqtt31){ + if(password != NULL && username == NULL){ + return MOSQ_ERR_INVAL; + } + } + mosquitto__free(mosq->username); mosq->username = NULL; @@ -81,13 +87,14 @@ int mosquitto_username_pw_set(struct mosquitto *mosq, const char *username, cons } mosq->username = mosquitto__strdup(username); if(!mosq->username) return MOSQ_ERR_NOMEM; - if(password){ - mosq->password = mosquitto__strdup(password); - if(!mosq->password){ - mosquitto__free(mosq->username); - mosq->username = NULL; - return MOSQ_ERR_NOMEM; - } + } + + if(password){ + mosq->password = mosquitto__strdup(password); + if(!mosq->password){ + mosquitto__free(mosq->username); + mosq->username = NULL; + return MOSQ_ERR_NOMEM; } } return MOSQ_ERR_SUCCESS; diff --git a/lib/send_connect.c b/lib/send_connect.c index 60c529a5f3..210f125a80 100644 --- a/lib/send_connect.c +++ b/lib/send_connect.c @@ -111,11 +111,21 @@ int send__connect(struct mosquitto *mosq, uint16_t keepalive, bool clean_session payloadlen += will_proplen + varbytes; } } + + /* After this check we can be sure that the username and password are + * always valid for the current protocol, so there is no need to check + * username before checking password. */ + if(mosq->protocol == mosq_p_mqtt31 || mosq->protocol == mosq_p_mqtt311){ + if(password != NULL && username == NULL){ + return MOSQ_ERR_INVAL; + } + } + if(username){ payloadlen += 2+strlen(username); - if(password){ - payloadlen += 2+strlen(password); - } + } + if(password){ + payloadlen += 2+strlen(password); } packet->command = CMD_CONNECT; @@ -145,9 +155,9 @@ int send__connect(struct mosquitto *mosq, uint16_t keepalive, bool clean_session } if(username){ byte = byte | 0x1<<7; - if(mosq->password){ - byte = byte | 0x1<<6; - } + } + if(mosq->password){ + byte = byte | 0x1<<6; } packet__write_byte(packet, byte); packet__write_uint16(packet, keepalive); @@ -173,11 +183,12 @@ int send__connect(struct mosquitto *mosq, uint16_t keepalive, bool clean_session packet__write_string(packet, mosq->will->msg.topic, strlen(mosq->will->msg.topic)); packet__write_string(packet, (const char *)mosq->will->msg.payload, mosq->will->msg.payloadlen); } + if(username){ packet__write_string(packet, username, strlen(username)); - if(password){ - packet__write_string(packet, password, strlen(password)); - } + } + if(password){ + packet__write_string(packet, password, strlen(password)); } mosq->keepalive = keepalive; diff --git a/man/mosquitto_pub.1.xml b/man/mosquitto_pub.1.xml index 46bf09baf1..2c1a425c27 100644 --- a/man/mosquitto_pub.1.xml +++ b/man/mosquitto_pub.1.xml @@ -21,10 +21,8 @@ hostname port-number - - username - password - + username + password message-topic URL @@ -366,8 +364,8 @@ Provide a password to be used for authenticating with the broker. Using this argument without also specifying - a username is invalid. See also the - option. + a username is invalid when using MQTT v3.1 or v3.1.1. + See also the option. diff --git a/man/mosquitto_rr.1.xml b/man/mosquitto_rr.1.xml index 0828a5a2d8..5af06af09f 100644 --- a/man/mosquitto_rr.1.xml +++ b/man/mosquitto_rr.1.xml @@ -21,10 +21,8 @@ hostname port-number - - username - password - + username + password message-topic @@ -387,8 +385,8 @@ Provide a password to be used for authenticating with the broker. Using this argument without also specifying - a username is invalid. See also the - option. + a username is invalid when using MQTT v3.1 or v3.1.1. + See also the option. diff --git a/man/mosquitto_sub.1.xml b/man/mosquitto_sub.1.xml index 0547288fcd..83a500ac4c 100644 --- a/man/mosquitto_sub.1.xml +++ b/man/mosquitto_sub.1.xml @@ -21,10 +21,8 @@ hostname port-number - - username - password - + username + password message-topic @@ -396,8 +394,8 @@ Provide a password to be used for authenticating with the broker. Using this argument without also specifying - a username is invalid. See also the - option. + a username is invalid when using MQTT v3.1 or v3.1.1. + See also the option. diff --git a/src/handle_connect.c b/src/handle_connect.c index 10fd9334dc..7fc160e954 100644 --- a/src/handle_connect.c +++ b/src/handle_connect.c @@ -586,25 +586,10 @@ int handle__connect(struct mosquitto_db *db, struct mosquitto *context) if(username_flag){ rc = packet__read_string(&context->in_packet, &username, &slen); - if(rc == MOSQ_ERR_SUCCESS){ - if(password_flag){ - rc = packet__read_binary(&context->in_packet, (uint8_t **)&password, &slen); - if(rc == MOSQ_ERR_NOMEM){ - rc = MOSQ_ERR_NOMEM; - goto handle_connect_error; - }else if(rc == MOSQ_ERR_PROTOCOL){ - if(context->protocol == mosq_p_mqtt31){ - /* Password flag given, but no password. Ignore. */ - }else{ - rc = MOSQ_ERR_PROTOCOL; - goto handle_connect_error; - } - } - } - }else if(rc == MOSQ_ERR_NOMEM){ + if(rc == MOSQ_ERR_NOMEM){ rc = MOSQ_ERR_NOMEM; goto handle_connect_error; - }else{ + }else if(rc != MOSQ_ERR_SUCCESS){ if(context->protocol == mosq_p_mqtt31){ /* Username flag given, but no username. Ignore. */ username_flag = 0; @@ -614,7 +599,7 @@ int handle__connect(struct mosquitto_db *db, struct mosquitto *context) } } }else{ - if(context->protocol == mosq_p_mqtt311 || context->protocol == mosq_p_mqtt5){ + if(context->protocol == mosq_p_mqtt311 || context->protocol == mosq_p_mqtt31){ if(password_flag){ /* username_flag == 0 && password_flag == 1 is forbidden */ log__printf(NULL, MOSQ_LOG_ERR, "Protocol error from %s: password without username, closing connection.", client_id); @@ -623,6 +608,21 @@ int handle__connect(struct mosquitto_db *db, struct mosquitto *context) } } } + if(password_flag){ + rc = packet__read_binary(&context->in_packet, (uint8_t **)&password, &slen); + if(rc == MOSQ_ERR_NOMEM){ + rc = MOSQ_ERR_NOMEM; + goto handle_connect_error; + }else if(rc == MOSQ_ERR_PROTOCOL){ + if(context->protocol == mosq_p_mqtt31){ + /* Password flag given, but no password. Ignore. */ + }else{ + rc = MOSQ_ERR_PROTOCOL; + goto handle_connect_error; + } + } + } + if(context->in_packet.pos != context->in_packet.remaining_length){ /* Surplus data at end of packet, this must be an error. */ rc = MOSQ_ERR_PROTOCOL; @@ -755,7 +755,7 @@ int handle__connect(struct mosquitto_db *db, struct mosquitto *context) #endif /* FINAL_WITH_TLS_PSK */ }else{ #endif /* WITH_TLS */ - if(username_flag){ + if(username_flag || password_flag){ /* FIXME - these ensure the mosquitto_client_id() and * mosquitto_client_username() functions work, but is hacky */ context->id = client_id; @@ -786,9 +786,7 @@ int handle__connect(struct mosquitto_db *db, struct mosquitto *context) context->password = password; username = NULL; /* Avoid free() in error: below. */ password = NULL; - } - - if(!username_flag){ + }else{ if((db->config->per_listener_settings && context->listener->security_options.allow_anonymous == false) || (!db->config->per_listener_settings && db->config->security_options.allow_anonymous == false)){