[RFC BlueZ 0/1] Validate element indexation

[RFC BlueZ 0/1] Validate element indexation

[Jakub Witowski]

Hello,

I've prepared validation of element indexation.

First of all I've used 64-bit unsigned value to collect all given indexes.
As You can deduce from "4.2.1.1 Composition Data Page 0", the maximum value of elements can be 61.
It is limited by max message size which is 376. Furthermore the element indexes should be given
with no gap between them, for example:
  element index: 3, 2, 0, 1 will be ok,
  element index: 3, 2, 0 should return an error because the idx 1 is missing

Secondly I think, that the validation of element index value may be required, cause for now
we support 255 (uint8_t).

Please let me know what do You thing of aboves.

BR,
Jakub Witowski 

Jakub Witowski (1):
  mesh: Validate element indexation

 mesh/node.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

-- 
2.20.1

[RFC BlueZ 1/1] mesh: Validate element indexation

[Jakub Witowski]

This implementation checks if there is no gap between given element_indexes
---
 mesh/node.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/mesh/node.c b/mesh/node.c
index 1f781cfe9..35f85b0f2 100644
--- a/mesh/node.c
+++ b/mesh/node.c
@@ -1439,6 +1439,14 @@ static bool add_local_node(struct mesh_node *node, uint16_t unicast, bool kr,
 	return true;
 }
 
+static void collect_ele_idxs(void *a, void *b)
+{
+	const struct node_element *element = a;
+	uint64_t *ele_idx_list = (uint64_t *)b;
+
+	*ele_idx_list |= (1 << element->idx);
+}
+
 static void get_managed_objects_cb(struct l_dbus_message *msg, void *user_data)
 {
 	struct l_dbus_message_iter objects, interfaces;
@@ -1449,6 +1457,7 @@ static void get_managed_objects_cb(struct l_dbus_message *msg, void *user_data)
 	bool have_app = false;
 	bool is_new;
 	uint8_t num_ele;
+	uint64_t ele_idx_list = 0;
 
 	is_new = (req->type != REQUEST_TYPE_ATTACH);
 
@@ -1533,6 +1542,12 @@ static void get_managed_objects_cb(struct l_dbus_message *msg, void *user_data)
 		goto fail;
 	}
 
+	/* Check for proper element indexation */
+	l_queue_foreach(node->elements, collect_ele_idxs, &ele_idx_list);
+
+	if (((ele_idx_list + 1) & ele_idx_list) != 0)
+		goto fail;
+
 	if (req->type == REQUEST_TYPE_ATTACH) {
 		node_ready_func_t cb = req->cb;
 
-- 
2.20.1

Re: [RFC BlueZ 0/1] Validate element indexation

[Stotland, Inga]

[-- Attachment #1: Type: text/plain, Size: 1391 bytes --]

Hi Jakub,

On Mon, 2019-07-08 at 16:13 +0200, Jakub Witowski wrote:
> Hello,
> 
> I've prepared validation of element indexation.
> 
> First of all I've used 64-bit unsigned value to collect all given
> indexes.
> As You can deduce from "4.2.1.1 Composition Data Page 0", the maximum
> value of elements can be 61.
> It is limited by max message size which is 376. Furthermore the
> element indexes should be given
> with no gap between them, for example:
>   element index: 3, 2, 0, 1 will be ok,
>   element index: 3, 2, 0 should return an error because the idx 1 is
> missing
> 
> Secondly I think, that the validation of element index value may be
> required, cause for now
> we support 255 (uint8_t).
> 
> Please let me know what do You thing of aboves.
> 
> BR,
> Jakub Witowski 
> 
> Jakub Witowski (1):
>   mesh: Validate element indexation
> 
>  mesh/node.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 

I agree that the validation for the gaps is needed. Interesting point
about max number of elements...

I wonder if a better check woul be to we to add to construct
composition data as a validation point to make sure it fits in mesh message. Plus, an additional strict check can be done when Attach method is called: stored composition can be byte compared to the one dynamically generated from collected properties...




[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3265 bytes --]

Re: [RFC BlueZ 0/1] Validate element indexation

[Michał Lowas-Rzechonek]

Inga, Jakub,

On 07/08, Stotland, Inga wrote:
> I agree that the validation for the gaps is needed. Interesting point
> about max number of elements...
> 
> I wonder if a better check woul be to we to add to construct
> composition data as a validation point to make sure it fits in mesh
> message. Plus, an additional strict check can be done when Attach
> method is called: stored composition can be byte compared to the one
> dynamically generated from collected properties...

If I read that correctly, this means we would need a way to build
Composition Data on the fly, during get_manager_object_cb processing.

I think it would be possible to get rid of validate_model_property
function - instead, we could build a temporary mesh_node instance
using information provided by the application as-is, and then:

- in case of existing nodes, generate Composition Data from both
  existing and temporary instances, and byte-compare the two

- in case of new nodes, simply save the temporary instace to 'nodes'
  list

All of that assumes that Composition Data generationchecks that:
 - everything fits into a buffer (this is already done)
 - mandatory models are present
 - indexation is OK

I think this would make things slightly more consistent, and we would
get rid of most "is_new" checks during attach/join/create/import.

regard
-- 
Michał Lowas-Rzechonek <michal.lowas-rzechonek@silvair.com>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND

Re: [RFC BlueZ 0/1] Validate element indexation

[Stotland, Inga]

[-- Attachment #1: Type: text/plain, Size: 1695 bytes --]

Hi Michal,

On Mon, 2019-07-08 at 20:22 +0200, Michał Lowas-Rzechonek wrote:
> Inga, Jakub,
> 
> On 07/08, Stotland, Inga wrote:
> > I agree that the validation for the gaps is needed. Interesting
> > point
> > about max number of elements...
> > 
> > I wonder if a better check woul be to we to add to construct
> > composition data as a validation point to make sure it fits in mesh
> > message. Plus, an additional strict check can be done when Attach
> > method is called: stored composition can be byte compared to the
> > one
> > dynamically generated from collected properties...
> 
> If I read that correctly, this means we would need a way to build
> Composition Data on the fly, during get_manager_object_cb processing.
> 
> I think it would be possible to get rid of validate_model_property
> function - instead, we could build a temporary mesh_node instance
> using information provided by the application as-is, and then:
> 
> - in case of existing nodes, generate Composition Data from both
>   existing and temporary instances, and byte-compare the two
> 
> - in case of new nodes, simply save the temporary instace to 'nodes'
>   list
> 
> All of that assumes that Composition Data generationchecks that:
>  - everything fits into a buffer (this is already done)
>  - mandatory models are present
>  - indexation is OK
> 
> I think this would make things slightly more consistent, and we would
> get rid of most "is_new" checks during attach/join/create/import.
> 

This is exactly what I meant. Thanks for writing this up in a more
explanatory way.
This would be a comprehensive validation of the node's integrity.

Regards,
Inga


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3265 bytes --]