Hi Brian,
>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.
>> >
I think, this check is not required as explained.
>> > > }
>> > >
>> > > + 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.
>
Okay, got it, thanks.
Since we plan to use the net key index, will the below sanity check stand valid(in case of app_idx == APP_IDX_DEV_REMOTE)?
As it may save some un-necesary processing of the message payload in case net key index is *Not* valid or *subnet* is deleted by Config Client. Please share your opinion. Thanks
>> 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);
>> > >
