Community
Participate
Working Groups
A memory leak vulnerability was found within the Mosquitto Broker (src/read_handle_server.c file), which using crafted CONNECT messages a malicious user could carry out denial of service attacks. The affected code is shown below: ====== File: src/read_handle_server.c 77 int mqtt3_handle_connect(struct mosquitto_db *db, struct mosquitto *context) 78 { 79 char *protocol_name = NULL; 80 uint8_t protocol_version; <snip> 107 108 /* Don't accept multiple CONNECT commands. */ 109 if(context->state != mosq_cs_new){ 110 rc = MOSQ_ERR_PROTOCOL; 111 goto handle_connect_error; 112 } 113 114 if(_mosquitto_read_string(&context->in_packet, &protocol_name)){ 115 rc = 1; 116 goto handle_connect_error; 117 return 1; 118 } 119 if(!protocol_name){ 120 rc = 3; 121 goto handle_connect_error; 122 return 3; 123 } 124 if(_mosquitto_read_byte(&context->in_packet, &protocol_version)){ 125 rc = 1; 126 goto handle_connect_error; 127 return 1; 128 } ====== 1) Line 77: The mqtt3_handle_connect() Mosquitto broker function parsed a CONNECT packet. (from client to broker) 2) Line 114: The “protocol_name” (memory leak) was read and stored into its pointer. 4) Line 124: The “protocol_version” pointer was tried to be stored but it failed, so the IF statement was executed. As the IF statement (line 124) did not implement the free() of the “protocol_name” pointer, it led in a memory leak vulnerability. This memory leak vulnerability could be triggered by sending the following CONNECT packet structure: ID + PACKET_LENGTH + LEN_PROTO_NAME + PROTO_NAME. The MQTT protocol name allows storing up to 0xFFFF (65.535) bytes on each CONNECT packet. An example of this vulnerability can be seen below (this memory leak vulnerability was checked against both the Mosquitto-1.4.15 and the last GIT version): ====== # valgrind --leak-check=full ./mosquitto -c mosquitto.conf > /dev/null ==19099== HEAP SUMMARY: ==19099== in use at exit: 224,103,740 bytes in 3,736 blocks ==19099== total heap usage: 31,957 allocs, 28,221 frees, 454,208,466 bytes allocated ==19099== ==19099== 5 bytes in 1 blocks are definitely lost in loss record 1 of 3 ==19099== at 0x4C2BBAF: malloc (vg_replace_malloc.c:299) ==19099== by 0x5ECC389: strdup (strdup.c:42) ==19099== by 0x40D4A5: _mosquitto_strdup (memory_mosq.c:113) ==19099== by 0x40A22B: _conf_parse_string (conf.c:1821) ==19099== by 0x4081CE: _config_read_file_core (conf.c:0) ==19099== by 0x406BE0: _config_read_file (conf.c:1771) ==19099== by 0x406BE0: mqtt3_config_read (conf.c:474) ==19099== by 0x40683A: mqtt3_config_parse_args (conf.c:338) ==19099== by 0x40475E: main (mosquitto.c:263) ==19099== ==19099== 4,140,069 bytes in 69 blocks are possibly lost in loss record 2 of 3 <<< MEM LEAK ==19099== at 0x4C2BBAF: malloc (vg_replace_malloc.c:299) ==19099== by 0x40D3E5: _mosquitto_malloc (memory_mosq.c:67) ==19099== by 0x4117D7: _mosquitto_read_string (net_mosq.c:712) ==19099== by 0x412995: mqtt3_handle_connect (read_handle_server.c:115) ==19099== by 0x411C7C: _mosquitto_packet_read (net_mosq.c:1135) ==19099== by 0x40CF7A: loop_handle_reads_writes (loop.c:522) ==19099== by 0x40CF7A: mosquitto_main_loop (loop.c:359) ==19099== by 0x404B4B: main (mosquitto.c:385) ==19099== ==19099== 219,963,666 bytes in 3,666 blocks are definitely lost in loss record 3 of 3 <<< MEM LEAK ==19099== at 0x4C2BBAF: malloc (vg_replace_malloc.c:299) ==19099== by 0x40D3E5: _mosquitto_malloc (memory_mosq.c:67) ==19099== by 0x4117D7: _mosquitto_read_string (net_mosq.c:712) ==19099== by 0x412995: mqtt3_handle_connect (read_handle_server.c:115) ==19099== by 0x411C7C: _mosquitto_packet_read (net_mosq.c:1135) ==19099== by 0x40CF7A: loop_handle_reads_writes (loop.c:522) ==19099== by 0x40CF7A: mosquitto_main_loop (loop.c:359) ==19099== by 0x404B4B: main (mosquitto.c:385) ==19099== ==19099== LEAK SUMMARY: ==19099== definitely lost: 219,963,671 bytes in 3,667 blocks ==19099== indirectly lost: 0 bytes in 0 blocks ==19099== possibly lost: 4,140,069 bytes in 69 blocks ==19099== still reachable: 0 bytes in 0 blocks ==19099== suppressed: 0 bytes in 0 blocks ==19099== ==19099== For counts of detected and suppressed errors, rerun with: -v ==19099== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0) ======
The diff patch with the knowledge I have over the code base would be the following patch, but it would be good to take a closer look to ensure I did not miss anything (e.g. double free): diff --git a/src/read_handle_server.c b/src/read_handle_server.c index a307868..8ed2142 100644 --- a/src/read_handle_server.c +++ b/src/read_handle_server.c @@ -123,6 +123,7 @@ int mqtt3_handle_connect(struct mosquitto_db *db, struct mosquitto *context) } if(_mosquitto_read_byte(&context->in_packet, &protocol_version)){ rc = 1; + _mosquitto_free(protocol_name); goto handle_connect_error; return 1; }
Thanks for the report, I confirm this is a problem. My solution would be as below. CVSS2 score 6.1: https://nvd.nist.gov/vuln-metrics/cvss/v2-calculator?vector=(AV:N/AC:L/Au:N/C:N/I:N/A:P/E:F/RL:OF/RC:C) CVSS v3 score 6.7: https://www.first.org/cvss/calculator/3.0#CVSS:3.0/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H/E:P/RL:O/RC:C diff --git a/src/read_handle_server.c b/src/read_handle_server.c index a16f205..57de06b 100644 --- a/src/read_handle_server.c +++ b/src/read_handle_server.c @@ -76,7 +76,7 @@ static char *client_id_gen(struct mosquitto_db *db) int mqtt3_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; @@ -111,20 +111,27 @@ int mqtt3_handle_connect(struct mosquitto_db *db, struct mosquitto *context) goto handle_connect_error; } - if(_mosquitto_read_string(&context->in_packet, &protocol_name)){ + /* 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(_mosquitto_read_uint16(packet, &slen)){ rc = 1; goto handle_connect_error; - return 1; } - if(!protocol_name){ - rc = 3; + if(slen != 4 /* MQTT */ && slen != 6 /* MQIsdp */){ + rc = MOSQ_ERR_PROTOCOL; + goto handle_connect_error; + } + protocol_name[slen-1] = '\0'; + + if(_mosquitto_read_bytes(&context->in_packet, protocol_name, slen)){ + rc = MOSQ_ERR_PROTOCOL; goto handle_connect_error; - return 3; } + if(_mosquitto_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){ @@ -133,7 +140,6 @@ int mqtt3_handle_connect(struct mosquitto_db *db, struct mosquitto *context) protocol_version, context->address); } _mosquitto_send_connack(context, 0, CONNACK_REFUSED_PROTOCOL_VERSION); - _mosquitto_free(protocol_name); rc = MOSQ_ERR_PROTOCOL; goto handle_connect_error; } @@ -145,13 +151,11 @@ int mqtt3_handle_connect(struct mosquitto_db *db, struct mosquitto *context) protocol_version, context->address); } _mosquitto_send_connack(context, 0, CONNACK_REFUSED_PROTOCOL_VERSION); - _mosquitto_free(protocol_name); rc = MOSQ_ERR_PROTOCOL; goto handle_connect_error; } if((context->in_packet.command&0x0F) != 0x00){ /* Reserved flags not set to 0, must disconnect. */ - _mosquitto_free(protocol_name); rc = MOSQ_ERR_PROTOCOL; goto handle_connect_error; } @@ -161,11 +165,9 @@ int mqtt3_handle_connect(struct mosquitto_db *db, struct mosquitto *context) _mosquitto_log_printf(NULL, MOSQ_LOG_INFO, "Invalid protocol \"%s\" in CONNECT from %s.", protocol_name, context->address); } - _mosquitto_free(protocol_name); rc = MOSQ_ERR_PROTOCOL; goto handle_connect_error; } - _mosquitto_free(protocol_name); if(_mosquitto_read_byte(&context->in_packet, &connect_flags)){ rc = 1;
(that is the concept code I've just quickly put together without any testing)
Thanks for your comments Roger. Obviously this is a better solution than mine ;)
Hi, Looking into the https://bugs.eclipse.org/bugs/show_bug.cgi?id=CVE-2017-7651 previous vulnerability, which leads in a same behaviour from the same attack surface, maybe the issue rate could be similar to this: https://nvd.nist.gov/vuln-metrics/cvss/v3-calculator?vector=AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H/E:F/RL:O/RC:C/CR:X/IR:X/AR:H/MAV:N/MAC:L/MPR:N/MUI:N/MS:U/MC:N/MI:N/MA:H What do you think about? On the other hand, just wondering whether a CVE is going to be request to this vulnerability? Many thanks, Daniel
To register the CVE, I need a single paragraph description and a CWE [1]. Can you provide that please? [1] https://cwe.mitre.org/
(In reply to Wayne Beaton from comment #6) > To register the CVE, I need a single paragraph description and a CWE [1]. > Can you provide that please? > > [1] https://cwe.mitre.org/ Please, find below the issue description and its CWE reference: Description: A Memory Leak vulnerability was found within the Mosquitto Broker. Unauthenticated clients can send crafted CONNECT packets which could cause a denial of service in the Mosquitto Broker. CWE: https://cwe.mitre.org/data/definitions/401.html
I'm removing the Committer-only restriction to disclose. I've assumed that this impacts version <= 1.4.15. If this is inaccurate, I can update the report. I've created a pull request https://github.com/CVEProject/cvelist/pull/569
(In reply to Wayne Beaton from comment #8) > I'm removing the Committer-only restriction to disclose. > > I've assumed that this impacts version <= 1.4.15. If this is inaccurate, I > can update the report. > > I've created a pull request https://github.com/CVEProject/cvelist/pull/569 Hi, Please, could you update the CVE information just to add who found this? >> Daniel Romero (NCC Group) Thanks, Daniel
(In reply to Daniel Romero from comment #9) > Please, could you update the CVE information just to add who found this? >> > Daniel Romero (NCC Group) There is a "credit" field that we can (and will) set in the CVE metadata, but AFAICT that information is not currently displayed on the Mitre site.
(In reply to Wayne Beaton from comment #10) > (In reply to Daniel Romero from comment #9) > > Please, could you update the CVE information just to add who found this? >> > > Daniel Romero (NCC Group) > > There is a "credit" field that we can (and will) set in the CVE metadata, > but AFAICT that information is not currently displayed on the Mitre site. Thanks for the reply. If you don't mind, another field (such as comments) could be used for this.
