[PATCH BlueZ v2 0/5] Use composition data to validate app against the node

[PATCH BlueZ v2 0/5] Use composition data to validate app against the node

[Michał Lowas-Rzechonek]

This patchset streamlines app validation by creating a temporary node
during Attach, Join and CreateNetwork calls, then validating its
composition data to:
 - fit in Config Model Composition Data Get message
 - declare mandatory models on primary element
 - declare consecutive element indexes

During Attach call, temporary composition data is also compared with
data generated for existing node, guaranteeing immutablity required by
the specification.

Michał Lowas-Rzechonek (5):
  mesh: Convert void pointers to anonymous unions in managed_obj_request
  mesh: Validate application by comparing composition data
  mesh: Keep element and model lists sorted and unique
  mesh: Check that element indexes are consecutive
  mesh: Check that config server is present in primary element

 mesh/mesh-defs.h |   2 +
 mesh/node.c      | 507 ++++++++++++++++++++++-------------------------
 2 files changed, 241 insertions(+), 268 deletions(-)

-- 
2.19.1

[PATCH BlueZ v2 1/5] mesh: Convert void pointers to anonymous unions in managed_obj_request

[Michał Lowas-Rzechonek]

---
 mesh/node.c | 75 +++++++++++++++++++++++++----------------------------
 1 file changed, 35 insertions(+), 40 deletions(-)

diff --git a/mesh/node.c b/mesh/node.c
index ceb46e4df..e51913edf 100644
--- a/mesh/node.c
+++ b/mesh/node.c
@@ -58,9 +58,11 @@
 #define DEFAULT_CRPL 10
 #define DEFAULT_SEQUENCE_NUMBER 0
 
-#define REQUEST_TYPE_JOIN 0
-#define REQUEST_TYPE_ATTACH 1
-#define REQUEST_TYPE_CREATE 2
+enum request_type {
+	REQUEST_TYPE_JOIN,
+	REQUEST_TYPE_ATTACH,
+	REQUEST_TYPE_CREATE,
+};
 
 struct node_element {
 	char *path;
@@ -110,10 +112,16 @@ struct mesh_node {
 };
 
 struct managed_obj_request {
-	void *data;
-	void *cb;
-	void *user_data;
-	uint8_t type;
+	union {
+		const uint8_t *uuid;
+		struct mesh_node *node;
+	};
+	union {
+		node_ready_func_t ready_cb;
+		node_join_ready_func_t join_ready_cb;
+	};
+	struct l_dbus_message *pending_msg;
+	enum request_type type;
 };
 
 static struct l_queue *nodes;
@@ -1571,7 +1579,7 @@ static void get_managed_objects_cb(struct l_dbus_message *msg, void *user_data)
 		node = l_new(struct mesh_node, 1);
 		node->elements = l_queue_new();
 	} else {
-		node = req->data;
+		node = req->node;
 	}
 
 	num_ele = 0;
@@ -1642,8 +1650,6 @@ static void get_managed_objects_cb(struct l_dbus_message *msg, void *user_data)
 	}
 
 	if (req->type == REQUEST_TYPE_ATTACH) {
-		node_ready_func_t cb = req->cb;
-
 		if (num_ele != node->num_ele)
 			goto fail;
 
@@ -1652,7 +1658,7 @@ static void get_managed_objects_cb(struct l_dbus_message *msg, void *user_data)
 
 			node->disc_watch = l_dbus_add_disconnect_watch(bus,
 					node->owner, app_disc_cb, node, NULL);
-			cb(req->user_data, MESH_ERROR_NONE, node);
+			req->ready_cb(req->pending_msg, MESH_ERROR_NONE, node);
 		} else
 			goto fail;
 
@@ -1665,8 +1671,6 @@ static void get_managed_objects_cb(struct l_dbus_message *msg, void *user_data)
 		init_storage_dir(node);
 
 	} else if (req->type == REQUEST_TYPE_JOIN) {
-		node_join_ready_func_t cb = req->cb;
-
 		if (!agent) {
 			l_error("Interface %s not found",
 						MESH_PROVISION_AGENT_INTERFACE);
@@ -1675,22 +1679,20 @@ static void get_managed_objects_cb(struct l_dbus_message *msg, void *user_data)
 
 		node->num_ele = num_ele;
 		set_defaults(node);
-		memcpy(node->uuid, req->data, 16);
+		memcpy(node->uuid, req->uuid, 16);
 
 		if (!create_node_config(node, node->uuid))
 			goto fail;
 
-		cb(node, agent);
-
+		req->join_ready_cb(node, agent);
 	} else {
 		/* Callback for create node request */
-		node_ready_func_t cb = req->cb;
 		struct keyring_net_key net_key;
 		uint8_t dev_key[16];
 
 		node->num_ele = num_ele;
 		set_defaults(node);
-		memcpy(node->uuid, req->data, 16);
+		memcpy(node->uuid, req->uuid, 16);
 
 		if (!create_node_config(node, node->uuid))
 			goto fail;
@@ -1717,7 +1719,7 @@ static void get_managed_objects_cb(struct l_dbus_message *msg, void *user_data)
 		if (!keyring_put_net_key(node, PRIMARY_NET_IDX, &net_key))
 			goto fail;
 
-		cb(req->user_data, MESH_ERROR_NONE, node);
+		req->ready_cb(req->pending_msg, MESH_ERROR_NONE, node);
 	}
 
 	return;
@@ -1726,26 +1728,19 @@ fail:
 		mesh_agent_remove(agent);
 
 	if (!is_new) {
-		/* Handle failed Attach request */
-		node_ready_func_t cb = req->cb;
-
 		free_node_dbus_resources(node);
 
-		cb(req->user_data, MESH_ERROR_FAILED, node);
+		req->ready_cb(req->pending_msg, MESH_ERROR_FAILED, node);
 	} else {
 		/* Handle failed Join and Create requests */
 		if (node)
 			node_remove(node);
 
-		if (req->type == REQUEST_TYPE_JOIN) {
-			node_join_ready_func_t cb = req->cb;
-
-			cb(NULL, NULL);
-		} else {
-			node_ready_func_t cb = req->cb;
-
-			cb(req->user_data, MESH_ERROR_FAILED, NULL);
-		}
+		if (req->type == REQUEST_TYPE_JOIN)
+			req->join_ready_cb(NULL, NULL);
+		else
+			req->ready_cb(req->pending_msg, MESH_ERROR_FAILED,
+									NULL);
 	}
 }
 
@@ -1770,9 +1765,9 @@ int node_attach(const char *app_path, const char *sender, uint64_t token,
 	node->owner = l_strdup(sender);
 
 	req = l_new(struct managed_obj_request, 1);
-	req->data = node;
-	req->cb = cb;
-	req->user_data = user_data;
+	req->node = node;
+	req->ready_cb = cb;
+	req->pending_msg = user_data;
 	req->type = REQUEST_TYPE_ATTACH;
 
 	l_dbus_method_call(dbus_get_bus(), sender, app_path,
@@ -1794,8 +1789,8 @@ void node_join(const char *app_path, const char *sender, const uint8_t *uuid,
 	l_debug("");
 
 	req = l_new(struct managed_obj_request, 1);
-	req->data = (void *) uuid;
-	req->cb = cb;
+	req->uuid = uuid;
+	req->join_ready_cb = cb;
 	req->type = REQUEST_TYPE_JOIN;
 
 	l_dbus_method_call(dbus_get_bus(), sender, app_path,
@@ -1813,9 +1808,9 @@ void node_create(const char *app_path, const char *sender, const uint8_t *uuid,
 	l_debug("");
 
 	req = l_new(struct managed_obj_request, 1);
-	req->data = (void *) uuid;
-	req->cb = cb;
-	req->user_data = user_data;
+	req->uuid = uuid;
+	req->ready_cb = cb;
+	req->pending_msg = user_data;
 	req->type = REQUEST_TYPE_CREATE;
 
 	l_dbus_method_call(dbus_get_bus(), sender, app_path,
-- 
2.19.1

[PATCH BlueZ v2 2/5] mesh: Validate application by comparing composition data

[Michał Lowas-Rzechonek]

Instead of validating application by enumerating D-Bus objects, create a
temporary node instance and check if composition data generated for the
temporary matches the node loaded from storage.

This allows node validation logic (primary element, mandatory models etc)
to be confined in node_generate_comp() function.

This also streamlines code implementing Attach(), Join(),
CreateNetwork() and ImportLocalNode() calls.
---
 mesh/mesh-defs.h |   2 +
 mesh/node.c      | 385 ++++++++++++++++++-----------------------------
 2 files changed, 147 insertions(+), 240 deletions(-)

diff --git a/mesh/mesh-defs.h b/mesh/mesh-defs.h
index 1a199f156..4988b9e0a 100644
--- a/mesh/mesh-defs.h
+++ b/mesh/mesh-defs.h
@@ -79,6 +79,8 @@
 #define MAX_MODEL_COUNT		0xff
 #define MAX_ELE_COUNT		0xff
 
+#define MAX_MSG_LEN 380
+
 #define IS_UNASSIGNED(x)	((x) == UNASSIGNED_ADDRESS)
 #define IS_UNICAST(x)		(((x) > UNASSIGNED_ADDRESS) && \
 					((x) < VIRTUAL_ADDRESS_LOW))
diff --git a/mesh/node.c b/mesh/node.c
index e51913edf..3eb381f29 100644
--- a/mesh/node.c
+++ b/mesh/node.c
@@ -112,16 +112,16 @@ struct mesh_node {
 };
 
 struct managed_obj_request {
-	union {
-		const uint8_t *uuid;
-		struct mesh_node *node;
-	};
+	struct mesh_node *node;
 	union {
 		node_ready_func_t ready_cb;
 		node_join_ready_func_t join_ready_cb;
 	};
 	struct l_dbus_message *pending_msg;
 	enum request_type type;
+	union {
+		struct mesh_node *attach;
+	};
 };
 
 static struct l_queue *nodes;
@@ -160,14 +160,6 @@ static bool match_element_idx(const void *a, const void *b)
 	return (element->idx == index);
 }
 
-static bool match_model_id(const void *a, const void *b)
-{
-	const struct mesh_model *mod = a;
-	uint32_t mod_id = L_PTR_TO_UINT(b);
-
-	return mod_id == mesh_model_get_model_id(mod);
-}
-
 static bool match_element_path(const void *a, const void *b)
 {
 	const struct node_element *element = a;
@@ -212,11 +204,6 @@ static struct mesh_node *node_new(const uint8_t uuid[16])
 	node->net = mesh_net_new(node);
 	memcpy(node->uuid, uuid, sizeof(node->uuid));
 
-	if (!nodes)
-		nodes = l_queue_new();
-
-	l_queue_push_tail(nodes, node);
-
 	return node;
 }
 
@@ -412,6 +399,11 @@ static bool init_from_storage(struct mesh_config_node *db_node,
 
 	struct mesh_node *node = node_new(uuid);
 
+	if (!nodes)
+		nodes = l_queue_new();
+
+	l_queue_push_tail(nodes, node);
+
 	node->comp = l_new(struct node_composition, 1);
 	node->comp->cid = db_node->cid;
 	node->comp->pid = db_node->pid;
@@ -436,7 +428,7 @@ static bool init_from_storage(struct mesh_config_node *db_node,
 	memcpy(node->token, db_node->token, 8);
 
 	num_ele = l_queue_length(db_node->elements);
-	if (num_ele > 0xff)
+	if (num_ele > MAX_ELE_COUNT)
 		goto fail;
 
 	node->num_ele = num_ele;
@@ -1140,58 +1132,6 @@ static void app_disc_cb(struct l_dbus *bus, void *user_data)
 	free_node_dbus_resources(node);
 }
 
-static bool validate_model_property(struct node_element *ele,
-					struct l_dbus_message_iter *property,
-					uint8_t *num_models, bool vendor)
-{
-	struct l_dbus_message_iter ids;
-	uint16_t mod_id, vendor_id;
-	uint8_t count;
-	const char *signature = !vendor ? "aq" : "a(qq)";
-
-	if (!l_dbus_message_iter_get_variant(property, signature, &ids)) {
-		/* Allow empty elements */
-		if (l_queue_length(ele->models) == 0) {
-			*num_models = 0;
-			return true;
-		} else
-			return false;
-	}
-
-	count = 0;
-	if (!vendor) {
-		/* Bluetooth SIG defined models */
-		while (l_dbus_message_iter_next_entry(&ids, &mod_id)) {
-			struct mesh_model *mod;
-
-			/* Skip internally implemented models */
-			if ((VENDOR_ID_MASK | mod_id) == CONFIG_SRV_MODEL)
-				continue;
-
-			mod = l_queue_find(ele->models, match_model_id,
-					L_UINT_TO_PTR(VENDOR_ID_MASK | mod_id));
-			if (!mod)
-				return false;
-			count++;
-		}
-	} else {
-		/* Vendor defined models */
-		while (l_dbus_message_iter_next_entry(&ids, &vendor_id,
-								&mod_id)) {
-			struct mesh_model *mod;
-
-			mod = l_queue_find(ele->models, match_model_id,
-				L_UINT_TO_PTR((vendor_id << 16) | mod_id));
-			if (!mod)
-				return false;
-			count++;
-		}
-	}
-
-	*num_models = count;
-	return true;
-}
-
 static void get_models_from_properties(struct node_element *ele,
 					struct l_dbus_message_iter *property,
 								bool vendor)
@@ -1231,94 +1171,57 @@ static void get_models_from_properties(struct node_element *ele,
 }
 
 static bool get_element_properties(struct mesh_node *node, const char *path,
-					struct l_dbus_message_iter *properties,
-								bool is_new)
+					struct l_dbus_message_iter *properties)
 {
-	struct node_element *ele;
+	struct node_element *ele = l_new(struct node_element, 1);
 	const char *key;
 	struct l_dbus_message_iter var;
-	bool have_index = false;
-	uint8_t idx, mod_cnt, vendor_cnt;
+	bool idx = false;
+	bool loc = false;
+	bool mods = false;
+	bool vendor_mods = false;
 
 	l_debug("path %s", path);
 
 	while (l_dbus_message_iter_next_entry(properties, &key, &var)) {
-		if (!strcmp(key, "Index")) {
-			if (!l_dbus_message_iter_get_variant(&var, "y", &idx))
-				return false;
-			have_index = true;
-			break;
+		if (!idx && !strcmp(key, "Index")) {
+			if (!l_dbus_message_iter_get_variant(&var, "y",
+								&ele->idx))
+				goto fail;
+			idx = true;
+			continue;
 		}
-	}
 
-	if (!have_index) {
-		l_debug("Mandatory property \"Index\" not found");
-		return false;
-	}
-
-	if (!is_new) {
-		/* Validate composition: check the element index */
-		ele = l_queue_find(node->elements, match_element_idx,
-							L_UINT_TO_PTR(idx));
-		if (!ele) {
-			l_debug("Element with index %u not found", idx);
-			return false;
+		if (!loc && !strcmp(key, "Location")) {
+			if (!l_dbus_message_iter_get_variant(&var, "q",
+							&ele->location))
+				goto fail;
+			loc = true;
+			continue;
 		}
-	} else {
-		ele = l_new(struct node_element, 1);
-		ele->location = DEFAULT_LOCATION;
-		ele->idx = idx;
-	}
-
-	mod_cnt = 0;
-	vendor_cnt = 0;
 
-	while (l_dbus_message_iter_next_entry(properties, &key, &var)) {
-		if (!strcmp(key, "Location")) {
-			uint16_t loc;
-
-			l_dbus_message_iter_get_variant(&var, "q", &loc);
-
-			/* Validate composition: location match */
-			if (!is_new && (ele->location != loc))
-				return false;
-
-			ele->location = loc;
-
-		} else if (!strcmp(key, "Models")) {
-
-			if (is_new)
-				get_models_from_properties(ele, &var, false);
-			else if (!validate_model_property(ele, &var, &mod_cnt,
-									false))
-				return false;
-
-		} else if (!strcmp(key, "VendorModels")) {
-
-			if (is_new)
-				get_models_from_properties(ele, &var, true);
-			else if (!validate_model_property(ele, &var,
-							&vendor_cnt, true))
-				return false;
+		if (!mods && !strcmp(key, "Models")) {
+			get_models_from_properties(ele, &var, false);
+			mods = true;
+			continue;
+		}
 
+		if (!vendor_mods && !strcmp(key, "VendorModels")) {
+			get_models_from_properties(ele, &var, true);
+			vendor_mods = true;
+			continue;
 		}
 	}
 
-	if (is_new) {
-		l_queue_push_tail(node->elements, ele);
-	} else {
-		/* Account for internal Configuration Server model */
-		if (idx == 0)
-			mod_cnt += 1;
-
-		/* Validate composition: number of models must match */
-		if (l_queue_length(ele->models) != (mod_cnt + vendor_cnt))
-			return false;
-
-		ele->path = l_strdup(path);
-	}
+	if (!idx || !loc || !mods || !vendor_mods)
+		goto fail;
 
+	l_queue_push_tail(node->elements, ele);
 	return true;
+fail:
+	l_free(ele);
+
+	return false;
 }
 
 static void convert_node_to_storage(struct mesh_node *node,
@@ -1415,65 +1318,59 @@ static void set_defaults(struct mesh_node *node)
 }
 
 static bool get_app_properties(struct mesh_node *node, const char *path,
-					struct l_dbus_message_iter *properties,
-								bool is_new)
+					struct l_dbus_message_iter *properties)
 {
 	const char *key;
 	struct l_dbus_message_iter variant;
-	uint16_t value;
+	bool cid = false;
+	bool pid = false;
+	bool vid = false;
 
 	l_debug("path %s", path);
 
-	if (is_new) {
-		node->comp = l_new(struct node_composition, 1);
-		node->comp->crpl = DEFAULT_CRPL;
-	}
+	node->comp = l_new(struct node_composition, 1);
+	node->comp->crpl = DEFAULT_CRPL;
 
 	while (l_dbus_message_iter_next_entry(properties, &key, &variant)) {
-
-		if (!strcmp(key, "CompanyID")) {
+		if (!cid && !strcmp(key, "CompanyID")) {
 			if (!l_dbus_message_iter_get_variant(&variant, "q",
-									&value))
-				return false;
-
-			if (!is_new && node->comp->cid != value)
-				return false;
-
-			node->comp->cid = value;
+							&node->comp->cid))
+				goto fail;
+			cid = true;
+			continue;
+		}
 
-		} else if (!strcmp(key, "ProductID")) {
+		if (!pid && !strcmp(key, "ProductID")) {
 			if (!l_dbus_message_iter_get_variant(&variant, "q",
-									&value))
-				return false;
-
-			if (!is_new && node->comp->pid != value)
-				return false;
-
-			node->comp->pid = value;
+							&node->comp->pid))
+				goto fail;
+			pid = true;
+			continue;
+		}
 
-		} else if (!strcmp(key, "VersionID")) {
+		if (!vid && !strcmp(key, "VersionID")) {
 			if (!l_dbus_message_iter_get_variant(&variant, "q",
-									&value))
+							&node->comp->vid))
 				return false;
+			vid = true;
+			continue;
+		}
 
-			if (!is_new && node->comp->vid != value)
-				return false;
-
-			node->comp->vid = value;
-
-		} else if (!strcmp(key, "CRPL")) {
+		if (!strcmp(key, "CRPL"))
 			if (!l_dbus_message_iter_get_variant(&variant, "q",
-									&value))
-				return false;
-
-			if (!is_new && node->comp->crpl != value)
-				return false;
-
-			node->comp->crpl = value;
-		}
+							&node->comp->crpl))
+				goto fail;
 	}
 
+	if (!vid || !pid || !vid)
+		goto fail;
+
 	return true;
+fail:
+	l_free(node->comp);
+	node->comp = NULL;
+
+	return false;
 }
 
 static bool add_local_node(struct mesh_node *node, uint16_t unicast, bool kr,
@@ -1552,18 +1449,40 @@ static bool init_storage_dir(struct mesh_node *node)
 	return true;
 }
 
+static bool check_req_node(struct managed_obj_request *req)
+{
+	uint8_t node_comp[MAX_MSG_LEN - 2];
+	uint8_t attach_comp[MAX_MSG_LEN - 2];
+
+	uint16_t node_len = node_generate_comp(req->node, node_comp,
+							sizeof(node_comp));
+
+	if (!node_len)
+		return false;
+
+	if (req->type == REQUEST_TYPE_ATTACH) {
+		uint16_t attach_len = node_generate_comp(req->attach,
+					attach_comp, sizeof(attach_comp));
+
+		if (node_len != attach_len ||
+				memcmp(node_comp, attach_comp, node_len)) {
+			l_debug("Failed to verify app's composition data");
+			return false;
+		}
+	}
+
+	return true;
+}
+
 static void get_managed_objects_cb(struct l_dbus_message *msg, void *user_data)
 {
 	struct l_dbus_message_iter objects, interfaces;
 	struct managed_obj_request *req = user_data;
 	const char *path;
-	struct mesh_node *node = NULL;
+	struct mesh_node *node = req->node;
 	void *agent = NULL;
 	bool have_app = false;
-	bool is_new;
-	uint8_t num_ele;
-
-	is_new = (req->type != REQUEST_TYPE_ATTACH);
+	unsigned int num_ele;
 
 	if (l_dbus_message_is_error(msg)) {
 		l_error("Failed to get app's dbus objects");
@@ -1575,14 +1494,8 @@ static void get_managed_objects_cb(struct l_dbus_message *msg, void *user_data)
 		goto fail;
 	}
 
-	if (is_new) {
-		node = l_new(struct mesh_node, 1);
+	if (!node->elements)
 		node->elements = l_queue_new();
-	} else {
-		node = req->node;
-	}
-
-	num_ele = 0;
 
 	while (l_dbus_message_iter_next_entry(&objects, &path, &interfaces)) {
 		struct l_dbus_message_iter properties;
@@ -1593,21 +1506,14 @@ static void get_managed_objects_cb(struct l_dbus_message *msg, void *user_data)
 			bool res;
 
 			if (!strcmp(MESH_ELEMENT_INTERFACE, interface)) {
-
-				if (num_ele == MAX_ELE_COUNT)
-					goto fail;
-
 				res = get_element_properties(node, path,
-							&properties, is_new);
+								&properties);
 				if (!res)
 					goto fail;
-
-				num_ele++;
-
 			} else if (!strcmp(MESH_APPLICATION_INTERFACE,
 								interface)) {
 				res = get_app_properties(node, path,
-							&properties, is_new);
+								&properties);
 				if (!res)
 					goto fail;
 
@@ -1637,7 +1543,7 @@ static void get_managed_objects_cb(struct l_dbus_message *msg, void *user_data)
 		goto fail;
 	}
 
-	if (num_ele == 0) {
+	if (l_queue_isempty(node->elements)) {
 		l_error("Interface %s not found", MESH_ELEMENT_INTERFACE);
 		goto fail;
 	}
@@ -1649,17 +1555,24 @@ static void get_managed_objects_cb(struct l_dbus_message *msg, void *user_data)
 		goto fail;
 	}
 
+	set_defaults(node);
+	num_ele = l_queue_length(node->elements);
+
+	if (num_ele > MAX_ELE_COUNT)
+		goto fail;
+
+	node->num_ele = num_ele;
+
+	if (!check_req_node(req))
+		goto fail;
+
 	if (req->type == REQUEST_TYPE_ATTACH) {
-		if (num_ele != node->num_ele)
-			goto fail;
+		struct l_dbus *bus = dbus_get_bus();
 
-		if (register_node_object(node)) {
-			struct l_dbus *bus = dbus_get_bus();
+		node_remove(node);
+		node = req->attach;
 
-			node->disc_watch = l_dbus_add_disconnect_watch(bus,
-					node->owner, app_disc_cb, node, NULL);
-			req->ready_cb(req->pending_msg, MESH_ERROR_NONE, node);
-		} else
+		if (!register_node_object(node))
 			goto fail;
 
 		/*
@@ -1670,30 +1583,24 @@ static void get_managed_objects_cb(struct l_dbus_message *msg, void *user_data)
 		 */
 		init_storage_dir(node);
 
+		node->disc_watch = l_dbus_add_disconnect_watch(bus,
+				node->owner, app_disc_cb, node, NULL);
+
 	} else if (req->type == REQUEST_TYPE_JOIN) {
-		if (!agent) {
+		if (!node->agent) {
 			l_error("Interface %s not found",
 						MESH_PROVISION_AGENT_INTERFACE);
 			goto fail;
 		}
 
-		node->num_ele = num_ele;
-		set_defaults(node);
-		memcpy(node->uuid, req->uuid, 16);
-
 		if (!create_node_config(node, node->uuid))
 			goto fail;
 
-		req->join_ready_cb(node, agent);
 	} else {
 		/* Callback for create node request */
 		struct keyring_net_key net_key;
 		uint8_t dev_key[16];
 
-		node->num_ele = num_ele;
-		set_defaults(node);
-		memcpy(node->uuid, req->uuid, 16);
-
 		if (!create_node_config(node, node->uuid))
 			goto fail;
 
@@ -1713,35 +1620,32 @@ static void get_managed_objects_cb(struct l_dbus_message *msg, void *user_data)
 		init_storage_dir(node);
 
 		if (!keyring_put_remote_dev_key(node, DEFAULT_NEW_UNICAST,
-							num_ele, dev_key))
+						node->num_ele, dev_key))
 			goto fail;
 
 		if (!keyring_put_net_key(node, PRIMARY_NET_IDX, &net_key))
 			goto fail;
 
-		req->ready_cb(req->pending_msg, MESH_ERROR_NONE, node);
 	}
 
+	if (req->type == REQUEST_TYPE_JOIN)
+		req->join_ready_cb(node, node->agent);
+	else
+		req->ready_cb(req->pending_msg, MESH_ERROR_NONE, node);
+
 	return;
 fail:
 	if (agent)
 		mesh_agent_remove(agent);
 
-	if (!is_new) {
-		free_node_dbus_resources(node);
+	/* Handle failed requests */
+	if (node)
+		node_remove(node);
 
-		req->ready_cb(req->pending_msg, MESH_ERROR_FAILED, node);
-	} else {
-		/* Handle failed Join and Create requests */
-		if (node)
-			node_remove(node);
-
-		if (req->type == REQUEST_TYPE_JOIN)
-			req->join_ready_cb(NULL, NULL);
-		else
-			req->ready_cb(req->pending_msg, MESH_ERROR_FAILED,
-									NULL);
-	}
+	if (req->type == REQUEST_TYPE_JOIN)
+		req->join_ready_cb(NULL, NULL);
+	else
+		req->ready_cb(req->pending_msg, MESH_ERROR_FAILED, NULL);
 }
 
 /* Establish relationship between application and mesh node */
@@ -1765,9 +1669,10 @@ int node_attach(const char *app_path, const char *sender, uint64_t token,
 	node->owner = l_strdup(sender);
 
 	req = l_new(struct managed_obj_request, 1);
-	req->node = node;
+	req->node = node_new(node->uuid);
 	req->ready_cb = cb;
 	req->pending_msg = user_data;
+	req->attach = node;
 	req->type = REQUEST_TYPE_ATTACH;
 
 	l_dbus_method_call(dbus_get_bus(), sender, app_path,
@@ -1789,7 +1694,7 @@ void node_join(const char *app_path, const char *sender, const uint8_t *uuid,
 	l_debug("");
 
 	req = l_new(struct managed_obj_request, 1);
-	req->uuid = uuid;
+	req->node = node_new(uuid);
 	req->join_ready_cb = cb;
 	req->type = REQUEST_TYPE_JOIN;
 
@@ -1808,7 +1713,7 @@ void node_create(const char *app_path, const char *sender, const uint8_t *uuid,
 	l_debug("");
 
 	req = l_new(struct managed_obj_request, 1);
-	req->uuid = uuid;
+	req->node = node_new(uuid);
 	req->ready_cb = cb;
 	req->pending_msg = user_data;
 	req->type = REQUEST_TYPE_CREATE;
-- 
2.19.1

Re: [PATCH BlueZ v2 2/5] mesh: Validate application by comparing composition data

[Stotland, Inga]

[-- Attachment #1: Type: text/plain, Size: 19315 bytes --]

Hi Michal,

On Tue, 2019-07-23 at 12:06 +0200, Michał Lowas-Rzechonek wrote:
> Instead of validating application by enumerating D-Bus objects,
> create a
> temporary node instance and check if composition data generated for
> the
> temporary matches the node loaded from storage.
> 
> This allows node validation logic (primary element, mandatory models
> etc)
> to be confined in node_generate_comp() function.
> 
> This also streamlines code implementing Attach(), Join(),
> CreateNetwork() and ImportLocalNode() calls.
> ---
>  mesh/mesh-defs.h |   2 +
>  mesh/node.c      | 385 ++++++++++++++++++---------------------------
> --
>  2 files changed, 147 insertions(+), 240 deletions(-)
> 
> diff --git a/mesh/mesh-defs.h b/mesh/mesh-defs.h
> index 1a199f156..4988b9e0a 100644
> --- a/mesh/mesh-defs.h
> +++ b/mesh/mesh-defs.h
> @@ -79,6 +79,8 @@
>  #define MAX_MODEL_COUNT		0xff
>  #define MAX_ELE_COUNT		0xff
>  
> +#define MAX_MSG_LEN 380
> +
>  #define IS_UNASSIGNED(x)	((x) == UNASSIGNED_ADDRESS)
>  #define IS_UNICAST(x)		(((x) > UNASSIGNED_ADDRESS) &&
> \
>  					((x) < VIRTUAL_ADDRESS_LOW))
> diff --git a/mesh/node.c b/mesh/node.c
> index e51913edf..3eb381f29 100644
> --- a/mesh/node.c
> +++ b/mesh/node.c
> @@ -112,16 +112,16 @@ struct mesh_node {
>  };
>  
>  struct managed_obj_request {
> -	union {
> -		const uint8_t *uuid;
> -		struct mesh_node *node;
> -	};
> +	struct mesh_node *node;
>  	union {
>  		node_ready_func_t ready_cb;
>  		node_join_ready_func_t join_ready_cb;
>  	};
>  	struct l_dbus_message *pending_msg;
>  	enum request_type type;
> +	union {
> +		struct mesh_node *attach;
> +	};
>  };
>  
>  static struct l_queue *nodes;
> @@ -160,14 +160,6 @@ static bool match_element_idx(const void *a,
> const void *b)
>  	return (element->idx == index);
>  }
>  
> -static bool match_model_id(const void *a, const void *b)
> -{
> -	const struct mesh_model *mod = a;
> -	uint32_t mod_id = L_PTR_TO_UINT(b);
> -
> -	return mod_id == mesh_model_get_model_id(mod);
> -}
> -
>  static bool match_element_path(const void *a, const void *b)
>  {
>  	const struct node_element *element = a;
> @@ -212,11 +204,6 @@ static struct mesh_node *node_new(const uint8_t
> uuid[16])
>  	node->net = mesh_net_new(node);
>  	memcpy(node->uuid, uuid, sizeof(node->uuid));
>  
> -	if (!nodes)
> -		nodes = l_queue_new();
> -
> -	l_queue_push_tail(nodes, node);
> -
>  	return node;
>  }
>  
> @@ -412,6 +399,11 @@ static bool init_from_storage(struct
> mesh_config_node *db_node,
>  
>  	struct mesh_node *node = node_new(uuid);
>  
> +	if (!nodes)
> +		nodes = l_queue_new();
> +
> +	l_queue_push_tail(nodes, node);
> +
>  	node->comp = l_new(struct node_composition, 1);
>  	node->comp->cid = db_node->cid;
>  	node->comp->pid = db_node->pid;
> @@ -436,7 +428,7 @@ static bool init_from_storage(struct
> mesh_config_node *db_node,
>  	memcpy(node->token, db_node->token, 8);
>  
>  	num_ele = l_queue_length(db_node->elements);
> -	if (num_ele > 0xff)
> +	if (num_ele > MAX_ELE_COUNT)
>  		goto fail;
>  
>  	node->num_ele = num_ele;
> @@ -1140,58 +1132,6 @@ static void app_disc_cb(struct l_dbus *bus,
> void *user_data)
>  	free_node_dbus_resources(node);
>  }
>  
> -static bool validate_model_property(struct node_element *ele,
> -					struct l_dbus_message_iter
> *property,
> -					uint8_t *num_models, bool
> vendor)
> -{
> -	struct l_dbus_message_iter ids;
> -	uint16_t mod_id, vendor_id;
> -	uint8_t count;
> -	const char *signature = !vendor ? "aq" : "a(qq)";
> -
> -	if (!l_dbus_message_iter_get_variant(property, signature,
> &ids)) {
> -		/* Allow empty elements */
> -		if (l_queue_length(ele->models) == 0) {
> -			*num_models = 0;
> -			return true;
> -		} else
> -			return false;
> -	}
> -
> -	count = 0;
> -	if (!vendor) {
> -		/* Bluetooth SIG defined models */
> -		while (l_dbus_message_iter_next_entry(&ids, &mod_id)) {
> -			struct mesh_model *mod;
> -
> -			/* Skip internally implemented models */
> -			if ((VENDOR_ID_MASK | mod_id) ==
> CONFIG_SRV_MODEL)
> -				continue;
> -
> -			mod = l_queue_find(ele->models, match_model_id,
> -					L_UINT_TO_PTR(VENDOR_ID_MASK |
> mod_id));
> -			if (!mod)
> -				return false;
> -			count++;
> -		}
> -	} else {
> -		/* Vendor defined models */
> -		while (l_dbus_message_iter_next_entry(&ids, &vendor_id,
> -								&mod_id
> )) {
> -			struct mesh_model *mod;
> -
> -			mod = l_queue_find(ele->models, match_model_id,
> -				L_UINT_TO_PTR((vendor_id << 16) |
> mod_id));
> -			if (!mod)
> -				return false;
> -			count++;
> -		}
> -	}
> -
> -	*num_models = count;
> -	return true;
> -}
> -
>  static void get_models_from_properties(struct node_element *ele,
>  					struct l_dbus_message_iter
> *property,
>  								bool
> vendor)
> @@ -1231,94 +1171,57 @@ static void get_models_from_properties(struct
> node_element *ele,
>  }
>  
>  static bool get_element_properties(struct mesh_node *node, const
> char *path,
> -					struct l_dbus_message_iter
> *properties,
> -								bool
> is_new)
> +					struct l_dbus_message_iter
> *properties)
>  {
> -	struct node_element *ele;
> +	struct node_element *ele = l_new(struct node_element, 1);
>  	const char *key;
>  	struct l_dbus_message_iter var;
> -	bool have_index = false;
> -	uint8_t idx, mod_cnt, vendor_cnt;
> +	bool idx = false;
> +	bool loc = false;
> +	bool mods = false;
> +	bool vendor_mods = false;
>  
>  	l_debug("path %s", path);
>  
>  	while (l_dbus_message_iter_next_entry(properties, &key, &var))
> {
> -		if (!strcmp(key, "Index")) {
> -			if (!l_dbus_message_iter_get_variant(&var, "y",
> &idx))
> -				return false;
> -			have_index = true;
> -			break;
> +		if (!idx && !strcmp(key, "Index")) {
> +			if (!l_dbus_message_iter_get_variant(&var, "y",
> +								&ele-
> >idx))
> +				goto fail;
> +			idx = true;
> +			continue;
>  		}
> -	}
>  
> -	if (!have_index) {
> -		l_debug("Mandatory property \"Index\" not found");
> -		return false;
> -	}
> -
> -	if (!is_new) {
> -		/* Validate composition: check the element index */
> -		ele = l_queue_find(node->elements, match_element_idx,
> -							L_UINT_TO_PTR(i
> dx));
> -		if (!ele) {
> -			l_debug("Element with index %u not found",
> idx);
> -			return false;
> +		if (!loc && !strcmp(key, "Location")) {
> +			if (!l_dbus_message_iter_get_variant(&var, "q",
> +							&ele-
> >location))
> +				goto fail;
> +			loc = true;
> +			continue;
>  		}
> -	} else {
> -		ele = l_new(struct node_element, 1);
> -		ele->location = DEFAULT_LOCATION;
> -		ele->idx = idx;
> -	}
> -
> -	mod_cnt = 0;
> -	vendor_cnt = 0;
>  
> -	while (l_dbus_message_iter_next_entry(properties, &key, &var))
> {
> -		if (!strcmp(key, "Location")) {
> -			uint16_t loc;
> -
> -			l_dbus_message_iter_get_variant(&var, "q",
> &loc);
> -
> -			/* Validate composition: location match */
> -			if (!is_new && (ele->location != loc))
> -				return false;
> -
> -			ele->location = loc;
> -
> -		} else if (!strcmp(key, "Models")) {
> -
> -			if (is_new)
> -				get_models_from_properties(ele, &var,
> false);
> -			else if (!validate_model_property(ele, &var,
> &mod_cnt,
> -									
> false))
> -				return false;
> -
> -		} else if (!strcmp(key, "VendorModels")) {
> -
> -			if (is_new)
> -				get_models_from_properties(ele, &var,
> true);
> -			else if (!validate_model_property(ele, &var,
> -							&vendor_cnt,
> true))
> -				return false;
> +		if (!mods && !strcmp(key, "Models")) {
> +			get_models_from_properties(ele, &var, false);
> +			mods = true;
> +			continue;
> +		}
>  
> +		if (!vendor_mods && !strcmp(key, "VendorModels")) {
> +			get_models_from_properties(ele, &var, true);
> +			vendor_mods = true;
> +			continue;
>  		}
>  	}
>  
> -	if (is_new) {
> -		l_queue_push_tail(node->elements, ele);
> -	} else {
> -		/* Account for internal Configuration Server model */
> -		if (idx == 0)
> -			mod_cnt += 1;
> -
> -		/* Validate composition: number of models must match */
> -		if (l_queue_length(ele->models) != (mod_cnt +
> vendor_cnt))
> -			return false;
> -
> -		ele->path = l_strdup(path);

The path needs to be preserved if this is the Attach() request.
One way to do this would be to save it here, in the temporary node and
then after all the checks are done, move it to the node that is "owned"
by the daemon prior to deleting the temporary node in
get_managed_objects_cb()

> -	}
> +	if (!idx || !loc || !mods || !vendor_mods)
> +		goto fail;
> 

"Location" property is described as optional in mesh-api.txt. It's
populated with "Default Location", if the property is not present.
I believe, it was more out of the convenience for the app develpers,
but maybe we should change the property description as mandatory, i.e.,
remove "optional" from the property descrition in mesh-api.txt

>  
> +	l_queue_push_tail(node->elements, ele);
>  	return true;
> +fail:
> +	l_free(ele);
> +
> +	return false;
>  }
>  
>  static void convert_node_to_storage(struct mesh_node *node,
> @@ -1415,65 +1318,59 @@ static void set_defaults(struct mesh_node
> *node)
>  }
>  
>  static bool get_app_properties(struct mesh_node *node, const char
> *path,
> -					struct l_dbus_message_iter
> *properties,
> -								bool
> is_new)
> +					struct l_dbus_message_iter
> *properties)
>  {
>  	const char *key;
>  	struct l_dbus_message_iter variant;
> -	uint16_t value;
> +	bool cid = false;
> +	bool pid = false;
> +	bool vid = false;
>  
>  	l_debug("path %s", path);
>  
> -	if (is_new) {
> -		node->comp = l_new(struct node_composition, 1);
> -		node->comp->crpl = DEFAULT_CRPL;
> -	}
> +	node->comp = l_new(struct node_composition, 1);
> +	node->comp->crpl = DEFAULT_CRPL;
>  
>  	while (l_dbus_message_iter_next_entry(properties, &key,
> &variant)) {
> -
> -		if (!strcmp(key, "CompanyID")) {
> +		if (!cid && !strcmp(key, "CompanyID")) {
>  			if (!l_dbus_message_iter_get_variant(&variant,
> "q",
> -									
> &value))
> -				return false;
> -
> -			if (!is_new && node->comp->cid != value)
> -				return false;
> -
> -			node->comp->cid = value;
> +							&node->comp-
> >cid))
> +				goto fail;
> +			cid = true;
> +			continue;
> +		}
>  
> -		} else if (!strcmp(key, "ProductID")) {
> +		if (!pid && !strcmp(key, "ProductID")) {
>  			if (!l_dbus_message_iter_get_variant(&variant,
> "q",
> -									
> &value))
> -				return false;
> -
> -			if (!is_new && node->comp->pid != value)
> -				return false;
> -
> -			node->comp->pid = value;
> +							&node->comp-
> >pid))
> +				goto fail;
> +			pid = true;
> +			continue;
> +		}
>  
> -		} else if (!strcmp(key, "VersionID")) {
> +		if (!vid && !strcmp(key, "VersionID")) {
>  			if (!l_dbus_message_iter_get_variant(&variant,
> "q",
> -									
> &value))
> +							&node->comp-
> >vid))
>  				return false;
> +			vid = true;
> +			continue;
> +		}
>  
> -			if (!is_new && node->comp->vid != value)
> -				return false;
> -
> -			node->comp->vid = value;
> -
> -		} else if (!strcmp(key, "CRPL")) {
> +		if (!strcmp(key, "CRPL"))
>  			if (!l_dbus_message_iter_get_variant(&variant,
> "q",
> -									
> &value))
> -				return false;
> -
> -			if (!is_new && node->comp->crpl != value)
> -				return false;
> -
> -			node->comp->crpl = value;
> -		}
> +							&node->comp-
> >crpl))
> +				goto fail;
>  	}
>  
> +	if (!vid || !pid || !vid)
> +		goto fail;
> +
>  	return true;
> +fail:
> +	l_free(node->comp);
> +	node->comp = NULL;
> +
> +	return false;
>  }
>  
>  static bool add_local_node(struct mesh_node *node, uint16_t unicast,
> bool kr,
> @@ -1552,18 +1449,40 @@ static bool init_storage_dir(struct mesh_node
> *node)
>  	return true;
>  }
>  
> +static bool check_req_node(struct managed_obj_request *req)
> +{
> +	uint8_t node_comp[MAX_MSG_LEN - 2];
> +	uint8_t attach_comp[MAX_MSG_LEN - 2];
> +
> +	uint16_t node_len = node_generate_comp(req->node, node_comp,
> +							sizeof(node_com
> p));
> +
> +	if (!node_len)
> +		return false;
> +
> +	if (req->type == REQUEST_TYPE_ATTACH) {
> +		uint16_t attach_len = node_generate_comp(req->attach,
> +					attach_comp,
> sizeof(attach_comp));
> +
> +		if (node_len != attach_len ||
> +				memcmp(node_comp, attach_comp,
> node_len)) {
> +			l_debug("Failed to verify app's composition
> data");
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
>  static void get_managed_objects_cb(struct l_dbus_message *msg, void
> *user_data)
>  {
>  	struct l_dbus_message_iter objects, interfaces;
>  	struct managed_obj_request *req = user_data;
>  	const char *path;
> -	struct mesh_node *node = NULL;
> +	struct mesh_node *node = req->node;
>  	void *agent = NULL;
>  	bool have_app = false;
> -	bool is_new;
> -	uint8_t num_ele;
> -
> -	is_new = (req->type != REQUEST_TYPE_ATTACH);
> +	unsigned int num_ele;
>  
>  	if (l_dbus_message_is_error(msg)) {
>  		l_error("Failed to get app's dbus objects");
> @@ -1575,14 +1494,8 @@ static void get_managed_objects_cb(struct
> l_dbus_message *msg, void *user_data)
>  		goto fail;
>  	}
>  
> -	if (is_new) {
> -		node = l_new(struct mesh_node, 1);
> +	if (!node->elements)
>  		node->elements = l_queue_new();
> -	} else {
> -		node = req->node;
> -	}
> -
> -	num_ele = 0;
>  
>  	while (l_dbus_message_iter_next_entry(&objects, &path,
> &interfaces)) {
>  		struct l_dbus_message_iter properties;
> @@ -1593,21 +1506,14 @@ static void get_managed_objects_cb(struct
> l_dbus_message *msg, void *user_data)
>  			bool res;
>  
>  			if (!strcmp(MESH_ELEMENT_INTERFACE, interface))
> {
> -
> -				if (num_ele == MAX_ELE_COUNT)
> -					goto fail;
> -
>  				res = get_element_properties(node,
> path,
> -							&properties,
> is_new);
> +								&proper
> ties);
>  				if (!res)
>  					goto fail;
> -
> -				num_ele++;
> -
>  			} else if (!strcmp(MESH_APPLICATION_INTERFACE,
>  								interfa
> ce)) {
>  				res = get_app_properties(node, path,
> -							&properties,
> is_new);
> +								&proper
> ties);
>  				if (!res)
>  					goto fail;
>  
> @@ -1637,7 +1543,7 @@ static void get_managed_objects_cb(struct
> l_dbus_message *msg, void *user_data)
>  		goto fail;
>  	}
>  
> -	if (num_ele == 0) {
> +	if (l_queue_isempty(node->elements)) {
>  		l_error("Interface %s not found",
> MESH_ELEMENT_INTERFACE);
>  		goto fail;
>  	}
> @@ -1649,17 +1555,24 @@ static void get_managed_objects_cb(struct
> l_dbus_message *msg, void *user_data)
>  		goto fail;
>  	}
>  
> +	set_defaults(node);
> +	num_ele = l_queue_length(node->elements);
> +
> +	if (num_ele > MAX_ELE_COUNT)
> +		goto fail;
> +
> +	node->num_ele = num_ele;
> +
> +	if (!check_req_node(req))
> +		goto fail;
> +
>  	if (req->type == REQUEST_TYPE_ATTACH) {
> -		if (num_ele != node->num_ele)
> -			goto fail;
> +		struct l_dbus *bus = dbus_get_bus();
>  
> -		if (register_node_object(node)) {
> -			struct l_dbus *bus = dbus_get_bus();

So here, prior to removing the temorary node, the element paths need to
be copied into the "req->attach" version of the node.
Same goes for node->agent and node->provisioner.	

> +		node_remove(node);
> +		node = req->attach;
>  
> -			node->disc_watch =
> l_dbus_add_disconnect_watch(bus,
> -					node->owner, app_disc_cb, node,
> NULL);
> -			req->ready_cb(req->pending_msg,
> MESH_ERROR_NONE, node);
> -		} else
> +		if (!register_node_object(node))
>  			goto fail;
>  
>  		/*
> @@ -1670,30 +1583,24 @@ static void get_managed_objects_cb(struct
> l_dbus_message *msg, void *user_data)
>  		 */
>  		init_storage_dir(node);
>  
> +		node->disc_watch = l_dbus_add_disconnect_watch(bus,
> +				node->owner, app_disc_cb, node, NULL);
> +
>  	} else if (req->type == REQUEST_TYPE_JOIN) {
> -		if (!agent) {
> +		if (!node->agent) {
>  			l_error("Interface %s not found",
>  						MESH_PROVISION_AGENT_IN
> TERFACE);
>  			goto fail;
>  		}
>  
> -		node->num_ele = num_ele;
> -		set_defaults(node);
> -		memcpy(node->uuid, req->uuid, 16);
> -
>  		if (!create_node_config(node, node->uuid))
>  			goto fail;
>  
> -		req->join_ready_cb(node, agent);
>  	} else {
>  		/* Callback for create node request */
>  		struct keyring_net_key net_key;
>  		uint8_t dev_key[16];
>  
> -		node->num_ele = num_ele;
> -		set_defaults(node);
> -		memcpy(node->uuid, req->uuid, 16);
> -
>  		if (!create_node_config(node, node->uuid))
>  			goto fail;
>  
> @@ -1713,35 +1620,32 @@ static void get_managed_objects_cb(struct
> l_dbus_message *msg, void *user_data)
>  		init_storage_dir(node);
>  
>  		if (!keyring_put_remote_dev_key(node,
> DEFAULT_NEW_UNICAST,
> -							num_ele,
> dev_key))
> +						node->num_ele,
> dev_key))
>  			goto fail;
>  
>  		if (!keyring_put_net_key(node, PRIMARY_NET_IDX,
> &net_key))
>  			goto fail;
>  
> -		req->ready_cb(req->pending_msg, MESH_ERROR_NONE, node);
>  	}
>  
> +	if (req->type == REQUEST_TYPE_JOIN)
> +		req->join_ready_cb(node, node->agent);
> +	else
> +		req->ready_cb(req->pending_msg, MESH_ERROR_NONE, node);
> +
>  	return;
>  fail:
>  	if (agent)
>  		mesh_agent_remove(agent);
>  
> -	if (!is_new) {
> -		free_node_dbus_resources(node);
> +	/* Handle failed requests */
> +	if (node)
> +		node_remove(node);
>  
> -		req->ready_cb(req->pending_msg, MESH_ERROR_FAILED,
> node);
> -	} else {
> -		/* Handle failed Join and Create requests */
> -		if (node)
> -			node_remove(node);
> -
> -		if (req->type == REQUEST_TYPE_JOIN)
> -			req->join_ready_cb(NULL, NULL);
> -		else
> -			req->ready_cb(req->pending_msg,
> MESH_ERROR_FAILED,
> -									
> NULL);
> -	}
> +	if (req->type == REQUEST_TYPE_JOIN)
> +		req->join_ready_cb(NULL, NULL);
> +	else
> +		req->ready_cb(req->pending_msg, MESH_ERROR_FAILED,
> NULL);
>  }
>  
>  /* Establish relationship between application and mesh node */
> @@ -1765,9 +1669,10 @@ int node_attach(const char *app_path, const
> char *sender, uint64_t token,
>  	node->owner = l_strdup(sender);
>  
>  	req = l_new(struct managed_obj_request, 1);
> -	req->node = node;
> +	req->node = node_new(node->uuid);
>  	req->ready_cb = cb;
>  	req->pending_msg = user_data;
> +	req->attach = node;
>  	req->type = REQUEST_TYPE_ATTACH;
>  
>  	l_dbus_method_call(dbus_get_bus(), sender, app_path,
> @@ -1789,7 +1694,7 @@ void node_join(const char *app_path, const char
> *sender, const uint8_t *uuid,
>  	l_debug("");
>  
>  	req = l_new(struct managed_obj_request, 1);
> -	req->uuid = uuid;
> +	req->node = node_new(uuid);
>  	req->join_ready_cb = cb;
>  	req->type = REQUEST_TYPE_JOIN;
>  
> @@ -1808,7 +1713,7 @@ void node_create(const char *app_path, const
> char *sender, const uint8_t *uuid,
>  	l_debug("");
>  
>  	req = l_new(struct managed_obj_request, 1);
> -	req->uuid = uuid;
> +	req->node = node_new(uuid);
>  	req->ready_cb = cb;
>  	req->pending_msg = user_data;
>  	req->type = REQUEST_TYPE_CREATE;

Thanks,
Inga

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3265 bytes --]

Re: [PATCH BlueZ v2 2/5] mesh: Validate application by comparing composition data

[michal.lowas-rzechonek]

Inga,

On 07/24, Stotland, Inga wrote:
> > -		ele->path = l_strdup(path);
> 
> The path needs to be preserved if this is the Attach() request.
> One way to do this would be to save it here, in the temporary node and
> then after all the checks are done, move it to the node that is "owned"
> by the daemon prior to deleting the temporary node in
> get_managed_objects_cb()
(...)
> 
> So here, prior to removing the temorary node, the element paths need to
> be copied into the "req->attach" version of the node.
> Same goes for node->agent and node->provisioner.	

True, thanks! I'll fix this in v3.

> "Location" property is described as optional in mesh-api.txt. It's
> populated with "Default Location", if the property is not present.
> I believe, it was more out of the convenience for the app develpers,
> but maybe we should change the property description as mandatory, i.e.,
> remove "optional" from the property descrition in mesh-api.txt

I'm inclined to make this property mandatory, but I don't have a strong
opinion about it. So just tell me how should it be and I'll adjust v3
accordingly.

-- 
Michał Lowas-Rzechonek <michal.lowas-rzechonek@silvair.com>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND

Re: [PATCH BlueZ v2 2/5] mesh: Validate application by comparing composition data

[Stotland, Inga]

[-- Attachment #1: Type: text/plain, Size: 1942 bytes --]

Hi Michal,

On Wed, 2019-07-24 at 09:55 +0200, michal.lowas-rzechonek@silvair.com
wrote:
> Inga,
> 
> On 07/24, Stotland, Inga wrote:
> > > -		ele->path = l_strdup(path);
> > 
> > The path needs to be preserved if this is the Attach() request.
> > One way to do this would be to save it here, in the temporary node
> > and
> > then after all the checks are done, move it to the node that is
> > "owned"
> > by the daemon prior to deleting the temporary node in
> > get_managed_objects_cb()
> (...)
> > So here, prior to removing the temorary node, the element paths
> > need to
> > be copied into the "req->attach" version of the node.
> > Same goes for node->agent and node->provisioner.	
> 
> True, thanks! I'll fix this in v3.
> 

Also, could you please add some comment there, like "Deleting the
temporary node" just to make the point that tit was temporary? I am
afraid that for an outside person this will not be clear why the node
is being removed.

> > "Location" property is described as optional in mesh-api.txt. It's
> > populated with "Default Location", if the property is not present.
> > I believe, it was more out of the convenience for the app
> > develpers,
> > but maybe we should change the property description as mandatory,
> > i.e.,
> > remove "optional" from the property descrition in mesh-api.txt
> 
> I'm inclined to make this property mandatory, but I don't have a
> strong
> opinion about it. So just tell me how should it be and I'll adjust v3
> accordingly.
> 

Let's try to keep it as optional then.

This will require some *fuzzy* matching of the composition data,
something like:
add a boolean flag to generate_node_composition() that indicates
whether the composition is generated for the verification or as a
result of an external request, i.e. for the config server model. Based
on the flag, either include location field, or not.

Regards,
Inga



[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3265 bytes --]

Re: [PATCH BlueZ v2 2/5] mesh: Validate application by comparing composition data

[michal.lowas-rzechonek]

Hi Inga,

On 07/24, Stotland, Inga wrote:
> > > So here, prior to removing the temorary node, the element paths
> > > need to be copied into the "req->attach" version of the node.
> > > Same goes for node->agent and node->provisioner.	
> > 
> > True, thanks! I'll fix this in v3.
> 
> Also, could you please add some comment there, like "Deleting the
> temporary node" just to make the point that tit was temporary? I am
> afraid that for an outside person this will not be clear why the node
> is being removed.

Sure thing.

> > > "Location" property is described as optional in mesh-api.txt. It's
> > > populated with "Default Location", if the property is not present.
(...)
> Let's try to keep it as optional then.

Will do.

> This will require some *fuzzy* matching of the composition data,
> something like:
> add a boolean flag to generate_node_composition() that indicates
> whether the composition is generated for the verification or as a
> result of an external request, i.e. for the config server model. Based
> on the flag, either include location field, or not.

I don't think I follow. If the application doesn't provide the
"Location" property, everything behaves as if it provided it with value
"0", no?

Location descriptor is *not* optional in Composition Data (Table 4.4 in
section 4.2.1.1).


-- 
Michał Lowas-Rzechonek <michal.lowas-rzechonek@silvair.com>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND

Re: [PATCH BlueZ v2 2/5] mesh: Validate application by comparing composition data

[Stotland, Inga]

[-- Attachment #1: Type: text/plain, Size: 1824 bytes --]

Hi Michal,

On Wed, 2019-07-24 at 21:12 +0200, michal.lowas-rzechonek@silvair.com
wrote:
> Hi Inga,
> 
> On 07/24, Stotland, Inga wrote:
> > > > So here, prior to removing the temorary node, the element paths
> > > > need to be copied into the "req->attach" version of the node.
> > > > Same goes for node->agent and node->provisioner.	
> > > 
> > > True, thanks! I'll fix this in v3.
> > 
> > Also, could you please add some comment there, like "Deleting the
> > temporary node" just to make the point that tit was temporary? I am
> > afraid that for an outside person this will not be clear why the
> > node
> > is being removed.
> 
> Sure thing.
> 
> > > > "Location" property is described as optional in mesh-api.txt.
> > > > It's
> > > > populated with "Default Location", if the property is not
> > > > present.
> (...)
> > Let's try to keep it as optional then.
> 
> Will do.
> 
> > This will require some *fuzzy* matching of the composition data,
> > something like:
> > add a boolean flag to generate_node_composition() that indicates
> > whether the composition is generated for the verification or as a
> > result of an external request, i.e. for the config server model.
> > Based
> > on the flag, either include location field, or not.
> 
> I don't think I follow. If the application doesn't provide the
> "Location" property, everything behaves as if it provided it with
> value
> "0", no?
> 
> Location descriptor is *not* optional in Composition Data (Table 4.4
> in
> section 4.2.1.1).
> 
> 


Never mind, I was overthinking this. Even if the property is
*optional*, the value (or the fact of its presense) is not supposed to
change.
Let's keep it optional, and, just as it is currently, and it be
populated with "Default Location" if it's not found. 

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3265 bytes --]

[PATCH BlueZ v2 3/5] mesh: Keep element and model lists sorted and unique

[Michał Lowas-Rzechonek]

This keeps composition data unchanged even if elements or models are
registered in a different order.
---
 mesh/node.c | 80 ++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 67 insertions(+), 13 deletions(-)

diff --git a/mesh/node.c b/mesh/node.c
index 3eb381f29..bfb0dc5b4 100644
--- a/mesh/node.c
+++ b/mesh/node.c
@@ -160,6 +160,20 @@ static bool match_element_idx(const void *a, const void *b)
 	return (element->idx == index);
 }
 
+static int compare_element_idx(const void *a, const void *b, void *user_data)
+{
+	uint32_t a_idx = ((const struct node_element *)a)->idx;
+	uint32_t b_idx = ((const struct node_element *)b)->idx;
+
+	if (a_idx < b_idx)
+		return -1;
+
+	if (a_idx > b_idx)
+		return 1;
+
+	return 0;
+}
+
 static bool match_element_path(const void *a, const void *b)
 {
 	const struct node_element *element = a;
@@ -171,6 +185,29 @@ static bool match_element_path(const void *a, const void *b)
 	return (!strcmp(element->path, path));
 }
 
+static bool match_model_id(const void *a, const void *b)
+{
+	const struct mesh_model *mod = a;
+	uint32_t mod_id = L_PTR_TO_UINT(b);
+
+	return mesh_model_get_model_id(mod) == mod_id;
+}
+
+static int compare_model_id(const void *a, const void *b, void *user_data)
+{
+	uint32_t a_id = mesh_model_get_model_id(a);
+	uint32_t b_id = mesh_model_get_model_id(b);
+
+	if (a_id < b_id)
+		return -1;
+
+	if (a_id > b_id)
+		return 1;
+
+	return 0;
+}
+
+
 struct mesh_node *node_find_by_addr(uint16_t addr)
 {
 	if (!IS_UNICAST(addr))
@@ -287,6 +324,17 @@ void node_remove(struct mesh_node *node)
 	free_node_resources(node);
 }
 
+static bool element_add_model(struct node_element *ele, struct mesh_model *mod)
+{
+	uint32_t mod_id = mesh_model_get_model_id(mod);
+
+	if (l_queue_find(ele->models, match_model_id, L_UINT_TO_PTR(mod_id)))
+		return false;
+
+	l_queue_insert(ele->models, mod, compare_model_id, NULL);
+	return true;
+}
+
 static bool add_models(struct mesh_node *node, struct node_element *ele,
 					struct mesh_config_element *db_ele)
 {
@@ -305,7 +353,10 @@ static bool add_models(struct mesh_node *node, struct node_element *ele,
 		if (!mod)
 			return false;
 
-		l_queue_push_tail(ele->models, mod);
+		if (!element_add_model(ele, mod)) {
+			mesh_model_free(mod);
+			return false;
+		}
 	}
 
 	return true;
@@ -334,7 +385,8 @@ static void add_internal_model(struct mesh_node *node, uint32_t mod_id,
 	if (!ele->models)
 		ele->models = l_queue_new();
 
-	l_queue_push_tail(ele->models, mod);
+	if (!element_add_model(ele, mod))
+		mesh_model_free(mod);
 }
 
 static bool add_element(struct mesh_node *node,
@@ -1026,12 +1078,12 @@ bool node_parse_composition(struct mesh_node *node, uint8_t *data,
 		while (len >= 2 && m--) {
 			mod_id = l_get_le16(data);
 			mod = mesh_model_new(ele->idx, mod_id);
-			if (!mod) {
+			if (!mod || !element_add_model(ele, mod)) {
+				mesh_model_free(mod);
 				element_free(ele);
 				goto fail;
 			}
 
-			l_queue_push_tail(ele->models, mod);
 			data += 2;
 			len -= 2;
 		}
@@ -1048,12 +1100,12 @@ bool node_parse_composition(struct mesh_node *node, uint8_t *data,
 			mod_id |= (vendor_id << 16);
 			mod = mesh_model_vendor_new(ele->idx, vendor_id,
 									mod_id);
-			if (!mod) {
+			if (!mod || !element_add_model(ele, mod)) {
+				mesh_model_free(mod);
 				element_free(ele);
 				goto fail;
 			}
 
-			l_queue_push_tail(ele->models, mod);
 			data += 4;
 			len -= 4;
 		}
@@ -1151,12 +1203,9 @@ static void get_models_from_properties(struct node_element *ele,
 		while (l_dbus_message_iter_next_entry(&ids, &mod_id)) {
 			struct mesh_model *mod;
 
-			/* Skip internally implemented models */
-			if ((VENDOR_ID_MASK | mod_id) == CONFIG_SRV_MODEL)
-				continue;
-
 			mod = mesh_model_new(ele->idx, mod_id);
-			l_queue_push_tail(ele->models, mod);
+			if (!element_add_model(ele, mod))
+				mesh_model_free(mod);
 		}
 		return;
 	}
@@ -1166,7 +1215,8 @@ static void get_models_from_properties(struct node_element *ele,
 		struct mesh_model *mod;
 
 		mod = mesh_model_vendor_new(ele->idx, vendor_id, mod_id);
-		l_queue_push_tail(ele->models, mod);
+		if (!element_add_model(ele, mod))
+			mesh_model_free(mod);
 	}
 }
 
@@ -1216,7 +1266,11 @@ static bool get_element_properties(struct mesh_node *node, const char *path,
 	if (!idx || !loc || !mods || !vendor_mods)
 		goto fail;
 
-	l_queue_push_tail(node->elements, ele);
+	if (l_queue_find(node->elements, match_element_idx,
+						L_UINT_TO_PTR(ele->idx)))
+		goto fail;
+
+	l_queue_insert(node->elements, ele, compare_element_idx, NULL);
 	return true;
 fail:
 	l_free(ele);
-- 
2.19.1

[PATCH BlueZ v2 4/5] mesh: Check that element indexes are consecutive

[Michał Lowas-Rzechonek]

This patch checks that application does not omit element indexes.

---
 mesh/node.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/mesh/node.c b/mesh/node.c
index bfb0dc5b4..8ff75c110 100644
--- a/mesh/node.c
+++ b/mesh/node.c
@@ -898,6 +898,7 @@ uint8_t node_friend_mode_get(struct mesh_node *node)
 uint16_t node_generate_comp(struct mesh_node *node, uint8_t *buf, uint16_t sz)
 {
 	uint16_t n, features;
+	uint16_t num_ele = 0;
 	const struct l_queue_entry *ele_entry;
 
 	if (!node || !node->comp || sz < MIN_COMP_SIZE)
@@ -935,6 +936,11 @@ uint16_t node_generate_comp(struct mesh_node *node, uint8_t *buf, uint16_t sz)
 		uint8_t num_s = 0, num_v = 0;
 		uint8_t *mod_buf;
 
+		if (ele->idx != num_ele)
+			return 0;
+
+		num_ele++;
+
 		/* At least fit location and zeros for number of models */
 		if ((n + 4) > sz)
 			return n;
@@ -997,6 +1003,9 @@ element_done:
 
 	}
 
+	if (!num_ele)
+		return 0;
+
 	return n;
 }
 
-- 
2.19.1

[PATCH BlueZ v2 5/5] mesh: Check that config server is present in primary element

[Michał Lowas-Rzechonek]

This verifies that Config Server model is supported by element #0, and
is not supported by any other element.
---
 mesh/node.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/mesh/node.c b/mesh/node.c
index 8ff75c110..fe8cad008 100644
--- a/mesh/node.c
+++ b/mesh/node.c
@@ -899,6 +899,8 @@ uint16_t node_generate_comp(struct mesh_node *node, uint8_t *buf, uint16_t sz)
 {
 	uint16_t n, features;
 	uint16_t num_ele = 0;
+	uint8_t *cfgmod_idx = NULL;
+
 	const struct l_queue_entry *ele_entry;
 
 	if (!node || !node->comp || sz < MIN_COMP_SIZE)
@@ -961,6 +963,9 @@ uint16_t node_generate_comp(struct mesh_node *node, uint8_t *buf, uint16_t sz)
 			mod_id = mesh_model_get_model_id(
 					(const struct mesh_model *) mod);
 
+			if (mod_id == CONFIG_SRV_MODEL)
+				cfgmod_idx = &ele->idx;
+
 			if ((mod_id & VENDOR_ID_MASK) == VENDOR_ID_MASK) {
 				if (n + 2 > sz)
 					goto element_done;
@@ -1006,6 +1011,9 @@ element_done:
 	if (!num_ele)
 		return 0;
 
+	if (!cfgmod_idx || *cfgmod_idx != PRIMARY_ELE_IDX)
+		return 0;
+
 	return n;
 }
 
-- 
2.19.1

Re: [PATCH BlueZ v2 0/5] Use composition data to validate app against the node

[Stotland, Inga]

[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]

Hi Michal,

On Tue, 2019-07-23 at 12:06 +0200, Michał Lowas-Rzechonek wrote:
> This patchset streamlines app validation by creating a temporary node
> during Attach, Join and CreateNetwork calls, then validating its
> composition data to:
>  - fit in Config Model Composition Data Get message
>  - declare mandatory models on primary element
>  - declare consecutive element indexes
> 
> During Attach call, temporary composition data is also compared with
> data generated for existing node, guaranteeing immutablity required
> by
> the specification.
> 
> Michał Lowas-Rzechonek (5):
>   mesh: Convert void pointers to anonymous unions in
> managed_obj_request
>   mesh: Validate application by comparing composition data
>   mesh: Keep element and model lists sorted and unique
>   mesh: Check that element indexes are consecutive
>   mesh: Check that config server is present in primary element
> 
>  mesh/mesh-defs.h |   2 +
>  mesh/node.c      | 507 ++++++++++++++++++++++-----------------------
> --
>  2 files changed, 241 insertions(+), 268 deletions(-)
> 

I agree with the general idea of this change. There are just a couple
of things that need to be fixed (sent as comments to a specific patch).

Regards,
Inga

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3265 bytes --]