Re: [PATCH BlueZ v3 2/3] mesh: Explicit Remote/Local Device key usage

["Stotland, Inga" <inga.stotland@intel.com>]

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

Hi Brian,

On Thu, 2019-09-26 at 11:14 -0700, Brian Gix wrote:
> When sending or receiving Device Key (privileged) mesh messages, the
> remote vs local 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..e9b346102 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 remote = (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', &remote);
>  	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);

I think that mesh_model_send() should be modified to return an error
code (int) instead of boolean. Otherwise, it may fail for a different
reason than a mismatch in device key and the returned error is
misleading.
In fact, the Send() call returns D-Bus "Failed" error upon getting
"false" from mesh_model_send() and this is not documented in the API
doc.

This probably should go a separate fix.

>  
>  	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,

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