[security] Introducing signing support to MySensors

[fleinze]

@Anticimex
I think I may have found a bug in the code:
I wanted to build a gateway that accepts both signed and unsigned messages. For this I used MySigningAtsha204Soft signer(false);.
Now the gateway did accept unsigned messages, but also all messages sent to the gateway by another node with signing where unsigned.
After changing this line in MySensor.cpp:

if (signer.requestSignatures() && DO_SIGN(msg.sender))

to

if (signer.requestSignatures() || DO_SIGN(msg.sender))

it worked as I wanted it. The gateway accepts unsigned messages from nodes but if a node expects signed messages, the messages to the gateway are also signed.
This line means that the gateway requests signing from the node if he always requests signing or if the node requests signing.
What do you think?

[Anticimex]

@fleinze I am not sure I understand the basic problem. What do you mean by " but also all messages sent to the gateway by another node with signing where unsigned.". The signing is based on requirement. A node (or gateway) either require messages sent to it to be signed or not. You have told your gateway that it is not supposed to require signed messages so it will accept unsigned messages sent to it. It is up to the node to configure if the node require signed messages in return. The gateway configuration has nothing to do with the preferences of a node. If you tell your gateway that it does not require signing, no messages to it will be signed. The opposite also holds true. If you configure it to require signed messages, all messages have to be signed. That is "working as designed".

[Anticimex]

@fleinze after reading the post a few times I think I understand better :) you want nodes who require signed messages from the gateway to send signed messages to the gateway as well even though the gateway is configured to not require it. I don't have a problem with that change as long as it is also portable to the nodes (the same code works equally well for both gateways and nodes usecases). You are welcome to put a pull request on the code so I can have a closer look. If approved I will update the head post to reflect this changed behaviour.

[fleinze]

:+1: The change only effects gateways. I will put a pull request, thx.

[Anticimex]

@fleinze do you have some special reason for why you want this? I don't see a real benefit and it would slightly increase the traffic. I suppose it would add some symmetry to node-gw communication.

[Anticimex]

@fleinze I am quite stressed out for the moment so I forget my own design :)
What you actually should do is to configure the gateway to require signed messages. It will then do so, but only from nodes that in turn require signed messages. So you should not need to change anything, just set the GW to require signatures.

[fleinze]

@Anticimex If I do so, this is the output of the Gateway:

0;0;3;0;9;gateway started, id=0, parent=0, distance=0
0;0;3;0;14;Gateway startup complete.
0;0;3;0;9;no sign
0;0;3;0;9;no sign
0;0;3;0;9;no sign
0;0;3;0;9;no sign
0;0;3;0;9;read: 2-2-0 s=0,c=0,t=6,pt=0,l=0,sg=0:
2;0;0;0;6;
0;0;3;0;9;read: 2-2-0 s=1,c=0,t=30,pt=0,l=0,sg=0:
2;1;0;0;30;
0;0;3;0;9;no sign
0;0;3;0;9;no sign
0;0;3;0;9;no sign

I tried to compile the node with deactivated signing feature and also with MySigningNone.
The messages get rejected because in MySensor.cpp line 570 (developement branch) it is not checked if the sender requires signing:

	if (signer.requestSignatures() && msg.destination == nc.nodeId && mGetLength(msg) && !mGetAck(msg) &&
		(mGetCommand(msg) != C_INTERNAL ||
		 (msg.type != I_GET_NONCE_RESPONSE && msg.type != I_GET_NONCE && msg.type != I_REQUEST_SIGNING &&
		  msg.type != I_ID_REQUEST && msg.type != I_ID_RESPONSE &&
		  msg.type != I_FIND_PARENT && msg.type != I_FIND_PARENT_RESPONSE)))

My thought is that if I have a mixed network (signing and non-signing nodes) for some sensors I do not need signing (i.e. temperature-sensors). But if I have a button sensor that can actually switch something on or off (via the controller), it would be a security benefit if the messages from the sensor to the gateway are signed.

[Anticimex]

@fleinze If you have upgraded the library version, the signing table might have shifted in EEPROM. You then need to run the clear EEPROM sketch to reset the stored state in order for the gw/nodes to re-learn the existing signing preferences of the network.

[Anticimex]

@fleinze said:

My thought is that if I have a mixed network (signing and non-signing nodes) for some sensors I do not need signing (i.e. temperature-sensors). But if I have a button sensor that can actually switch something on or off (via the controller), it would be a security benefit if the messages from the sensor to the gateway are signed.

This is the exact usecase for the gw default behavior to only require signing from nodes that require signing in return. But I also got "no sign" errors after I flashed development branch yesterday on node/gw and only after I wiped the GW EEPROM I got it back online. So please try that. If it still does not work, I have to look closer, and see why this has broken because it has been working like that when I submitted the signing behavior.

[fleinze]

@Anticimex I tried to make it work:

• took a fresh copy of the development branch
• activated signing in MyConfig.h
• flashed the ClearEepromConfig sketch to both the test-gateway and the test-node.
• built the gateway with MySigningAtsha204Soft signer; (requires signing by default)
• built the node with MySigningNone signer;

Still got the no sign message from the gateway...
Please have a look at this.

[Anticimex]

@fleinze mysigningnone is a special case. It won't make signatures, so could you instead try with either hard or soft atsha?

[Anticimex]

@fleinze also, you can mix hard and soft atsha signing in a network, but you can't mix with "none". If you mark a node that it can do signing or require signing and use the "none" backend, nodes that use atsha (hard or soft) will not accept the signatures.

[fleinze]

@Anticimex
Ok, I built the node with MySigningAtsha204Soft signer(false); so it does not require signing and the gateway with MySigningAtsha204Soft signer;
-> no sign error.
How do I correctly build a node that does not sign in a mixed network (signing and non-signing nodes mixed)?

[Anticimex]

@fleinze
In a network where you have "mixed" nodes, I would suggest something like this:
The gateway has signing enabled and is set to require signatures and uses either hard or soft ATSHA (since you earlier wrote that you wanted the GW to sign messages to nodes that signed messages in return).
For all nodes that need to be secure, enable signing and pick either hard or soft ATSHA (depending on the node hardware) and set them to require signatures.
For all nodes that do not need to be secure, just disable signing alltogether, or pick any signing backend and set it to NOT require signatures.
The result should be like this:
A "insecure" node sends and receives unsigned messages to/from GW.
A "secure" node sends and receives signed messages to/from GW.
The GW will send signed messages to all nodes that has reported to the GW that they require signatures.
The GW will send unsigned messages to all nodes that has reported to the GW that they do NOT require signatures.

[fleinze]

@Anticimex In your original post you said:

However, the difference is that the gateway will only require signed messages from nodes it knows in turn require signed messages.

I made pull request 208 to match this behavior in the code:
https://github.com/mysensors/Arduino/pull/208

• If node is not a gateway, everything is as always. (1 || x)
• if node is gateway and the sender requires signed messages, check signature (0 || 1)
• if node is gateway and the sender does not requires signed messages, do not check signature (0 || 0)
  please consider adding this.

[Anticimex]

@fleinze Ok, lets discuss this in the PR and conclude it in this thread later.

[Anticimex]

Ok, the problem is identified and @fleinze has provided a fix that resolves the issue which is now merged to the development branch.
Thanks for finding the issue and fixing it!
There is no change in the signing behavior as it is documented. Now gateway does as it is supposed to.

[mfalkvidd]

Great work @fleinze and @Anticimex !
Reading this thread (yes, all of it) has made me ready to start configuring my nodes to use signing. Better start now, while my sensor network is still small :)

[FotoFieber]

Compiling with the latest dev branch SecureActuator example

#define MY_SIGNING_REQUEST_SIGNATURES

I get following error:

In file included from /Users/mm/Documents/Arduino165/libraries/MySensors/MySensor.h:157:0,
                 from SecureActuator.ino:58:
/Users/mm/Documents/Arduino165/libraries/MySensors/core/MyTransport.cpp: In function 'void transportProcess()':
/Users/mm/Documents/Arduino165/libraries/MySensors/core/MyTransport.cpp:92:10: error: 'MY_IS_GATEWAY' was not declared in this scope
    if ((!MY_IS_GATEWAY || DO_SIGN(sender)) &&
          ^
/Users/mm/Documents/Arduino165/libraries/MySensors/core/MyTransport.cpp:93:20: error: 'nc' was not declared in this scope
     destination == nc.nodeId &&
                    ^
Fehler beim Kompilieren.```

Is this a problem of the code or of my setup?

[Anticimex]

@FotoFieber @hek something related to the recent refactoring? I have not had the opportunity to evaluate the effects on signing myself.