The dataP pointer accesses an invalid address.

[Lkerenl-h]

lwm2m_dm_create() in core/management.c

...
    if (format != LWM2M_CONTENT_TLV
     && (size > 1 || dataP[0].type != LWM2M_TYPE_OBJECT_INSTANCE))
    {
        format = LWM2M_CONTENT_TLV;
...

When the lwm2mserver of the example is used, if you enter create 0 /123 {}. segmentation fault

[mlasch]

I am able to reproduce the segmentation fault with your create command on the server example. The example server calls lwm2m_dm_create() with dataP set to NULL pointer (and size to -1) when it is unable to parse the input data like {}.

dataP[0].type != LWM2M_TYPE_OBJECT_INSTANCE then causes the segmentation fault.

My suggestions:

1. dataP should be checked before being accessed and if set to NULL the function should return COAP_400_BAD_REQUEST. (same for size)
2. prv_create_client() in lwm2mserver.c could also be adjusted to check for parsing errors earlier.

[qleisan]

I prefer 2) it is the parsing of command line input that fails (ie "syntax error") not a coap message error.

[qleisan]

I might need to revise my comment above. "{}" is legal when creating a new object? (not providing all/any resource information)

[sbernard31]

"{}" is legal when creating a new object?

We are talking about SENML JSON OR OLD JSON content format right used for CREATE request?

Not so easy question.
Mainly because create request target an object (not an object instance) and so if there is no record we don't know the object instance ID to use for the new instance.
You could say that you let device choose the ID. This is what we do in Leshan, but recently I discover that letting device choose ID with those formats was not allowed ( (see OpenMobileAlliance/OMA_LwM2M_for_Developers#422 (comment))), so I'm not sure Leshan implement the right behavior.

Currently I would say this is not a valid payload and maybe Leshan should not allow this.

@qleisan, @sbertin-telular what do you think ?

[sbertin-telular]

The parsing of "{}" would be done in the server's prv_create_client function. This is independent of the format used for the CREATE request to the client. I don't think this function is even aware of what format will be used for the client communication. It can be SENML JSON, OLD JSON, or a simple integer for object ID 31024. The instance ID may be in the URI part of the command.

Hypothetically, a create with no resources could be valid if all resources are optional. I'm not sure what the usefulness of such an object would be, but we should probably allow the CREATE to go to the client if we can. At the least it could be useful for checking that the client would reject it when there is a single required resource.

[sbernard31]

This is independent of the format used for the CREATE request to the client.

You mean this is a dedicated format used for example command line ? (I guess it looks like OLD JSON or something like this?)

The instance ID may be in the URI part of the command.

I understand this is not allowed see (transport§6.4.4-Device-Management-and-Service-Enablement-Interface)

Operation	CoAP Method and Content Formats	Path	Success	Failure
Create	POST Content Format: SenML CBOR, SenML JSON, or TLV (see [LwM2M-CORE])	/{Object ID}	2.01 Created	4.00 Bad Request, 4.01 Unauthorized, 4.04 Not Found, 4.05 Method Not Allowed, 4.06 Not Acceptable

Unless again you talk about the URI of the command line ?

Hypothetically, a create with no resources could be valid if all resources are optional

Only if format allow it and I'm not sure this is allowed for SENML JSON or OLD JSON for reason I exposed above but I could be wrong.

[sbertin-telular]

The parsing of "{}" would be done in the server's prv_create_client function. This is independent of the format used for the CREATE request to the client. I don't think this function is even aware of what format will be used for the client communication. It can be SENML JSON, OLD JSON, or a simple integer for object ID 31024. The instance ID may be in the URI part of the command.

This was all about parsing of the command line.

Hypothetically, a create with no resources could be valid if all resources are optional

Only if format allow it and I'm not sure this is allowed for SENML JSON or OLD JSON for reason I exposed above but I could be wrong.

I haven't checked if any of the formats allow it.

[sbernard31]

If I correctly understand there is an issue with server example command line parsing.

But is there also an issue at client side where an invalid request could make crash the lwm2m client/device ?

[sbertin-telular]

The lwm2m_dm_create() function only exists in the server. This is not an issue on the client.

[sbernard31]

@Lkerenl-h If I'm not wrong you opened an security issue saying that a DoS attack could be made using this issue.
Could you explain this more ? because if this is just a server example command line parsing issue I can not see how this is possible ?

[Lkerenl-h]

@sbernard31 Yes, this problem can only be triggered in the sample server command line.

[Lkerenl-h]

But I'm not sure if there is a similar problem elsewhere, which will pass the wrong dataP to the lwm2m_dm_create function for parsing.
My suggestion is to add a conditional judgment to a similar function, and we cannot assume that all user inputs are trustworthy.

[sbernard31]

My suggestion is to add a conditional judgment to a similar function, and we cannot assume that all user inputs are trustworthy.

Unless I missed something, I think you're right and we should :

1. check inputs to avoid this kind of seg fault.
2. AND/OR clearly defined function contract (e.g. allowed value for function arguments)

Sometime 2) is enough but It could be less bug resilient and is not adapted when inputs come from "uncontrolled" source (like a foreign peer or a user)

[sbertin-telular]

I agree, we should check the inputs. That probably applies for any lwm2m_* function. Those are the ones intended to be called from outside code.

[sbertin-telular]

But I'm not sure if there is a similar problem elsewhere, which will pass the wrong dataP to the lwm2m_dm_create function for parsing.

I just grepped the code and there is exactly one call to lwm2m_dm_create(). That is in the example server. Whether other server implementations exist using Wakaama that could cause the same problem, I can't say.

[qleisan]

The "naive" solution to the problem (segmentation fault when executing server command create 0 /123 {}

  if (format != LWM2M_CONTENT_TLV
   && (size > 1 || dataP[0].type != LWM2M_TYPE_OBJECT_INSTANCE))
  {
      format = LWM2M_CONTENT_TLV;
  }

is replaced by

  if (format != LWM2M_CONTENT_TLV && size > 0){
      // dataP[0] is now safe to access
      if (size > 1 || dataP[0].type != LWM2M_TYPE_OBJECT_INSTANCE)
      {
          format = LWM2M_CONTENT_TLV;
      }
  }

however I am far from convinced that this solution is the best/correct one and the problem grows a lot when taking the more holistic view...

[sbertin-telular]

A little too naïve. That might handle this specific case, but we would need to check all the pointers passed in (except callback and userData) for NULL at the top of the function and return COAP_400_BAD_REQUEST if any are NULL.

I'm having second thoughts about checking all the inputs to lwm2m_* functions. It could increase the code size with little benefit over a clearly defined function contract. Code size is important if it is to be used on small embedded devices.

[sbernard31]

I'm having second thoughts about checking all the inputs to lwm2m_* functions. It could increase the code size with little benefit over a clearly defined function contract. Code size is important if it is to be used on small embedded devices.

Just an Idea but maybe we could add a #ifdef which allow to keep or remove this inputs checks ?

[qleisan]

size = lwm2m_data_parse(&uri, ... , &dataP)
This function parses SENML/JSON/... and returns a list of length size of tree data structures lwm2m_data_t

• size > 0 indicate successful parsing e.g. create 0 /123 [{"bn":"/123/","n":"6/2","v":69}]
• size <= indicate unsuccessful parsing or contradicting URI information (including instance) e.g.
  create 0 /123 [{"bn":"/124/","n":"6/2","v":69}] or
  create 0 /123/7 [{"bn":"/123/","n":"6/2","v":69}]. Other checks are also made.
• the tree (or list of trees) returned does not duplicate the &uri information e.g.
  create 0 /123 [{"bn":"/123/","n":"6/2","v":69}] creates a tree of depth 2 with a LWM2M_TYPE_OBJECT_INSTANCE node and a LWM2M_TYPE_UNSIGNED_INTEGER child node while
  create 0 /123/6 [{"bn":"/123/","n":"6/2","v":69}] only creates a tree with a LWM2M_TYPE_UNSIGNED_INTEGER node

prv_create_client(...) use lwm2m_data_parse() and special handling of object 31024 to attempt to build a tree.

I propose the code introduce a check here that returns an error if size < 0, this will however disqualify create 0 /123 {} as valid input, but it is not valid SENML/lwm2m JSON is it?

After this point the code checks if &uri contains instance information and if it does a new tree top node of type LWM2M_TYPE_OBJECT_INSTANCE is created with the list of trees as children.
Finally lwm2m_dm_create() is called.

[sbertin-telular]

I propose the code introduce a check here that returns an error if size < 0, this will however disqualify create 0 /123 {} as valid input, but it is not valid SENML/lwm2m JSON is it?

Right now several formats could be vaild. SENML JSON, old JSOM, or just an integer for object 31024. If the check is after all the possible parsing, I'm good with that. I just don't want to fail with the first format tried.

[qleisan]

@sbernard31 @sbertin-telular - the fix is merged, can we close this issue?

[sbernard31]

@Lkerenl-h this should be fixed in master. Could you check if it works for you ? 🙏
I close this issue but please reopen it if you face an issue.

@qleisan, @sbertin-telular do we need a more general issue about : checking inputs for any lwm2m_* function. (#514 (comment))