From f80772b8d745a71d5588cce6f15c28d5746317da Mon Sep 17 00:00:00 2001 From: Euripedes Rocha Date: Thu, 15 Dec 2022 10:50:25 +0100 Subject: [PATCH] Bugfix: Remove Remove possible null pointer dereferences - Removed a possible derefrence on data in case of MQTT5 SUBACK with MQTT5 disabled. - Covered a case of NULL data on message with negative size. - Use correct type on calloc for alpn_protos - Changed strcasecmp to strncasecmp. --- lib/mqtt_msg.c | 18 +++++++++--------- mqtt_client.c | 15 +++++++++------ 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/lib/mqtt_msg.c b/lib/mqtt_msg.c index 1ccc74f..31a5fb0 100644 --- a/lib/mqtt_msg.c +++ b/lib/mqtt_msg.c @@ -466,18 +466,18 @@ mqtt_message_t *mqtt_msg_publish(mqtt_connection_t *connection, const char *topi *message_id = 0; } - if (connection->message.length + data_length > connection->buffer_length) { - // Not enough size in buffer -> fragment this message - connection->message.fragmented_msg_data_offset = connection->message.length; - memcpy(connection->buffer + connection->message.length, data, connection->buffer_length - connection->message.length); - connection->message.length = connection->buffer_length; - connection->message.fragmented_msg_total_length = data_length + connection->message.fragmented_msg_data_offset; - } else { - if (data != NULL) { + if (data != NULL) { + if (connection->message.length + data_length > connection->buffer_length) { + // Not enough size in buffer -> fragment this message + connection->message.fragmented_msg_data_offset = connection->message.length; + memcpy(connection->buffer + connection->message.length, data, connection->buffer_length - connection->message.length); + connection->message.length = connection->buffer_length; + connection->message.fragmented_msg_total_length = data_length + connection->message.fragmented_msg_data_offset; + } else { memcpy(connection->buffer + connection->message.length, data, data_length); connection->message.length += data_length; + connection->message.fragmented_msg_total_length = 0; } - connection->message.fragmented_msg_total_length = 0; } return fini_message(connection, MQTT_MSG_TYPE_PUBLISH, 0, qos, retain); } diff --git a/mqtt_client.c b/mqtt_client.c index d732d95..9a0e7c7 100644 --- a/mqtt_client.c +++ b/mqtt_client.c @@ -240,7 +240,7 @@ static esp_err_t esp_mqtt_check_cfg_conflict(const mqtt_config_storage_t *cfg, c bool ssl_cfg_enabled = cfg->use_global_ca_store || cfg->cacert_buf || cfg->clientcert_buf || cfg->psk_hint_key || cfg->alpn_protos; bool is_ssl_scheme = false; if (cfg->scheme) { - is_ssl_scheme = (strcasecmp(cfg->scheme, MQTT_OVER_SSL_SCHEME) == 0) || (strcasecmp(cfg->scheme, MQTT_OVER_WSS_SCHEME) == 0); + is_ssl_scheme = (strncasecmp(cfg->scheme, MQTT_OVER_SSL_SCHEME, sizeof(MQTT_OVER_SSL_SCHEME)) == 0) || (strncasecmp(cfg->scheme, MQTT_OVER_WSS_SCHEME, sizeof(MQTT_OVER_WSS_SCHEME)) == 0); } if (!is_ssl_scheme && ssl_cfg_enabled) { @@ -283,12 +283,12 @@ static esp_err_t esp_mqtt_client_create_transport(esp_mqtt_client_handle_t clien client->transport_list = esp_transport_list_init(); ESP_MEM_CHECK(TAG, client->transport_list, return ESP_ERR_NO_MEM); - if ((strcasecmp(client->config->scheme, MQTT_OVER_TCP_SCHEME) == 0) || (strcasecmp(client->config->scheme, MQTT_OVER_WS_SCHEME) == 0)) { + if ((strncasecmp(client->config->scheme, MQTT_OVER_TCP_SCHEME, sizeof(MQTT_OVER_TCP_SCHEME)) == 0) || (strncasecmp(client->config->scheme, MQTT_OVER_WS_SCHEME, sizeof(MQTT_OVER_WS_SCHEME)) == 0)) { esp_transport_handle_t tcp = esp_transport_tcp_init(); ESP_MEM_CHECK(TAG, tcp, return ESP_ERR_NO_MEM); esp_transport_set_default_port(tcp, MQTT_TCP_DEFAULT_PORT); esp_transport_list_add(client->transport_list, tcp, MQTT_OVER_TCP_SCHEME); - if (strcasecmp(client->config->scheme, MQTT_OVER_WS_SCHEME) == 0) { + if (strncasecmp(client->config->scheme, MQTT_OVER_WS_SCHEME, sizeof(MQTT_OVER_WS_SCHEME)) == 0) { #if MQTT_ENABLE_WS esp_transport_handle_t ws = esp_transport_ws_init(tcp); ESP_MEM_CHECK(TAG, ws, return ESP_ERR_NO_MEM); @@ -305,13 +305,13 @@ static esp_err_t esp_mqtt_client_create_transport(esp_mqtt_client_handle_t clien ret = ESP_FAIL; #endif } - } else if ((strcasecmp(client->config->scheme, MQTT_OVER_SSL_SCHEME) == 0) || (strcasecmp(client->config->scheme, MQTT_OVER_WSS_SCHEME) == 0)) { + } else if ((strncasecmp(client->config->scheme, MQTT_OVER_SSL_SCHEME, sizeof(MQTT_OVER_SSL_SCHEME)) == 0) || (strncasecmp(client->config->scheme, MQTT_OVER_WSS_SCHEME, sizeof(MQTT_OVER_WSS_SCHEME)) == 0)) { #if MQTT_ENABLE_SSL esp_transport_handle_t ssl = esp_transport_ssl_init(); ESP_MEM_CHECK(TAG, ssl, return ESP_ERR_NO_MEM); esp_transport_set_default_port(ssl, MQTT_SSL_DEFAULT_PORT); esp_transport_list_add(client->transport_list, ssl, MQTT_OVER_SSL_SCHEME); - if (strcasecmp(client->config->scheme, MQTT_OVER_WSS_SCHEME) == 0) { + if (strncasecmp(client->config->scheme, MQTT_OVER_WSS_SCHEME, sizeof(MQTT_OVER_WSS_SCHEME)) == 0) { #if MQTT_ENABLE_WS esp_transport_handle_t wss = esp_transport_ws_init(ssl); ESP_MEM_CHECK(TAG, wss, return ESP_ERR_NO_MEM); @@ -485,7 +485,7 @@ esp_err_t esp_mqtt_set_config(esp_mqtt_client_handle_t client, const esp_mqtt_cl client->config->num_alpn_protos++; } // mbedTLS expects the list to be null-terminated - client->config->alpn_protos = calloc(client->config->num_alpn_protos + 1, sizeof(config->broker.verification.alpn_protos)); + client->config->alpn_protos = calloc(client->config->num_alpn_protos + 1, sizeof(*config->broker.verification.alpn_protos)); ESP_MEM_CHECK(TAG, client->config->alpn_protos, goto _mqtt_set_config_failed); for (int i = 0; i < client->config->num_alpn_protos; i++) { @@ -1076,6 +1076,9 @@ static esp_err_t deliver_suback(esp_mqtt_client_handle_t client) if (client->connect_info.protocol_ver == MQTT_PROTOCOL_V_5) { #ifdef MQTT_PROTOCOL_5 msg_data = mqtt5_get_suback_data(msg_buf, &msg_data_len, &client->event.property->user_property); +#else + // SUBACK Using MQTT5 received but MQTT5 is disabled, This is unlikely to happen. + return ESP_FAIL; #endif } else { msg_data = mqtt_get_suback_data(msg_buf, &msg_data_len);