Merge branch 'security-fixes-2' into 'main'

dhrishi · dhrishi · commit eda95c706631 · 2026-05-28T16:01:56.000+08:00
components: fix null dereference, resource leaks, and input validation

See merge request app-frameworks/esp-matter!1529
diff --git a/components/esp_matter/data_model/esp_matter_attribute_utils.cpp b/components/esp_matter/data_model/esp_matter_attribute_utils.cpp
@@ -672,6 +672,12 @@ bool val_compare(const esp_matter_attr_val_t *val1, const esp_matter_attr_val_t
         if (val1->val.a.s == null_len || val1->val.a.s == 0) {
             return true;
         }
+        if (val1->val.a.b == nullptr && val2->val.a.b == nullptr) {
+            return true;
+        }
+        if (val1->val.a.b == nullptr || val2->val.a.b == nullptr) {
+            return false;
+        }
         return memcmp(val1->val.a.b, val2->val.a.b, val1->val.a.s) == 0;
     }
     case ESP_MATTER_VAL_TYPE_UINT8:
diff --git a/components/esp_matter/data_model/esp_matter_data_model.cpp b/components/esp_matter/data_model/esp_matter_data_model.cpp
@@ -1326,6 +1326,7 @@ void set_user_callback(command_t *command, callback_t user_callback)
 {
     if (!command) {
         ESP_LOGE(TAG, "Command cannot be NULL");
+        return;
     }
     _command_t *current_command = (_command_t *)command;
     current_command->user_callback = user_callback;
diff --git a/components/esp_matter/data_model/private/singly_linked_list.h b/components/esp_matter/data_model/private/singly_linked_list.h
@@ -78,11 +78,13 @@ template <typename T>
 void SinglyLinkedList<T>::remove(T **head, T *target)
 {
     T **p = head;
-    while (*p != target) {
+    while (*p && *p != target) {
         p = &(*p)->next;
     }
-    *p = target->next;
-    free(target);
+    if (*p != nullptr) {
+        *p = target->next;
+        free(target);
+    }
 }
 
 template <typename T>
diff --git a/components/esp_matter_bridge/esp_matter_bridge.cpp b/components/esp_matter_bridge/esp_matter_bridge.cpp
@@ -55,6 +55,8 @@ static esp_err_t store_device_persistent_info(device_persistent_info_t *persiste
                        persistent_info, sizeof(device_persistent_info_t));
     if (err != ESP_OK) {
         ESP_LOGE(TAG, "Failed on nvs_set_blob when storing device_persistent_info");
+        nvs_close(handle);
+        return err;
     }
     err = nvs_commit(handle);
     if (err != ESP_OK) {
@@ -255,6 +257,10 @@ esp_err_t set_device_type(device_t *bridged_device, uint32_t device_type_id, voi
         ESP_LOGE(TAG, "bridged_device cannot be NULL");
         return ESP_ERR_INVALID_ARG;
     }
+    if (!device_type_callback) {
+        ESP_LOGE(TAG, "device_type_callback is NULL, call initialize() first");
+        return ESP_ERR_INVALID_STATE;
+    }
     err = device_type_callback(bridged_device->endpoint, device_type_id, priv_data);
     if (err != ESP_OK) {
         return err;
@@ -345,7 +351,9 @@ device_t *create_device(node_t *node, uint16_t parent_endpoint_id, uint32_t devi
         remove_device(dev);
         return NULL;
     }
-    store_bridged_endpoint_ids();
+    if (store_bridged_endpoint_ids() != ESP_OK) {
+        ESP_LOGE(TAG, "Failed to persist bridged endpoint IDs");
+    }
     return dev;
 }
 
diff --git a/components/esp_matter_bridge/esp_matter_bridge.h b/components/esp_matter_bridge/esp_matter_bridge.h
@@ -19,7 +19,7 @@
 #include <esp_matter_data_model.h>
 
 #define MAX_BRIDGED_DEVICE_COUNT \
-    CONFIG_ESP_MATTER_MAX_DYNAMIC_ENDPOINT_COUNT - 1 - CONFIG_ESP_MATTER_AGGREGATOR_ENDPOINT_COUNT
+    (CONFIG_ESP_MATTER_MAX_DYNAMIC_ENDPOINT_COUNT - 1 - CONFIG_ESP_MATTER_AGGREGATOR_ENDPOINT_COUNT)
 // There is an endpoint reserved as root endpoint
 
 namespace esp_matter_bridge {
diff --git a/components/esp_matter_console/esp_matter_console.cpp b/components/esp_matter_console/esp_matter_console.cpp
@@ -48,20 +48,21 @@ void engine::for_each_command(command_iterator_t *on_command, void *arg)
 
 esp_err_t engine::exec_command(int argc, char *argv[])
 {
-    esp_err_t err = ESP_ERR_INVALID_ARG;
     if (argc <= 0) {
-        return err;
+        return ESP_ERR_INVALID_ARG;
     }
+
     // find the command from the command set
     for (unsigned i = 0; i < _command_set_count; ++i) {
         for (unsigned j = 0; j < _command_set_size[i]; ++j) {
             if (strcmp(argv[0], _command_set[i][j].name) == 0 && _command_set[i][j].handler) {
-                err = _command_set[i][j].handler(argc - 1, &argv[1]);
-                break;
+                return _command_set[i][j].handler(argc - 1, &argv[1]);
             }
         }
     }
-    return err;
+
+    return ESP_ERR_INVALID_ARG;
+
 }
 
 esp_err_t engine::register_commands(const command_t *command_set, unsigned count)
diff --git a/components/esp_matter_console/esp_matter_console_attribute.cpp b/components/esp_matter_console/esp_matter_console_attribute.cpp
@@ -17,9 +17,9 @@ static esp_err_t console_set_handler(int argc, char **argv)
 {
     VerifyOrReturnError(argc >= 4, ESP_ERR_INVALID_ARG, ESP_LOGE(TAG, "The arguments for this command is invalid"));
 
-    uint16_t endpoint_id = strtoul((const char *)&argv[0][2], NULL, 16);
-    uint32_t cluster_id = strtoul((const char *)&argv[1][2], NULL, 16);
-    uint32_t attribute_id = strtoul((const char *)&argv[2][2], NULL, 16);
+    uint16_t endpoint_id = strtoul(argv[0], NULL, 0);
+    uint32_t cluster_id = strtoul(argv[1], NULL, 0);
+    uint32_t attribute_id = strtoul(argv[2], NULL, 0);
 
     attribute_t *attr = attribute::get(endpoint_id, cluster_id, attribute_id);
     if (!attr) {
@@ -158,9 +158,9 @@ static esp_err_t console_set_handler(int argc, char **argv)
 static esp_err_t console_get_handler(int argc, char **argv)
 {
     VerifyOrReturnError(argc >= 3, ESP_ERR_INVALID_ARG, ESP_LOGE(TAG, "The arguments for this command is invalid"));
-    uint16_t endpoint_id = strtoul((const char *)&argv[0][2], NULL, 16);
-    uint32_t cluster_id = strtoul((const char *)&argv[1][2], NULL, 16);
-    uint32_t attribute_id = strtoul((const char *)&argv[2][2], NULL, 16);
+    uint16_t endpoint_id = strtoul(argv[0], NULL, 0);
+    uint32_t cluster_id = strtoul(argv[1], NULL, 0);
+    uint32_t attribute_id = strtoul(argv[2], NULL, 0);
 
     attribute_t *attr = attribute::get(endpoint_id, cluster_id, attribute_id);
     if (!attr) {
diff --git a/components/esp_matter_console/esp_matter_console_udc.cpp b/components/esp_matter_console/esp_matter_console_udc.cpp
@@ -32,7 +32,8 @@ static esp_err_t send_udc_request(int argc, char *argv[])
 {
     ESP_RETURN_ON_FALSE(argc == 2, ESP_ERR_INVALID_ARG, TAG, "Incorrect arguments");
     chip::Inet::IPAddress commissioner;
-    chip::Inet::IPAddress::FromString(argv[0], commissioner);
+    ESP_RETURN_ON_FALSE(chip::Inet::IPAddress::FromString(argv[0], commissioner),
+                        ESP_ERR_INVALID_ARG, TAG, "Invalid IP address");
     uint16_t port = (uint16_t)strtol(argv[1], nullptr, 10);
     chip::Protocols::UserDirectedCommissioning::IdentificationDeclaration id;
     chip::Server::GetInstance().SendUserDirectedCommissioningRequest(
@@ -44,7 +45,8 @@ static esp_err_t send_udc_cancel(int argc, char *argv[])
 {
     ESP_RETURN_ON_FALSE(argc == 2, ESP_ERR_INVALID_ARG, TAG, "Incorrect arguments");
     chip::Inet::IPAddress commissioner;
-    chip::Inet::IPAddress::FromString(argv[0], commissioner);
+    ESP_RETURN_ON_FALSE(chip::Inet::IPAddress::FromString(argv[0], commissioner),
+                        ESP_ERR_INVALID_ARG, TAG, "Invalid IP address");
     uint16_t port = (uint16_t)strtol(argv[1], nullptr, 10);
     chip::Protocols::UserDirectedCommissioning::IdentificationDeclaration id;
     id.SetCancelPasscode(true);

Original file line number	Diff line number	Diff line change
`@@ -672,6 +672,12 @@ bool val_compare(const esp_matter_attr_val_t *val1, const esp_matter_attr_val_t`
`672`	`672`	`if (val1->val.a.s == null_len \|\| val1->val.a.s == 0) {`
`673`	`673`	`return true;`
`674`	`674`	`}`
	`675`	`+ if (val1->val.a.b == nullptr && val2->val.a.b == nullptr) {`
	`676`	`+ return true;`
	`677`	`+ }`
	`678`	`+ if (val1->val.a.b == nullptr \|\| val2->val.a.b == nullptr) {`
	`679`	`+ return false;`
	`680`	`+ }`
`675`	`681`	`return memcmp(val1->val.a.b, val2->val.a.b, val1->val.a.s) == 0;`
`676`	`682`	`}`
`677`	`683`	`case ESP_MATTER_VAL_TYPE_UINT8:`
Original file line number	Diff line number	Diff line change
`@@ -1326,6 +1326,7 @@ void set_user_callback(command_t *command, callback_t user_callback)`
`1326`	`1326`	`{`
`1327`	`1327`	`if (!command) {`
`1328`	`1328`	`ESP_LOGE(TAG, "Command cannot be NULL");`
	`1329`	`+ return;`
`1329`	`1330`	`}`
`1330`	`1331`	`_command_t current_command = (_command_t )command;`
`1331`	`1332`	`current_command->user_callback = user_callback;`
Original file line number	Diff line number	Diff line change
`@@ -78,11 +78,13 @@ template <typename T>`
`78`	`78`	`void SinglyLinkedList<T>::remove(T *head, T target)`
`79`	`79`	`{`
`80`	`80`	`T **p = head;`
`81`		`- while (*p != target) {`
	`81`	`+ while (p && p != target) {`
`82`	`82`	`p = &(*p)->next;`
`83`	`83`	`}`
`84`		`- *p = target->next;`
`85`		`- free(target);`
	`84`	`+ if (*p != nullptr) {`
	`85`	`+ *p = target->next;`
	`86`	`+ free(target);`
	`87`	`+ }`
`86`	`88`	`}`
`87`	`89`
`88`	`90`	`template <typename T>`
Original file line number	Diff line number	Diff line change
`@@ -48,20 +48,21 @@ void engine::for_each_command(command_iterator_t on_command, void arg)`
`48`	`48`
`49`	`49`	`esp_err_t engine::exec_command(int argc, char *argv[])`
`50`	`50`	`{`
`51`		`- esp_err_t err = ESP_ERR_INVALID_ARG;`
`52`	`51`	`if (argc <= 0) {`
`53`		`- return err;`
	`52`	`+ return ESP_ERR_INVALID_ARG;`
`54`	`53`	`}`
	`54`	`+`
`55`	`55`	`// find the command from the command set`
`56`	`56`	`for (unsigned i = 0; i < _command_set_count; ++i) {`
`57`	`57`	`for (unsigned j = 0; j < _command_set_size[i]; ++j) {`
`58`	`58`	`if (strcmp(argv[0], _command_set[i][j].name) == 0 && _command_set[i][j].handler) {`
`59`		`- err = _command_set[i][j].handler(argc - 1, &argv[1]);`
`60`		`- break;`
	`59`	`+ return _command_set[i][j].handler(argc - 1, &argv[1]);`
`61`	`60`	`}`
`62`	`61`	`}`
`63`	`62`	`}`
`64`		`- return err;`
	`63`	`+`
	`64`	`+ return ESP_ERR_INVALID_ARG;`
	`65`	`+`
`65`	`66`	`}`
`66`	`67`
`67`	`68`	`esp_err_t engine::register_commands(const command_t *command_set, unsigned count)`