[PATCH BlueZ v2 0/3] mesh: Fix Remote/Local dev key usage

[PATCH BlueZ v2 0/3] mesh: Fix Remote/Local dev key usage

[Brian Gix]

As discussed at the Bluetooth SIG Mesh Working Group, this patch set
locks down usage of Device Keys to nodes that have explicit permission
to use them.  This permission will typically be granted by the
Provisioner (mesh network owner) in the form of Application access to
the Device Key of the node that requests are directed to. This
*includes* the access to a nodes local Config Server.

Usage Model:

DevKeySend(remote == false)
	node.json is the only Device Key used. However, it is illegal
	for loopback messages (where DST == any of our element unicast
	addresses) so this call will return an immediate "Unauthorized"
	error in such a case.

DevKeySend(remote == true)
	keyring Device Key matching the DST address is used, with no
	fallback to the node.json key.  If the DST does not exist in
	the keyring, then an immediate "Unauthorized" error will be
	returned.

In the past, we would first try the Keyring, and automatically
fallback to the node.json key, but this was a backdoor to always
letting Apps modify their own Config Server.  The daemon can't tell
the difference between Client and Server opcodes, so this was
always a and really not a very good guess.


DevKeyMessageReceived(local == true)
	node.json (local) Device Key was used to decrypt this
	message, and message has full permission to change any
	model's states.  This message is always received from
	an *external* node that knows our Device Key.  (If an
	App wants to change it's own App controlled states, it
	already can).

DevKeyMessageReceived(local == false)
	Keyring Device Key was used to decrypt this message,
	so it was a *remote* node's device key.  This will most
	likely be a response to an App controlled Client model.
	However, if it is a Server message that changes a state,
	or requests the state value, the App should silently
	*reject* it...  For that operation, the remote node
	needs to know *our* Device Key.

This differentiation is needed with received messages, because *any*
message can be secured with a device key.  But just as with the Config
Client/Server, the *correct* device key must be used for any state
changes and state requests.  If a provisioner has an On/Off server for
example, it might get an "Off" request from a remote node, but it should
only execute the "Off" if it was secured with local Device Key...  not
the remote Device Key.

Likewise, when *responding* to legal Server Commands secured with local
Device Keys, the App must respond with DevKeySend(remote == false),
because that was the only key that would have been legal for the remote
node to have used.

The "remote" parameter name for Send, and "local" name for Receive was
deliberate as the "true" value in both cases is the more privledged form.


Brian Gix (3):
  mesh: Add local/remote bools to DevKey transactions
  mesh: Use explicit Local vs Remote Device key usage
  mesh: Fix Key Ring permissions for local nodes

 doc/mesh-api.txt | 15 ++++++++++++---
 mesh/manager.c   |  4 ----
 mesh/model.c     | 11 +++++++----
 mesh/node.c      | 39 +++++++++++++++------------------------
 4 files changed, 34 insertions(+), 35 deletions(-)

-- 
2.21.0

[PATCH BlueZ v2 1/3] mesh: Add local/remote bools to DevKey transactions

[Brian Gix]

DevKey operations require authorization on the part of the applications
making the requests. Messages to state changing Servers should use keys
from the local Key Database to demonstrate authority.
---
 doc/mesh-api.txt | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/doc/mesh-api.txt b/doc/mesh-api.txt
index 9b9f4e3de..ec502dbd7 100644
--- a/doc/mesh-api.txt
+++ b/doc/mesh-api.txt
@@ -245,7 +245,7 @@ Methods:
 			org.bluez.mesh.Error.InvalidArguments
 			org.bluez.mesh.Error.NotFound
 
-	void DevKeySend(object element_path, uint16 destination,
+	void DevKeySend(object element_path, uint16 destination, boolean remote,
 					uint16 net_index, array{byte} data)
 
 		This method is used to send a message originated by a local
@@ -259,6 +259,11 @@ Methods:
 		destination must be a uint16 to a unicast address, or a well
 		known group address.
 
+		The remote parameter, if true, looks up the destination address
+		in the key database to encrypt the message.  If false the local
+		device key is used. If remote is true, but requested key does
+		not exist, a NotFound error will be returned.
+
 		The net_index parameter is the subnet index of the network on
 		which the message is to be sent.
 
@@ -782,8 +787,8 @@ Methods:
 
 		The data parameter is the incoming message.
 
-	void DevKeyMessageReceived(uint16 source, uint16 net_index,
-							array{byte} data)
+	void DevKeyMessageReceived(uint16 source, boolean local,
+					uint16 net_index, array{byte} data)
 
 		This method is called by meshd daemon when a message arrives
 		addressed to the application, which was sent with the remote
@@ -792,6 +797,10 @@ Methods:
 		The source parameter is unicast address of the remote
 		node-element that sent the message.
 
+		The local parameter if true indicates that the local device key
+		was used to decrypt the message. False indicates that the remote
+		nodes device key was used.
+
 		The net_index parameter indicates what subnet the message was
 		received on, and if a response is required, the same subnet
 		must be used to send the response.
-- 
2.21.0

[PATCH BlueZ v2 2/3] mesh: Use explicit Local vs Remote Device key usage

[Brian Gix]

When sending or receiving Device Key (privileged) mesh messages, the
local vs remote device key must be specified.  This allows Apps to
specify Key Ring stored device keys, and sanity checks that the correct
key exists before allowing the transmission. Loopback messages to local
servers *must* use keys from the Key Ring to indicate privilege has been
granted.
---
 mesh/model.c | 11 +++++++----
 mesh/node.c  | 25 +++++++++++++++----------
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/mesh/model.c b/mesh/model.c
index a06b684a5..c029a8365 100644
--- a/mesh/model.c
+++ b/mesh/model.c
@@ -735,14 +735,16 @@ static int add_sub(struct mesh_net *net, struct mesh_model *mod,
 }
 
 static void send_dev_key_msg_rcvd(struct mesh_node *node, uint8_t ele_idx,
-					uint16_t src, uint16_t net_idx,
-					uint16_t size, const uint8_t *data)
+					uint16_t src, uint16_t app_idx,
+					uint16_t net_idx, uint16_t size,
+					const uint8_t *data)
 {
 	struct l_dbus *dbus = dbus_get_bus();
 	struct l_dbus_message *msg;
 	struct l_dbus_message_builder *builder;
 	const char *owner;
 	const char *path;
+	bool local = (app_idx == APP_IDX_DEV_LOCAL);
 
 	owner = node_get_owner(node);
 	path = node_get_element_path(node, ele_idx);
@@ -758,6 +760,7 @@ static void send_dev_key_msg_rcvd(struct mesh_node *node, uint8_t ele_idx,
 	builder = l_dbus_message_builder_new(msg);
 
 	l_dbus_message_builder_append_basic(builder, 'q', &src);
+	l_dbus_message_builder_append_basic(builder, 'b', &local);
 	l_dbus_message_builder_append_basic(builder, 'q', &net_idx);
 	dbus_append_byte_array(builder, data, size);
 
@@ -936,8 +939,8 @@ bool mesh_model_rx(struct mesh_node *node, bool szmict, uint32_t seq0,
 			else if (decrypt_idx == APP_IDX_DEV_REMOTE ||
 				(decrypt_idx == APP_IDX_DEV_LOCAL &&
 				 mesh_net_is_local_address(net, src, 1)))
-				send_dev_key_msg_rcvd(node, i, src, 0,
-						forward.size, forward.data);
+				send_dev_key_msg_rcvd(node, i, src, decrypt_idx,
+						0, forward.size, forward.data);
 		}
 
 		/*
diff --git a/mesh/node.c b/mesh/node.c
index b6824f505..833377e99 100644
--- a/mesh/node.c
+++ b/mesh/node.c
@@ -1976,7 +1976,8 @@ static struct l_dbus_message *dev_key_send_call(struct l_dbus *dbus,
 	const char *sender, *ele_path;
 	struct l_dbus_message_iter iter_data;
 	struct node_element *ele;
-	uint16_t dst, net_idx, src;
+	uint16_t dst, app_idx, net_idx, src;
+	bool remote;
 	uint8_t *data;
 	uint32_t len;
 
@@ -1987,8 +1988,12 @@ static struct l_dbus_message *dev_key_send_call(struct l_dbus *dbus,
 	if (strcmp(sender, node->owner))
 		return dbus_error(msg, MESH_ERROR_NOT_AUTHORIZED, NULL);
 
-	if (!l_dbus_message_get_arguments(msg, "oqqay", &ele_path, &dst,
-							&net_idx, &iter_data))
+	if (!l_dbus_message_get_arguments(msg, "oqbqay", &ele_path, &dst,
+						&remote, &net_idx, &iter_data))
+		return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
+
+	/* Loopbacks to local servers must use *remote* addressing */
+	if (!remote && mesh_net_is_local_address(node->net, dst, 1))
 		return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
 
 	ele = l_queue_find(node->elements, match_element_path, ele_path);
@@ -1999,13 +2004,13 @@ static struct l_dbus_message *dev_key_send_call(struct l_dbus *dbus,
 	src = node_get_primary(node) + ele->idx;
 
 	if (!l_dbus_message_iter_get_fixed_array(&iter_data, &data, &len) ||
-					!len || len > MAX_MSG_LEN)
+						!len || len > MAX_MSG_LEN)
 		return dbus_error(msg, MESH_ERROR_INVALID_ARGS,
 							"Incorrect data");
 
-	/* TODO: use net_idx */
-	if (!mesh_model_send(node, src, dst, APP_IDX_DEV_REMOTE, net_idx,
-							DEFAULT_TTL, data, len))
+	app_idx = remote ? APP_IDX_DEV_REMOTE : APP_IDX_DEV_LOCAL;
+	if (!mesh_model_send(node, src, dst, app_idx, net_idx, DEFAULT_TTL,
+								data, len))
 		return dbus_error(msg, MESH_ERROR_NOT_FOUND, NULL);
 
 	return l_dbus_message_new_method_return(msg);
@@ -2226,9 +2231,9 @@ static void setup_node_interface(struct l_dbus_interface *iface)
 						"element_path", "destination",
 						"key_index", "data");
 	l_dbus_interface_method(iface, "DevKeySend", 0, dev_key_send_call,
-						"", "oqqay", "element_path",
-						"destination", "net_index",
-						"data");
+						"", "oqbqay", "element_path",
+						"destination", "remote",
+						"net_index", "data");
 	l_dbus_interface_method(iface, "Publish", 0, publish_call, "", "oqay",
 					"element_path", "model_id", "data");
 	l_dbus_interface_method(iface, "VendorPublish", 0, vendor_publish_call,
-- 
2.21.0

[PATCH BlueZ v2 3/3] mesh: Fix Key Ring permissions for local nodes

[Brian Gix]

We do *not* automatically create populated key rings for imported or
joined nodes, but we also do not *forbid* any node from adding a key
in it's possesion to the local key ring.
---
 mesh/manager.c |  4 ----
 mesh/node.c    | 14 --------------
 2 files changed, 18 deletions(-)

diff --git a/mesh/manager.c b/mesh/manager.c
index 501ec10fe..96d9af915 100644
--- a/mesh/manager.c
+++ b/mesh/manager.c
@@ -298,10 +298,6 @@ static struct l_dbus_message *import_node_call(struct l_dbus *dbus,
 		return dbus_error(msg, MESH_ERROR_INVALID_ARGS,
 							"Bad device key");
 
-	if (mesh_net_is_local_address(net, primary, num_ele))
-		return dbus_error(msg, MESH_ERROR_INVALID_ARGS,
-					"Cannot overwrite local device key");
-
 	if (!keyring_put_remote_dev_key(node, primary, num_ele, key))
 		return dbus_error(msg, MESH_ERROR_FAILED, NULL);
 
diff --git a/mesh/node.c b/mesh/node.c
index 833377e99..cad1ac8fb 100644
--- a/mesh/node.c
+++ b/mesh/node.c
@@ -1692,23 +1692,9 @@ static void get_managed_objects_cb(struct l_dbus_message *msg, void *user_data)
 					import->net_idx, import->net_key))
 			goto fail;
 
-		memcpy(net_key.old_key, import->net_key, 16);
-		net_key.net_idx = import->net_idx;
-		if (import->flags.kr)
-			net_key.phase = KEY_REFRESH_PHASE_TWO;
-		else
-			net_key.phase = KEY_REFRESH_PHASE_NONE;
-
 		/* Initialize directory for storing keyring info */
 		init_storage_dir(node);
 
-		if (!keyring_put_remote_dev_key(node, import->unicast,
-						num_ele, import->dev_key))
-			goto fail;
-
-		if (!keyring_put_net_key(node, import->net_idx, &net_key))
-			goto fail;
-
 	} else {
 		/* Callback for create node request */
 		struct keyring_net_key net_key;
-- 
2.21.0