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.
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);
>>
