Re: [PATCH BlueZ v4 3/3] mesh: Handle messages encrypted with a remote device key

["Gix, Brian" <brian.gix@intel.com>]

On Wed, 2019-07-03 at 13:42 +0200, Michał Lowas-Rzechonek wrote:
> This adds ability to receive messages encrypted using known remote
> device key. Such a key must be added to the node's keyring using
> ImportRemoteNode() method of org.bluez.mesh.Management1 interface.
> 
> Decrypted messages are then forwarded to the application using
> DevKeyMessageReceived() D-Bus API.
> 
> Also, messages originating from a local node and encrypted using local
> device key are forwarde to the application as well, if they weren't
> handled by internal model. This allows e.g. receiving status messages
> from a local Config Server in the application.
> ---
>  mesh/model.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 63 insertions(+), 6 deletions(-)
> 
> diff --git a/mesh/model.c b/mesh/model.c
> index aae913d92..0ea45987f 100644
> --- a/mesh/model.c
> +++ b/mesh/model.c
> @@ -308,7 +308,7 @@ static void forward_model(void *a, void *b)
>  
>  	l_debug("model %8.8x with idx %3.3x", mod->id, fwd->idx);
>  
> -	if (fwd->idx != APP_IDX_DEV_LOCAL &&
> +	if (fwd->idx != APP_IDX_DEV_LOCAL && fwd->idx != APP_IDX_DEV_REMOTE &&
>  					!has_binding(mod->bindings, fwd->idx))
>  		return;
>  
> @@ -359,16 +359,25 @@ static int dev_packet_decrypt(struct mesh_node *node, const uint8_t *data,
>  				uint16_t dst, uint8_t key_id, uint32_t seq,
>  				uint32_t iv_idx, uint8_t *out)
>  {
> +	uint8_t dev_key[16];
>  	const uint8_t *key;
>  
>  	key = node_get_device_key(node);
>  	if (!key)
> -		return false;
> +		return -1;
>  
>  	if (mesh_crypto_payload_decrypt(NULL, 0, data, size, szmict, src,
>  					dst, key_id, seq, iv_idx, out, key))
>  		return APP_IDX_DEV_LOCAL;
>  
> +	if (!keyring_get_remote_dev_key(node, src, dev_key))
> +		return -1;
> +
> +	key = dev_key;
> +	if (mesh_crypto_payload_decrypt(NULL, 0, data, size, szmict, src,
> +					dst, key_id, seq, iv_idx, out, key))
> +		return APP_IDX_DEV_REMOTE;
> +
>  	return -1;
>  }
>  
> @@ -695,6 +704,47 @@ static int add_sub(struct mesh_net *net, struct mesh_model *mod,
>  	return MESH_STATUS_SUCCESS;
>  }
>  
> +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)
> +{
> +	struct l_dbus *dbus = dbus_get_bus();
> +	struct l_dbus_message *msg;
> +	struct l_dbus_message_builder *builder;
> +	const char *owner;
> +	const char *path;
> +
> +	owner = node_get_owner(node);
> +	path = node_get_element_path(node, ele_idx);
> +	if (!path || !owner)
> +		return;
> +
> +	l_debug("Send \"DevKeyMessageReceived\"");
> +
> +	msg = l_dbus_message_new_method_call(dbus, owner, path,
> +						MESH_ELEMENT_INTERFACE,
> +						"DevKeyMessageReceived");
> +
> +	builder = l_dbus_message_builder_new(msg);
> +
> +	if (!l_dbus_message_builder_append_basic(builder, 'q', &src))
> +		goto error;
> +
> +	if (!l_dbus_message_builder_append_basic(builder, 'q', &net_idx))
> +		goto error;
> +
> +	if (!dbus_append_byte_array(builder, data, size))
> +		goto error;
> +
> +	if (!l_dbus_message_builder_finalize(builder))
> +		goto error;
> +
> +	l_dbus_send(dbus, msg);
> +
> +error:
> +	l_dbus_message_builder_destroy(builder);

I think there is a potential memory leak, not just here but in the other methods that use this technique to
construct method calls to the App  (sorry I did not catch this in earlier situations)

To get to the error label, we will have a "msg" which is never sent or freed...   I think here, and every where
else where we have a potentially failed builder, We need to call l_dbus_message_unref(msg) if we don't end up
sending the msg.

But I would like Inga's opinion on this judgement on this as well.

Otherwise, I think I will be ready to apply this whole patch-set.


> +}
> +
>  static void send_msg_rcvd(struct mesh_node *node, uint8_t ele_idx, bool is_sub,
>  					uint16_t src, uint16_t key_idx,
>  					uint16_t size, const uint8_t *data)
> @@ -843,10 +893,17 @@ bool mesh_model_rx(struct mesh_node *node, bool szmict, uint32_t seq0,
>  		 * Cycle through external models if the message has not been
>  		 * handled by internal models
>  		 */
> -		if (forward.has_dst && !forward.done)
> -			send_msg_rcvd(node, i, is_subscription, src,
> -					forward.idx, forward.size,
> -					forward.data);
> +		if (forward.has_dst && !forward.done) {
> +			if ((decrypt_idx & APP_IDX_MASK) == decrypt_idx)
> +				send_msg_rcvd(node, i, is_subscription, src,
> +						forward.idx, forward.size,
> +						forward.data);
> +			else if (decrypt_idx == APP_IDX_DEV_REMOTE ||
> +				(decrypt_idx == APP_IDX_DEV_LOCAL &&
> +				 mesh_net_is_local_address(net, src)))
> +				send_dev_key_msg_rcvd(node, i, src, 0,
> +						forward.size, forward.data);
> +		}
>  
>  		/*
>  		 * Either the message has been processed internally or