Summary: | CVE-2017-7654: Mosquitto Broker DoS through a Memory Leak vulnerability | ||
---|---|---|---|
Product: | Community | Reporter: | Daniel Romero <daniel.romero> |
Component: | Vulnerability Reports | Assignee: | Security vulnerabilitied reported against Eclipse projects <vulnerability.reports-inbox> |
Status: | RESOLVED FIXED | QA Contact: | |
Severity: | critical | ||
Priority: | P3 | CC: | brian, jreimann, roger, wayne.beaton |
Version: | unspecified | Keywords: | security |
Target Milestone: | --- | ||
Hardware: | All | ||
OS: | All | ||
Whiteboard: |
Description
Daniel Romero
2018-04-12 06:29:34 EDT
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. |