diff --git a/ChangeLog.txt b/ChangeLog.txt index 7fc02fb9..2a6a6a19 100644 --- a/ChangeLog.txt +++ b/ChangeLog.txt @@ -1,6 +1,10 @@ 1.5 - 2018xxxx ============== +Security: +- Fix memory leak that could be caused by a malicious CONNECT packet. This + does not yet have a CVE assigned. Closes #533493 (on Eclipse bugtracker) + Broker features: - Add per_listener_settings to allow authentication and access control to be per listener. diff --git a/src/handle_connect.c b/src/handle_connect.c index 86d06ef8..4246af16 100644 --- a/src/handle_connect.c +++ b/src/handle_connect.c @@ -108,7 +108,7 @@ void connection_check_acl(struct mosquitto_db *db, struct mosquitto *context, st int handle__connect(struct mosquitto_db *db, struct mosquitto *context) { - char *protocol_name = NULL; + char protocol_name[7]; uint8_t protocol_version; uint8_t connect_flags; uint8_t connect_ack = 0; @@ -124,6 +124,7 @@ int handle__connect(struct mosquitto_db *db, struct mosquitto *context) struct mosquitto__acl_user *acl_tail; struct mosquitto *found_context; int slen; + uint16_t slen16; struct mosquitto__subleaf *leaf; int i; struct mosquitto__security_options *security_opts; @@ -145,20 +146,27 @@ int handle__connect(struct mosquitto_db *db, struct mosquitto *context) goto handle_connect_error; } - if(packet__read_string(&context->in_packet, &protocol_name, &slen)){ + /* Read protocol name as length then bytes rather than with read_string + * because the length is fixed and we can check that. Removes the need + * for another malloc as well. */ + if(packet__read_uint16(&context->in_packet, &slen16)){ rc = 1; goto handle_connect_error; - return 1; } - if(!protocol_name){ - rc = 3; + slen = slen16; + if(slen != 4 /* MQTT */ && slen != 6 /* MQIsdp */){ + rc = MOSQ_ERR_PROTOCOL; goto handle_connect_error; - return 3; } + if(packet__read_bytes(&context->in_packet, protocol_name, slen)){ + rc = MOSQ_ERR_PROTOCOL; + goto handle_connect_error; + } + protocol_name[slen] = '\0'; + if(packet__read_byte(&context->in_packet, &protocol_version)){ rc = 1; goto handle_connect_error; - return 1; } if(!strcmp(protocol_name, PROTOCOL_NAME_v31)){ if((protocol_version&0x7F) != PROTOCOL_VERSION_v31){ @@ -195,8 +203,6 @@ int handle__connect(struct mosquitto_db *db, struct mosquitto *context) rc = MOSQ_ERR_PROTOCOL; goto handle_connect_error; } - mosquitto__free(protocol_name); - protocol_name = NULL; if(packet__read_byte(&context->in_packet, &connect_flags)){ rc = 1; @@ -672,7 +678,6 @@ handle_connect_error: mosquitto__free(will_payload); mosquitto__free(will_topic); mosquitto__free(will_struct); - mosquitto__free(protocol_name); #ifdef WITH_TLS if(client_cert) X509_free(client_cert); #endif