| Message ID | 1585740006-9569-1-git-send-email-anupam.r@samsung.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Brian Gix |
| Headers | show |
| Series | [BlueZ] mesh: Add check for valid netkey index | expand |
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. > l_debug("(%x) %p", app_idx, key); > l_debug("net_idx %x", net_idx); >
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); >>
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@silvair.com> 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); > > >
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@silvair.com> > 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); >> > >
On Thu, 2020-04-02 at 21:30 +0530, Anupam Roy wrote: > Hi Brian, > > > 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@silvair.com> > > 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 If the App uses DevKeySend() with remote == true, but the node does not have the device key for that remote node in it's keyring, the method will silently fail, and no message will be sent Over-the-Air. If the App wants to *respond* to an incoming command received with the local nodes device key, it should respond using DevKeySend() with remote==false, and net_index == the net_index from the cooresponding DevKeyMessageReceived().
Hi Brian, >On Thu, 2020-04-02 at 21:30 +0530, Anupam Roy wrote: >> Hi Brian, >> >> > 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@silvair.com> >> > 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 > >If the App uses DevKeySend() with remote == true, but the node does not have the device key for that remote >node in it's keyring, the method will silently fail, and no message will be sent Over-the-Air. > Okay, I got that, deleting the keyring (by subnet-delete <net idx>) should not have any effect for remote DevKeySend messages, as net idx would be used just to pick up Network Encryption Key from the node->net->subnet. Thanks for clarification. >If the App wants to *respond* to an incoming command received with the local nodes device key, it should >respond using DevKeySend() with remote==false, and net_index == the net_index from the cooresponding >DevKeyMessageReceived().
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; + } } + if (!keyring_get_net_key(node, net_idx, &net_key)) { + l_debug("no net key for (%x)", net_idx); + return false; + } l_debug("(%x) %p", app_idx, key); l_debug("net_idx %x", net_idx);