[PATCH BlueZ] mesh: Add remote dev key support and cleanup

[PATCH BlueZ] mesh: Add remote dev key support and cleanup

[Brian Gix]

If a local database of remote device keys exist, they will be used in
decryption attempts of received privledged messages. They will also
be cleaned up when local node storgae is deleted.
---
 mesh/model.c   |  13 +++++++-
 mesh/net.h     |   1 +
 mesh/node.c    |  49 ++++++++++++++++++++++++++++
 mesh/node.h    |   1 +
 mesh/storage.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++-----------
 mesh/storage.h |   2 ++
 6 files changed, 147 insertions(+), 20 deletions(-)

diff --git a/mesh/model.c b/mesh/model.c
index a632d80e1..032f25537 100644
--- a/mesh/model.c
+++ b/mesh/model.c
@@ -360,12 +360,23 @@ static int dev_packet_decrypt(struct mesh_node *node, const uint8_t *data,
 
 	dev_key = node_get_device_key(node);
 	if (!dev_key)
-		return false;
+		goto try_remote;
 
+	/* Always attempt decryption with local dev_key first */
 	if (mesh_crypto_payload_decrypt(NULL, 0, data, size, szmict, src,
 					dst, key_id, seq, iv_idx, out, dev_key))
 		return APP_IDX_DEV;
 
+try_remote:
+	/* Try remote device key if available */
+	dev_key = node_get_remote_device_key(node, src);
+	if (!dev_key)
+		return -1;
+
+	if (mesh_crypto_payload_decrypt(NULL, 0, data, size, szmict, src,
+					dst, key_id, seq, iv_idx, out, dev_key))
+		return APP_IDX_DEV_RMT;
+
 	return -1;
 }
 
diff --git a/mesh/net.h b/mesh/net.h
index e040e19fa..46f91f6b5 100644
--- a/mesh/net.h
+++ b/mesh/net.h
@@ -39,6 +39,7 @@ struct mesh_node;
 #define CREDFLAG_MASK	0x1000
 #define APP_IDX_MASK	0x0fff
 #define APP_IDX_DEV	0x7fff
+#define APP_IDX_DEV_RMT	0x6fff
 #define APP_IDX_ANY	0x8000
 #define APP_IDX_NET	0xffff
 
diff --git a/mesh/node.c b/mesh/node.c
index 157991dad..fe0488108 100644
--- a/mesh/node.c
+++ b/mesh/node.c
@@ -54,6 +54,8 @@
 #define DEFAULT_CRPL 10
 #define DEFAULT_SEQUENCE_NUMBER 0
 
+#define REMOTE_CACHE_LEN 5
+
 struct node_element {
 	char *path;
 	struct l_queue *models;
@@ -76,6 +78,7 @@ struct mesh_node {
 	char *path;
 	void *jconfig;
 	char *cfg_file;
+	struct l_queue *remotes;
 	uint32_t disc_watch;
 	time_t upd_sec;
 	uint32_t seq_number;
@@ -100,6 +103,11 @@ struct mesh_node {
 	uint8_t beacon;
 };
 
+struct remote_node {
+	uint16_t unicast;
+	uint8_t dev_key[16];
+};
+
 struct attach_obj_request {
 	node_attach_ready_func_t cb;
 	struct mesh_node *node;
@@ -112,6 +120,14 @@ struct join_obj_request {
 
 static struct l_queue *nodes;
 
+static bool match_remote_unicast(const void *a, const void *b)
+{
+	const struct remote_node *remote = a;
+	uint16_t unicast = L_PTR_TO_UINT(b);
+
+	return remote->unicast == unicast;
+}
+
 static bool match_node_unicast(const void *a, const void *b)
 {
 	const struct mesh_node *node = a;
@@ -454,6 +470,39 @@ const uint8_t *node_get_device_key(struct mesh_node *node)
 		return node->dev_key;
 }
 
+const uint8_t *node_get_remote_device_key(struct mesh_node *node, uint16_t src)
+{
+	struct remote_node *remote;
+
+	if (!node)
+		return NULL;
+
+	if (!node->remotes)
+		node->remotes = l_queue_new();
+
+	remote = l_queue_remove_if(node->remotes, match_remote_unicast,
+							L_UINT_TO_PTR(src));
+	if (remote) {
+		/* Keep most recently used at head */
+		l_queue_push_head(node->remotes, remote);
+		return remote->dev_key;
+	}
+
+	if (l_queue_length(node->remotes) >= REMOTE_CACHE_LEN) {
+		remote = l_queue_peek_tail(node->remotes);
+		l_queue_remove(node->remotes, remote);
+	} else
+		remote = l_new(struct remote_node, 1);
+
+	if (storage_get_remote_dev_key(node, src, remote->dev_key)) {
+		l_queue_push_head(node->remotes, remote);
+		return remote->dev_key;
+	}
+
+	l_free(remote);
+	return NULL;
+}
+
 void node_set_token(struct mesh_node *node, uint8_t token[8])
 {
 	memcpy(node->token, token, 8);
diff --git a/mesh/node.h b/mesh/node.h
index ebc82ffb8..8dc30855f 100644
--- a/mesh/node.h
+++ b/mesh/node.h
@@ -51,6 +51,7 @@ void node_set_token(struct mesh_node *node, uint8_t token[8]);
 const uint8_t *node_get_token(struct mesh_node *node);
 void node_set_device_key(struct mesh_node *node, uint8_t key[16]);
 const uint8_t *node_get_device_key(struct mesh_node *node);
+const uint8_t *node_get_remote_device_key(struct mesh_node *node, uint16_t src);
 void node_set_num_elements(struct mesh_node *node, uint8_t num_ele);
 uint8_t node_get_num_elements(struct mesh_node *node);
 bool node_parse_composition(struct mesh_node *node, uint8_t *buf, uint16_t len);
diff --git a/mesh/storage.c b/mesh/storage.c
index 8a70b5696..8aca12ee0 100644
--- a/mesh/storage.c
+++ b/mesh/storage.c
@@ -28,6 +28,7 @@
 #include <unistd.h>
 #include <dirent.h>
 #include <libgen.h>
+#include <ftw.h>
 
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -61,6 +62,20 @@ static bool simple_match(const void *a, const void *b)
 	return a == b;
 }
 
+/* This is a thread-safe always malloced version of dirname which will work
+ * regardless of which underlying dirname implimentation is used
+ */
+static char *alloc_dirname(const char *path)
+{
+	char *tmp = l_strdup(path);
+	char *dir;
+
+	dir = dirname(tmp);
+	strncpy(tmp, dir, strlen(path) + 1);
+
+	return tmp;
+}
+
 static bool read_node_cb(struct mesh_db_node *db_node, void *user_data)
 {
 	struct mesh_node *node = user_data;
@@ -634,15 +649,65 @@ fail:
 	return false;
 }
 
+bool storage_get_remote_dev_key(struct mesh_node *node, uint16_t unicast,
+							uint8_t dev_key[16])
+{
+	char *cfgname, *dir_name;
+	char *key_file;
+	size_t len;
+	int fd;
+	bool result = false;
+
+	if (!node)
+		return false;
+
+	cfgname = (char *) node_cfg_file_get(node);
+	if (!cfgname)
+		return false;
+
+	dir_name = alloc_dirname(cfgname);
+
+	len = strlen(dir_name) + 9 + 5;
+	key_file = l_malloc(len);
+	snprintf(key_file, len, "%s%s%4.4x", dir_name, "/remotes/", unicast);
+	l_free(dir_name);
+
+	fd = open(key_file, O_RDONLY);
+	if (fd) {
+		if (read(fd, dev_key, 16) == 16)
+			result = true;
+
+		close(fd);
+	}
+
+	l_free(key_file);
+	return result;
+}
+
+static int del_fobject(const char *fpath, const struct stat *sb, int typeflag,
+						struct FTW *ftwbuf)
+{
+	switch (typeflag) {
+	case FTW_DP:
+		rmdir(fpath);
+		l_debug("RMDIR %s", fpath);
+		break;
+
+	case FTW_SL:
+	default:
+		remove(fpath);
+		l_debug("RM %s", fpath);
+		break;
+	}
+	return 0;
+}
+
 /* Permanently remove node configuration */
 void storage_remove_node_config(struct mesh_node *node)
 {
-	char *cfgname;
+	char *cfgname, *dir_name, *mesh_path, *mesh_name;
 	struct json_object *jnode;
-	const char *dir_name;
 	uint16_t node_id;
-	size_t len;
-	char *bak;
 
 	if (!node)
 		return;
@@ -658,26 +723,24 @@ void storage_remove_node_config(struct mesh_node *node)
 	if (!cfgname)
 		return;
 
-	l_debug("Delete node config file %s", cfgname);
-	remove(cfgname);
-
-	/* Delete the backup file */
-	len = strlen(cfgname) + 5;
-	bak = l_malloc(len);
-	strncpy(bak, cfgname, len);
-	bak = strncat(bak, ".bak", 5);
-	remove(bak);
-	l_free(bak);
+	dir_name = alloc_dirname(cfgname);
+	l_debug("Delete node config %s", dir_name);
 
-	/* Delete the node directory */
-	dir_name = dirname(cfgname);
+	/* Make sure path name of node follows expected guidelines */
+	mesh_path = alloc_dirname(dir_name);
+	mesh_name = basename(mesh_path);
+	if (strcmp(mesh_name, "mesh"))
+		goto done;
 
-	l_debug("Delete directory %s", dir_name);
-	rmdir(dir_name);
+	nftw(dir_name, del_fobject, 5, FTW_DEPTH | FTW_PHYS);
 
-	l_free(cfgname);
 	node_cfg_file_set(node, NULL);
 
 	node_id = node_id_get(node);
 	l_queue_remove(node_ids, L_UINT_TO_PTR(node_id));
+	l_free(cfgname);
+
+done:
+	l_free(dir_name);
+	l_free(mesh_path);
 }
diff --git a/mesh/storage.h b/mesh/storage.h
index d71d11b8e..6ed758b6e 100644
--- a/mesh/storage.h
+++ b/mesh/storage.h
@@ -49,3 +49,5 @@ bool storage_set_device_key(struct mesh_node *node, uint8_t dev_key[16]);
 bool storage_set_unicast(struct mesh_node *node, uint16_t unicast);
 bool storage_set_key_refresh_phase(struct mesh_net *net, uint16_t net_idx,
 								uint8_t phase);
+bool storage_get_remote_dev_key(struct mesh_node *node, uint16_t unicast,
+							uint8_t dev_key[16]);
-- 
2.14.5

Re: [PATCH BlueZ] mesh: Add remote dev key support and cleanup

[Michał Lowas-Rzechonek]

Hi Brian,

A few comments.

On 04/23, Brian Gix wrote:
> +try_remote:
> +	/* Try remote device key if available */
> +	dev_key = node_get_remote_device_key(node, src);
> +	if (!dev_key)
> +		return -1;
> +
> +	if (mesh_crypto_payload_decrypt(NULL, 0, data, size, szmict, src,
> +					dst, key_id, seq, iv_idx, out, dev_key))
> +		return APP_IDX_DEV_RMT;
> +
What's the purpose of APP_IDX_DEV_RMT?

I'm not a fan of magic values (and I hope APP_IDX_DEV goes away afterw
we manage to implement proposed Config/Provisioning API), but at least
APP_IDX_DEV is used to indicate 'privileged' loopback messages.

Adding IDX_DEV_RMT doesn't seem necessary.

> diff --git a/mesh/node.c b/mesh/node.c
> index 157991dad..fe0488108 100644
> --- a/mesh/node.c
> +++ b/mesh/node.c
> @@ -54,6 +54,8 @@
>  #define DEFAULT_CRPL 10
>  #define DEFAULT_SEQUENCE_NUMBER 0
>  
> +#define REMOTE_CACHE_LEN 5
By the way: why the limits here are set so low (and there doesn't seem
to be a way to change them...)? I don't think we should put artificial
constraints on network size, so both CRPL and remote cache could safely
be set to cover full unicast space (32768). In any practical
application, the device running Linux + bluetooth will have more than
enough RAM for this.

Getting rid of these constraints would make the implementation
substiantially simpler (no MRU, for example).

FYI, we're running networks with couples of hundreds devices, and plan
to expand them even further.

> +/* This is a thread-safe always malloced version of dirname which will work
> + * regardless of which underlying dirname implimentation is used
> + */
> +static char *alloc_dirname(const char *path)
> +{
I was under impression that mesh daemon operates inside a main loop, so
it doesn't need to worry about thread safety?

> +bool storage_get_remote_dev_key(struct mesh_node *node, uint16_t unicast,
> +							uint8_t dev_key[16])
> +{
> (...)
> +	dir_name = alloc_dirname(cfgname);
I don't see why the remote key storage should be kept separately (and in
binary files?) from the main node json. It makes the implementation much
more complicated, as you need to worry about directories etc.

I believie that keeping remote keys in the same place, as a JSON object,
would be much simpler.

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

RE: [PATCH BlueZ] mesh: Add remote dev key support and cleanup

[Gix, Brian]

Hi Michal,

> From: Michał Lowas-Rzechonek [mailto:michal.lowas-rzechonek@silvair.com]
> > +try_remote:
> > +	/* Try remote device key if available */
> > +	dev_key = node_get_remote_device_key(node, src);
> > +	if (!dev_key)
> > +		return -1;
> > +
> > +	if (mesh_crypto_payload_decrypt(NULL, 0, data, size, szmict, src,
> > +					dst, key_id, seq, iv_idx, out,
> dev_key))
> > +		return APP_IDX_DEV_RMT;
> > +
> What's the purpose of APP_IDX_DEV_RMT?

App Index is an internal way to track which credentials were used to decrypt a message.  In theory, if the incoming message used the local device key, then the message is addressed to one of the local servers, and if the incoming message was decrypted by a *remote* device key it should be addressed to a local Client (as a response from a remote server).

However, also importantly, if a message that requires a response is received on *any* key (local dev, remote dev, or App Key) the response is supposed to be sent with the same key.

To enable that, we need to differentiate between the two possible device keys.

In any case, these "Magic Numbers" are internal use only, and do not show up on the external interface.  When *sending* Dev Key message from the Application, we will try the remote nodes device key first, and if it does not exist we fall back to the local key.

> 
> I'm not a fan of magic values (and I hope APP_IDX_DEV goes away afterw we
> manage to implement proposed Config/Provisioning API), but at least
> APP_IDX_DEV is used to indicate 'privileged' loopback messages.
> 
> Adding IDX_DEV_RMT doesn't seem necessary.
> 
> > diff --git a/mesh/node.c b/mesh/node.c index 157991dad..fe0488108
> > 100644
> > --- a/mesh/node.c
> > +++ b/mesh/node.c
> > @@ -54,6 +54,8 @@
> >  #define DEFAULT_CRPL 10
> >  #define DEFAULT_SEQUENCE_NUMBER 0
> >
> > +#define REMOTE_CACHE_LEN 5
> By the way: why the limits here are set so low (and there doesn't seem to be
> a way to change them...)? I don't think we should put artificial constraints on
> network size, so both CRPL and remote cache could safely be set to cover full
> unicast space (32768). In any practical application, the device running Linux +
> bluetooth will have more than enough RAM for this.

Under normal circumstances (a non-Provisioner node for example) There will only ever be the local device key available, and so the size of the remote cache is moot.

When being used as a Config Client, we are just trying to avoid reopening the file to fetch the device key to decode the expected response.  I was thinking of actually making it *smaller*, but there will be times (like during a key update) that multiple messages to multiple servers will be "in flight" simultaneously. However in most cases the configuration messages are rarely used outside of initial node setup.



> 
> Getting rid of these constraints would make the implementation
> substiantially simpler (no MRU, for example).


Increasing the Cache increases (in an open-ended way, depending on number of remote nodes) RAM usage for something that as stated above is rarely used.  All the device keys are always available...  they just may need to be read into RAM from storage.

> 
> FYI, we're running networks with couples of hundreds devices, and plan to
> expand them even further.
> 
> > +/* This is a thread-safe always malloced version of dirname which
> > +will work
> > + * regardless of which underlying dirname implimentation is used  */
> > +static char *alloc_dirname(const char *path) {
> I was under impression that mesh daemon operates inside a main loop, so it
> doesn't need to worry about thread safety?
> 
> > +bool storage_get_remote_dev_key(struct mesh_node *node, uint16_t
> unicast,
> > +							uint8_t dev_key[16])
> > +{
> > (...)
> > +	dir_name = alloc_dirname(cfgname);
> I don't see why the remote key storage should be kept separately (and in
> binary files?) from the main node json. It makes the implementation much
> more complicated, as you need to worry about directories etc.
> 
> I believie that keeping remote keys in the same place, as a JSON object,
> would be much simpler.

Again, this creates an open-ended RAM requirement that depends on the existence of external resources....  What if there are only 10 remote nodes?  Do we still commit enough RAM to handle 32,767 remote nodes just in case?  This implementation leave the open-ended burden of storage out of RAM, and acknowledges that Config messages, while important, are not the regular traffic...  Things like generic On/Off and Level messages should make up 99% of normal traffic, and those are sent with normal App Keys.


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

Re: [PATCH BlueZ] mesh: Add remote dev key support and cleanup

[Michal Lowas-Rzechonek]

Hi Brian,

On 04/24, Gix, Brian wrote:
> > From: Michał Lowas-Rzechonek [mailto:michal.lowas-rzechonek@silvair.com]
> > > +try_remote:
> > > +	/* Try remote device key if available */
> > > +	dev_key = node_get_remote_device_key(node, src);
> > > +	if (!dev_key)
> > > +		return -1;
> > > +
> > > +	if (mesh_crypto_payload_decrypt(NULL, 0, data, size, szmict, src,
> > > +					dst, key_id, seq, iv_idx, out,
> > dev_key))
> > > +		return APP_IDX_DEV_RMT;
> > > +
> > What's the purpose of APP_IDX_DEV_RMT?
> 
> App Index is an internal way to track which credentials were used to
> decrypt a message.  In theory, if the incoming message used the local
> device key, then the message is addressed to one of the local servers,
> and if the incoming message was decrypted by a *remote* device key it
> should be addressed to a local Client (as a response from a remote
> server).
>
> To enable that, we need to differentiate between the two possible
> device keys.
>
> However, also importantly, if a message that requires a response is
> received on *any* key (local dev, remote dev, or App Key) the response
> is supposed to be sent with the same key.
Right, but I think this is covered by the server/client model behaviour,
so I don't think we need to care if the message was encrypted using a
known remote device key, or our local device key.

This is indeed not explicitly mentioned in the spec, but I think the
consensus is that a server model shall always send responses using a
local device key.

I think this means that the API needs to allow using either local or
remote device key when *sending* a message (i.e. use remote key for
requests and local key for responses), but API for receiving messages
needs only to differentiate between device and application keys.

> When *sending* Dev Key message
> from the Application, we will try the remote nodes device key first,
> and if it does not exist we fall back to the local key.
I'm not sure the key selection should happen automaticall. I'd rather
have an explicit option to use local device key or remote one.

> In any case, these "Magic Numbers" are internal use only, and do not
> show up on the external interface.
Well, at the moment they are visible in Send() call: key_index argument
accepts 0x7fff as a way to indicate that local device key should be
used.

I guess you're refering to the updated doc/mesh-api.txt, so the plain Send()
method will accept only *real* app key indexes?

> > >  #define DEFAULT_CRPL 10
> > > +#define REMOTE_CACHE_LEN 5
> > By the way: why the limits here are set so low (and there doesn't seem to be
> > a way to change them...)?
> When being used as a Config Client, we are just trying to avoid
> reopening the file to fetch the device key to decode the expected
> response.
> (...)
> All the device keys are always available...  they
> just may need to be read into RAM from storage.
Ok, got it.

I was more concerned about the CRPL size. At the moment the limit means
that a node cannot simultaneously talk to more than 10 remote nodes
because of this (mesh/appkey.c:227):

	/* Replay Cache is fixed sized */
	if (l_queue_length(key->replay_cache) >= crpl) {
		int ret = l_queue_foreach_remove(key->replay_cache,
				clean_old_iv_index, L_UINT_TO_PTR(iv_index));

		if (!ret)
			return true;
	}

> > I don't see why the remote key storage should be kept separately (and in
> > binary files?) from the main node json. It makes the implementation much
> > more complicated, as you need to worry about directories etc.
> Again, this creates an open-ended RAM requirement that depends on the
> existence of external resources...
Not necessarily.

I understand that in the current implementation, the storage JSON is
kept in memory as a whole (mesh_node.jconfig), so adding stuff there
indeed increases the memory footprint of a node, but it doesn't need to
be like that.

What are your thoughts on replacing the JSON storage with something
more granular? The main bluetoothd seems to use a directory structure
and INI files, but a key-value store like gdbm might also be an option,
especially for frequently changing values like the sequence number.
A database would allow us to avoid explicit caching and rely on mmap().

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

Re: [PATCH BlueZ] mesh: Add remote dev key support and cleanup

[Michal Lowas-Rzechonek]

Hi Brian,

On 05/02, Gix, Brian wrote:
> > Right, but I think this is covered by the server/client model behaviour, so I don't
> > think we need to care if the message was encrypted using a known remote
> > device key, or our local device key.
> 
> There is the SHALL paragraph in the specification, under section
> 3.7.4.3:
> 
> "A response message shall always use the same DevKey or AppKey used by
> the corresponding request message."
Indeed, but on the other hand 4.4.1.1 says that:

"The application-layer security on the Configuration Server model shall use the
device key established during provisioning."

So it doesn't make any sense for a server model to accept messages encrypted
with remote device key. I think the only situation where a message using remote
device key should be accepted is processing responses directed to a local
client.

In the end, I agree we need to track the 'kind' of a device key... But, I think we
actually need to expose the local/remote distinction to the application, as an
(boolean?) argument of DevKeyMessageReceived. This is because foundation models
aren't the only models that are allowed to use device-level security.  Also,
you mentioned earlier that the daemon will not expose APIs for all
configuration client operations.

> > I think this means that the API needs to allow using either local or remote
> > device key when *sending* a message (i.e. use remote key for requests and
> > local key for responses), but API for receiving messages needs only to
> > differentiate between device and application keys.
And what about this then?

Because of the above, I don't think it's correct to always (automatically) use
remote device key if one is known.

If the previous comment about an additional argument makes sense, it would also
make sense to add it to DevKeySend.

> > I was more concerned about the CRPL size. At the moment the limit means that
> > a node cannot simultaneously talk to more than 10 remote nodes because of
> > this (mesh/appkey.c:227):
> 
> This will definitely be fixed, and should be following the value
> indicated by the Composition data of each node.
Note that the Composition Data Page 0 indicates a *minimum* CRPL size. A
node is free to use a longer list.

So if I read this right, the plan is to allow the application to declare CRPL
size during CreateNetwork/Join procedure?

> > What are your thoughts on replacing the JSON storage with something more
> > granular?
> >
> We would like to keep Node Storage, at least for the full Linux
> implementation, in JSON format
Got it. Thanks for the additional details.

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