eclipse-leshan / leshan

Java Library for LWM2M
https://www.eclipse.org/leshan/
BSD 3-Clause "New" or "Revised" License
647 stars 407 forks source link

Leshan client demo crash - LWM2M Security object 0, Server Private Key resource 4 #1316

Open davidahoward opened 1 year ago

davidahoward commented 1 year ago

The leshan client demo crashes if bootstrapped from 3rd party servers when using DTLS security with PSK. It is checking for, and requiring LWM2M Security object 0, Server Public Key resource 4, which although listed as mandatory is actually, NOT needed or required for Security mode PreShareKey or NoSecurity modes.

The transport spec. specifically says:

The "Server Public Key" Resource MUST NOT be used in the Pre-Shared Key mode.

https://www.openmobilealliance.org/release/LightweightM2M/V1_1_1-20190617-A/HTML-Version/OMA-TS-LightweightM2M_Transport-V1_1_1-20190617-A.html#5-2-8-1-0-5281-Pre-Shared-Keys

This is a leshan bug.

Also the leshan bootstrap demo is populating and sending a null resource 4, which it should not be doing.

sbernard31 commented 1 year ago

Which version of Leshan are you using ? I will try to reproduce this.

But I'm afraid that we still face this very very old issue https://github.com/OpenMobileAlliance/OMA_LwM2M_for_Developers/issues/77 that OMA never clearly revolves. (Or at least, I still don't understand how it should be implemented ...)

davidahoward commented 1 year ago

java -jar ./leshan-client-demo.jar --version leshan-client-demo v2.0.0-SNAPSHOT

Commit ID : b14a7406f5b12f98c577465605f7e6c20db3f36f Build Date: Wed Aug 31 09:23:10 UTC 2022 (1661937790014)

JVM: 11.0.16 (Ubuntu OpenJDK 64-Bit Server VM 11.0.16+8-post-Ubuntu-0ubuntu118.04) OS: Linux 5.4.0-1080-aws amd64

Code Source: https://github.com/eclipse/leshan/tree/b14a7406f5b12f98c577465605f7e6c20db3f36f

sbernard31 commented 1 year ago

OK I'm able to reproduce this. The reason is because Security class implements read like this :

case SEC_SERVER_PUBKEY: // server public key
    return ReadResponse.success(resourceid, serverPublicKey);

There is not null check because this is intended to be a mandatory resource. A way to implement it in a way where Server Public Keywill not be mandatory would :

case SEC_SERVER_PUBKEY: // server public key
  if (serverPublicKey != null)
      return ReadResponse.success(resourceid, serverPublicKey);
  else
      return ReadResponse.notFound();

Note this Security is just a very simple default implementation. And anyone who implement a LWM2M client with Leshan could easily change this default implementation. So at least there is an easy workaround.

But I would really like to get clarification from OMA before to change this.

Anyway, thanks you very much for reporting this :pray: ! (Hoping we will get answer from OMA)

mkgillmore commented 1 year ago

Is there a specific lwM2M developer issue created about this?

davidahoward commented 1 year ago

If this is considered normative text, it is clear:

https://www.openmobilealliance.org/release/LightweightM2M/V1_1_1-20190617-A/HTML-Version/OMA-TS-LightweightM2M_Transport-V1_1_1-20190617-A.html#5-2-8-1-0-5281-Pre-Shared-Keys

5.2.8.1. Pre-Shared Keys The "Server Public Key" Resource MUST NOT be used in the Pre-Shared Key mode.

sbernard31 commented 1 year ago

@davidahoward, maybe I'm not representative but for me it's not clear how this :

The "Server Public Key" Resource MUST NOT be used in the Pre-Shared Key mode.

works with the fact that this Server Public Key resource is mandatory.

Maybe this is an issue in the model, I reported it long time ago with any clear answer. Or maybe that means that the resource should we written with an empty byte array ? (what is currently implemented in Leshan) Or maybe something else ?

davidahoward commented 1 year ago

I've reported it again for resources 3,4,5 as they all are dependent what resource 2 (security mode) is. Hopefully, it will get corrected in normative text in such a way that optimizes the number of bytes over the air... After all, why send (or require) something that isn't going to be used? We shall see. It could also be fixed like Resource 10 - optional with must be in text linking to specific state of another resource...

jvermillard commented 1 year ago

my guess: they will not update because it will break the compatibility, so I guess the only reasonable choice is to keep what Leshan is doing: using an empty string

Warmek commented 1 year ago

The "Server Public Key" Resource MUST NOT be used in the Pre-Shared Key mode.

As I see it, the "mandatory" field seems to be referring to the client exposing those attributes to a LwM2M Server. It does not drive what the Bootstrap Server must send during the bootstrap. Also In the Server, the resource "Notification Storing When Disabled or Offline" is also marked as "mandatory", but the Leshan server does not send it during the bootstrap phase. I can't see why those fields wouldn't be implemented in the same way 🤔

sbernard31 commented 1 year ago

As I see it, the "mandatory" field seems to be referring to the client exposing those attributes to a LwM2M Server. It does not drive what the Bootstrap Server must send during the bootstrap.

Which part of the specification makes you think that ? Reading the LWM2M-v1.1.1@core§8.2.3. Operation on Object, you can read :

If the "Create" operation is permitted, the LwM2M Client MUST perform the instantiation of the Object
 only if all mandatory Resources are present in the "New Value" parameter  (see Section 6. Interfaces). 
If any of the mandatory Resources are not present, the LwM2M Client MUST send a "Bad Request" 
 error code to the LwM2M Server. 

There is nothing about Bootstrap Write Operation but FMPOV without more information I think this is reasonable to consider all creation/instantiation should behave like Create Operation. (See same question with Write with Replace Operation)

In the Server, the resource "Notification Storing When Disabled or Offline" is also marked as "mandatory", but the Leshan server does not send it during the bootstrap phase.

Reading the master code, I understand Leshan Bootstrap Server send this resource during bootstrap session. To be sure I verified with leshan-bsserver-demo on sandbox which confirms that. So I don't understand what makes you think that ?

I can't see why those fields wouldn't be implemented in the same way

I agree that behavior should be the same.