💬 Security & Signing

[Anticimex]

@damian interesting. It would appear that verified messages has part of the header overwritten. I will take a closer look tomorrow. If you want, please test with the latest release, although there have not to my knowledge been made any changes relating to this issue.
If you want, please also add some debug printing on the sensor value of the message buffer as it is received and goes though the process of verification and propagates out of the internals in the library. I will look in the code to see if I can find an explanation for the corruption.

[Anticimex]

@damian Can you please share your sketch including your debug statements? It appears to me that your node between the print "message.getBool: 1" and "message.sensor: 255" is sending something to the GW. That initiates security handshaking so "message" will not be the same between the first and second print.

[Damian]

@anticimex Sure, here it is (still under development so there are some minor, I hope, bugs)

// Enable debug prints to serial monitor
//#define MY_DEBUG

#define MY_DEBUG_VERBOSE_SIGNING
#define MY_SIGNING_SOFT
#define MY_SIGNING_SOFT_RANDOMSEED_PIN 7
#define MY_SIGNING_REQUEST_SIGNATURES


#define MY_RADIO_NRF24
#define MY_TRANSPORT_WAIT_READY_MS 5000                         
#define MY_NODE_ID 3                                            

#include <MySensors.h>
#include <Bounce2.h>
#include <DHT.h>

/*ZMIENIAMY TYLKO TU*/
#define R_CHILD_ID 0                                          
#define BUTTON_PIN A0                                         
#define RELAY_PIN 3                                           
#define NUMBER_OF_RELAYS 2
#define RELAY_ON 1                                            
#define RELAY_OFF 0                                           

#define T_CHILD_ID 10
#define H_CHILD_ID 11
#define DHT_PIN 8
#define SENSOR_TEMP_OFFSET 0

unsigned long waitDelay = 100;                                  
unsigned long connStatusCheckPeriod = 5*1000.;                   
unsigned long tempHumCheckPeriod = 300*1000.;
/*KONIEC ZMIAN*/



Bounce debouncer[NUMBER_OF_RELAYS];  

DHT dht;
bool metric = true;
float temp = 0.0;
float hum = 0.0;

MyMessage msgTemp(T_CHILD_ID, V_TEMP);
MyMessage msgHum(H_CHILD_ID, V_HUM);
MyMessage msg[NUMBER_OF_RELAYS];                               

bool firstRun = true;                                          
bool relayState[NUMBER_OF_RELAYS] = {false};                   
bool connectionState = false;                                 
bool controllerState [NUMBER_OF_RELAYS] = {false};                                 
bool wasOffline = true;                                         

unsigned long currentTime = 0;
unsigned long oldTime = 0;
unsigned long oldTimeSensor = 0;

unsigned long receiveTimeOld = 0;
unsigned long receiveTimeNew = 0;

bool buttonValue[NUMBER_OF_RELAYS] = {0};
bool oldButtonValue[NUMBER_OF_RELAYS] = {0};                                         


void before() {

}


void setup(){                                                    
  for (int i=0, r_pin=RELAY_PIN, b_pin=BUTTON_PIN; i<NUMBER_OF_RELAYS; i++, r_pin++, b_pin++) {
    pinMode(b_pin, INPUT_PULLUP);                            
    debouncer[i].attach(b_pin);                                  
    debouncer[i].interval(5);                                         

    pinMode(r_pin, OUTPUT);                                   

    digitalWrite(r_pin, RELAY_OFF);
  Serial.print("Setup b_pin:");
  Serial.println(b_pin);                             
  }

dht.setup(DHT_PIN);
}

void presentation() {
  sendSketchInfo("MultiRelay", "1.0");                                
  for (int i=0, r_id=R_CHILD_ID; i<NUMBER_OF_RELAYS; i++, r_id++) {
    present(r_id, S_BINARY);
    msg[i] = MyMessage(r_id, V_STATUS);                                   
  }

  present(H_CHILD_ID, S_HUM);
  present(T_CHILD_ID, S_TEMP);
  metric = getControllerConfig().isMetric;
}


void loop() {                                                     
  currentTime = millis();                                      
  if (firstRun) {                                                 
    firstRunFunc();                                              
  }

if (currentTime - oldTime >= connStatusCheckPeriod) {             
  oldTime = currentTime;
  connectionState = checkConnection();
  if (connectionState && wasOffline){
    for(int i=0;i<NUMBER_OF_RELAYS;i++) {
      send(msg[i].set(relayState[i]), false);                            
    }
  }
  Serial.print("Temp / Hum: ");
  Serial.print(temp);
  Serial.print(" / ");
  Serial.println(hum);
}

if (currentTime - oldTimeSensor >= tempHumCheckPeriod) {             
  oldTimeSensor = currentTime;
  dht.readSensor(true);
  temp = dht.getTemperature();
  temp += SENSOR_TEMP_OFFSET;
  hum = dht.getHumidity();
  send(msgTemp.set(temp, 1));
  send(msgHum.set(hum, 1));
}

  for(int i=0;i<NUMBER_OF_RELAYS;i++) {
      debouncer[i].update();
      buttonValue[i] = debouncer[i].read();
      
    if (buttonValue[i] != oldButtonValue[i] && buttonValue[i]==0) {
      relayState[i] = !relayState[i];
      
      switch (connectionState) {
        case 0:
        workOffline();
        break;
        case 1:
        workOnline();
        break;
      }
    }
    oldButtonValue[i] = buttonValue[i];                                   
  }
}

void receive(const MyMessage &message) {
  if (message.type==V_STATUS) {                                     
      if (firstRun) {                                               
        relayState[message.sensor] = message.getBool();
        } else {                                                    
        controllerState[message.sensor] =  message.getBool();                       
        Serial.print("\nRelay State: ");
        Serial.println(relayState[message.sensor]);
        Serial.print("Switching to: ");
        Serial.println(controllerState[message.sensor]);
        if (controllerState[message.sensor] != relayState[message.sensor]) {                        
          relayState[message.sensor] = controllerState[message.sensor];                             
          wait(waitDelay);                                          
          send(msg[message.sensor].set(relayState[message.sensor]), false);                         
          }
        Serial.print("Relay state after if: ");
        Serial.println(relayState[message.sensor]);
        digitalWrite(RELAY_PIN+message.sensor, relayState[message.sensor] ? RELAY_ON : RELAY_OFF); 
        }
    }
}

void firstRunFunc() {
    
    connectionState = checkConnection();                          
    wait(500);
    for (int i=0, r_pin=RELAY_PIN, r_id=R_CHILD_ID ;i<NUMBER_OF_RELAYS;i++, r_pin++, r_id++){
      switch (connectionState) {
        case 0:                                                     
          relayState[i] = false;                                      
          Serial.print("First Run: Offline: ");
          Serial.println(relayState[i]);
          digitalWrite(r_pin, relayState[i] ? RELAY_ON : RELAY_OFF);
          break;
        case 1:                                                    
          request(r_id, V_STATUS);                          
          wait(waitDelay);
          Serial.print("First Run: Online: ");
          Serial.println(relayState[i]);
          digitalWrite(r_pin, relayState[i] ? RELAY_ON : RELAY_OFF); 
          send(msg[i].set(relayState[i]), false);                         
          break;
      }
    }
    firstRun = !firstRun;                                         
  }

void workOffline() {
  wasOffline = true;
  for (int i=0, r_pin=RELAY_PIN;i<NUMBER_OF_RELAYS;i++, r_pin++){
    Serial.print("Pracujemy offline: ");
    Serial.println(relayState[i]);
    digitalWrite(r_pin, relayState[i] ? RELAY_ON : RELAY_OFF);
  }
}


void workOnline() {
  wasOffline = false;
  for (int i=0, r_pin=RELAY_PIN;i<NUMBER_OF_RELAYS;i++, r_pin++){
    Serial.print("Pracujemy online. Stan przekaznika ktory wysylamy do HA: ");
    Serial.println(relayState[i]);
    digitalWrite(r_pin, relayState[i] ? RELAY_ON : RELAY_OFF);
    wait(waitDelay);
    send(msg[i].set(relayState[i]), false);
    Serial.println("");
  }
}


void receiveTime(unsigned long ts) {
  if (firstRun) {
   receiveTimeNew = ts;
   Serial.print("Receive time first run old / new: ");
   Serial.print(receiveTimeOld);
   Serial.print(" / ");
   Serial.println(receiveTimeNew);
  } else {
    Serial.print("Received time:");
    Serial.println(ts);
    receiveTimeNew = ts;
  }
}

bool checkConnection() {
    bool rt = requestTime(false);
    wait(100);
    if ((receiveTimeNew != receiveTimeOld)) {
      Serial.print("Online, odebrany czas: ");
      Serial.println(receiveTimeNew);
      receiveTimeOld = receiveTimeNew;
      return true;
    }
  Serial.print("Offline, received time: ");
  Serial.println(receiveTimeNew);
  return false;
}

[Anticimex]

@damian Hm, I don't find your debug prints you used earlier.
But in general, I would say that the message object might be overwritten by the library, so if you need to use parts of it, make a copy of the parts you use first and then reference the copy to make sure a new incoming message does not overwrite it.
Eg, in receive():

void receive(const MyMessage &message) {
  if (message.type==V_STATUS) {                        
      bool value = message.getBool();
      uint8_t sensor = message.sensor;
      if (firstRun) {                                               
        relayState[sensor] = value;
        } else {                                                    
        controllerState[sensor] =  value;                       
        Serial.print("\nRelay State: ");
        Serial.println(relayState[sensor]);
        Serial.print("Switching to: ");
        Serial.println(controllerState[sensor]);
        if (controllerState[sensor] != relayState[sensor]) {                        
          relayState[sensor] = controllerState[sensor];                             
          wait(waitDelay);                                          
          send(msg[sensor].set(relayState[sensor]), false);                         
          }
        Serial.print("Relay state after if: ");
        Serial.println(relayState[sensor]);
        digitalWrite(RELAY_PIN+sensor, relayState[sensor] ? RELAY_ON : RELAY_OFF); 
        }
    }
}

And I believe the cause for your problem is that you do

send(msg[message.sensor].set(relayState[message.sensor]), false);  

after

Serial.print("Switching to: ");
        Serial.println(controllerState[message.sensor]);

but before

Serial.print("Relay state after if: ");
        Serial.println(relayState[message.sensor]);

so message might be changed between since the send() will request a nonce from the GW which then overwrites the buffer referenced by message.

[Damian]

@anticimex Thank you, I'll test it and let you know if it helped. PS. debug prints sent before was created in cito, this is my original script. But you find it well, it was instead of:

Serial.print("\nRelay State: ");
Serial.println(relayState[sensor]);

and

Serial.print("Relay state after if: ");
Serial.println(relayState[message.sensor]);

So basically the sections that you've mentioned as a possibly buggy.

[Damian]

@anticimex Yep, remodelling receive() function by adding variables and assigning values respectively message.sensor and message.getBool at the beginning got the job done. Now it works like a charm. Moreover, the whitelisting feature also works now - I had to do something wrong previously. Thank you once again for your help.

[Anticimex]

@damian great to hear! Happy secure home automating :)

[alowhum]

This page is not up to date, and this is causing some confusion:
https://github.com/tsathishkumar/MySController-rs/issues/15

[Anticimex]

@alowhum but that is not part of the core library, right? So this thread is perhaps not the best place for discussing 3rd party tools.

[alowhum]

"This thread contains comments for the article "Security & Signing" posted on MySensors.org."

@Anticimex This thread is the thread attached to a page on MySensors.org about security.

I pointed to that (awesome) tool on Github as an example that the page has caused some confusion about whether or not MySensors supports encryption.

[Anticimex]

@alowhum ok, thanks. Well we do. So please notify that author.

[Anticimex]

@alowhum as for the link back to mysensors documentation, that documentation is about message signing which is not to be confused with encryption. And on the top of the article is links to the latest documentation which should reflect the latest status on both signing and encryption.

[Anticimex]

@alowhum @hek we should perhaps retire that article or reduce it to a reference to the "actual" documentation instead to reduce the risk of confusion?

[hek]

Yes, maybe we could add a more prominent link in the article to the auto generated documentation. Right now the link easily missed in the ingress.... and it does not link directly to the overview documentation here:
https://www.mysensors.org/apidocs/group__MySigninggrpPub.html

I don't think we should scrap the page entirely as it contains the none API technical parts as a good overview.

[alowhum]

@hek said in 💬 Security & Signing:

https://www.mysensors.org/apidocs/group__MySigninggrpPub.html

That link still only refers to details about signing, and not encryption. So a user wanting to learn about security might still come away with the idea that only signing is supported.

[Anticimex]

@alowhum yes, it should link here: https://www.mysensors.org/apidocs/group__SecuritySettingGrpPub.html

[alowhum]

@anticimex Great!

[Yoshu]

Hi All not sure if this should be under gateway or security sections but just wondering If it is possible to run 2 seperate gateways for mysensors netwok

• a fully secured network with signing required both node and gateway for all nodes required to send and or receive signed data.
  -A second unsecured network for gerneral sensor data and equipment not requiring any security

[Anticimex]

@Yoshu it is possible. When you have two gateways, the networks will be completely isolated so you can run one of them secured while the other is not.
You might need to distinguish the nodes on in your controller by giving them static unique ID:s unless your controller ties the identifiers to each gateway or it might be difficult to determine which mysensors network a node belong to.

[lood29]

@Anticimex more important is to set different network id or you'll have lot of collisions and lost packets