Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Mime-Version: 1.0
Subject: RE: Re: [PATCH BlueZ] mesh: Add check for valid netkey index
Reply-To: anupam.r@samsung.com
From:   Anupam Roy <anupam.r@samsung.com>
To:     "Gix, Brian" <brian.gix@intel.com>
CC:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Semun Lee <semun.lee@samsung.com>,
        Dohyun Pyun <dh79.pyun@samsung.com>,
        Nitin Jhanwar <nitin.j@samsung.com>,
        AMIT KUMAR JAISWAL <amit.jaiswal@samsung.com>
In-Reply-To: <82b707704e1863c43748adce03057d2252fea59a.camel@intel.com>
Message-ID: <20200402140519epcms5p3608d451804bf03eefc34ce77b04ab0aa@epcms5p3>
Date:   Thu, 02 Apr 2020 19:35:19 +0530
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 105P
References: <82b707704e1863c43748adce03057d2252fea59a.camel@intel.com>
        <1585740006-9569-1-git-send-email-anupam.r@samsung.com>
        <CGME20200401112015epcas5p10545143f69c22449f559818476845cc5@epcms5p3>
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

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