Hi Anupam,
On Thu, 2020-04-02 at 19:35 +0530, Anupam Roy wrote:
> Hi Brian,
>
> > Hi Anupam,
> > On Wed, 2020-04-01 at 16:50 +0530, Anupam Roy wrote:
> > > This patch adds validation of net key index, which will be
> > > used to send message to nodes. Return error in case net key index
> > > is not valid. This avoids message encryption using device key
> > > and further processing of the message.
> > > ---
> > > mesh/model.c | 9 +++++++++
> > > 1 file changed, 9 insertions(+)
> > >
> > > diff --git a/mesh/model.c b/mesh/model.c
> > > index 9455833..6cc1dc5 100644
> > > --- a/mesh/model.c
> > > +++ b/mesh/model.c
> > > @@ -546,6 +546,7 @@ static bool msg_send(struct mesh_node *node, bool credential, uint16_t src,
> > > uint8_t dev_key[16];
> > > uint32_t iv_index, seq_num;
> > > const uint8_t *key;
> > > + struct keyring_net_key net_key;
> > > uint8_t *out;
> > > uint8_t key_aid = APP_AID_DEV;
> > > bool szmic = false;
> > > @@ -578,8 +579,16 @@ static bool msg_send(struct mesh_node *node, bool credential, uint16_t src,
> > > }
> > >
> > > net_idx = appkey_net_idx(node_get_net(node), app_idx);
> > > + if (net_idx == NET_IDX_INVALID) {
> > > + l_debug("no net key for (%x)", net_idx);
> > > + return false;
> > > + }
> >
> > This *might* be a valid "sanity" test... Although an App Key should never be allowed to be added, or exist
> > unless the bound netkey already exists. I think the only way an App Key might exist without it's bound
> > netkey
> > is if the daemon private node.json file was hand edited, which is not technically allowed.
> >
> > > }
> > >
> > > + if (!keyring_get_net_key(node, net_idx, &net_key)) {
> > > + l_debug("no net key for (%x)", net_idx);
> > > + return false;
> > > + }
> >
> > This check is invalid. Only an authorized Config Clients / Provisioners have Key Rings, which is used to
> > send
> > AddKey, UpdateKey and Provisioning data to remote nodes. The keyring is restricted to nodes which have
> > full
> > network configuration privleges.... Adding this would require *all* nodes in the network to be privleged
> > nodes, which would then allow them to make changes outside the Provisioner/ Config Client control.
> >
> Thanks you for your input. The intention was to validate 'Net Key Index' for DevKeySend messages, sent by
> *Config client/Provisioner* app specifically.
> Actually, one observation I had is that, even though, Config client supplies the 'Net Key Index', & daemon
> passes it all the way to msg_send(), eventually, it seems, it is ignored & encryption key is picked up from
> the primary subnet. Also, I noticed that Config client seems to allow deleting a created subnet (subnet-
> delete <net key index>) at present, at any point of time.
You are correct in finding this bug... It was found in paralell by Przemysław Fierek, and should be fixed as
of this commit:
commit 84a9b6ce4b66a2ba21cce8e4b0c3c6e097a5493a
Author: Przemysław Fierek <przemyslaw.fierek@xxxxxxxxxxx>
Date: Tue Mar 31 14:09:08 2020 +0200
mesh: Add net key index to sar structure
This patch adds net key index to struct mesh_sar. This fixes problem with
using invalid network key to encrypt application messages.
If you check out the current tip, hopefully it will solve the problem you found where the incorrect (primary
subnet) key was used instead of the requested net key.
> I am not sure if *Not using* the 'Net Key Index' by the daemon is by design.
>
> Anyways, I got what you explained about *authorized apps having Keyring access*. In such case, I think, the
> keyring check would make sense only in the following condition. Please share your opinion. Thanks.
> } else if (app_idx == APP_IDX_DEV_REMOTE) {
> if (!keyring_get_remote_dev_key(node, dst, dev_key))
> return false;
>
> key = dev_key;
>
> /** Add Key Ring Check for valid Net Key Index **/
> }
>
> > > l_debug("(%x) %p", app_idx, key);
> > > l_debug("net_idx %x", net_idx);
> > >
