Wrong base topic if gateway_name is changed

[BadWolf42]

Describe the bug Using home/OpenMQTTGateway/commands/MQTTtoSYS/config topic : If mqtt_topic and gateway_name are changed at the same time, then only gateway_name is set, Resulting in base topic <gateway_name>/, instead of <mqtt_topic>/<gateway_name>/.

  To Reproduce Each time on a fresh install with base topic set to home/OpenMQTTGateway/:

Publish {"mqtt_topic":"bt/"} on home/OpenMQTTGateway/commands/MQTTtoSYS/config Base topic becomes bt/OpenMQTTGateway/ -> OK

Publish {"gateway_name":"OMG"} on home/OpenMQTTGateway/commands/MQTTtoSYS/config Base topic becomes home/OMG/ -> OK

Publish {"mqtt_topic":"bt/","gateway_name":"OMG"} on home/OpenMQTTGateway/commands/MQTTtoSYS/config Base topic becomes OMG/ -> KO

  Expected behavior When publishing {"mqtt_topic":"bt/","gateway_name":"OMG"} on home/OpenMQTTGateway/commands/MQTTtoSYS/config Base topic should becomes bt/OMG/

  Environment (please complete the following information): OpenMQTTGateway version used V0.9.13, esp32-lolin32lite-ble on ESP32

  Checks: Code responsible of this feature is really straightforward PR #1053: https://github.com/1technophile/OpenMQTTGateway/blob/v0.9.13/main/main.ino#L2070-L2080

I don't understand why only 1 parameter works at the time. Maybe stopProcessing() is required before saveMqttConfig() and startProcessing() after client.disconnect()...

[1technophile]

Thanks for pointing that out, any idea @h2zero ?

[BadWolf42]

I just re-tested with v0.9.14 to be sure. Actually I was wrong. I did not test in this order and can tel that each time gateway_name is set, base topic becomes <gateway_name>/.

So (Each time on a fresh install with base topic set to home/OpenMQTTGateway/):

1. Publish {"mqtt_topic":"bt/"} on home/OpenMQTTGateway/commands/MQTTtoSYS/config Base topic becomes bt/OpenMQTTGateway/ -> OK

Debug log:

N: [ MQTT->OMG ]: {"mqtt_topic":"bt/"}
{
  "mqtt_server": "192.168.11.3",
  "mqtt_port": "1883",
  "mqtt_user": "",
  "mqtt_pass": "",
  "mqtt_topic": "bt/",
  "gateway_name": "OpenMQTTGateway",
  "mqtt_broker_secure": false,
  "mqtt_broker_cert": "",
  "mqtt_ss_index": 0,
  "ota_server_cert": ""
}

2. Publish {"gateway_name":"OMG"} on home/OpenMQTTGateway/commands/MQTTtoSYS/config Base topic becomes OMG/ -> KO

Debug log:

N: [ MQTT->OMG ]: {"gateway_name":"OMG"}
{
  "mqtt_server": "192.168.11.3",
  "mqtt_port": "1883",
  "mqtt_user": "",
  "mqtt_pass": "",
  "mqtt_topic": "",
  "gateway_name": "OMG",
  "mqtt_broker_secure": false,
  "mqtt_broker_cert": "",
  "mqtt_ss_index": 0,
  "ota_server_cert": ""
}

3. Publish {"mqtt_topic":"bt/","gateway_name":"OMG"} on home/OpenMQTTGateway/commands/MQTTtoSYS/config Base topic becomes OMG/ -> KO

Debug log:

N: [ MQTT->OMG ]: {"mqtt_topic":"bt/","gateway_name":"OMG"}
{
  "mqtt_server": "192.168.11.3",
  "mqtt_port": "1883",
  "mqtt_user": "",
  "mqtt_pass": "",
  "mqtt_topic": "",
  "gateway_name": "OMG",
  "mqtt_broker_secure": false,
  "mqtt_broker_cert": "",
  "mqtt_ss_index": 0,
  "ota_server_cert": ""
}

[BadWolf42]

Owwww got it !

gateway_name char* is statically declared with a size of parameters_size here: https://github.com/1technophile/OpenMQTTGateway/blob/c04c62916fceb5dbb7d834b035d353af178d7492/main/main.ino#L171

But strncpy uses a max length of parameters_size * 2 to copy gateway_name here: https://github.com/1technophile/OpenMQTTGateway/blob/c04c62916fceb5dbb7d834b035d353af178d7492/main/main.ino#L2075

As gateway_name is declared next to mqtt_topic, I supposed that mqtt_topic was overwritten by strncpy (as it fills the end of dest char* with \0). So I tested with a 80 char long gateway_name and as I suspected this happened:

N: [ MQTT->OMG ]: {"gateway_name":"01234567890123456789012345678901234567890123456789012345678901234567890123456789","mqtt_topic":"bt/"}
{
  "mqtt_server": "192.168.11.3",
  "mqtt_port": "1883",
  "mqtt_user": "",
  "mqtt_pass": "",
  "mqtt_topic": "01234567890123456789",
  "gateway_name": "01234567890123456789012345678901234567890123456789012345678901234567890123456789",
  "mqtt_broker_secure": false,
  "mqtt_broker_cert": "",
  "mqtt_ss_index": 0,
  "ota_server_cert": ""
}W: MQTT connection...
N: Connected to broker
Stack smashing protect failure!
abort() was called at PC 0x40190827 on core 1
ELF file SHA256: 0000000000000000
Backtrace: 0x4008fac4:0x3ffcdd60 0x4008fd41:0x3ffcdd80 0x40190827:0x3ffcdda0 0x400d412a:0x3ffcddc0 0x400dac3d:0x3ffcded0 0x401068a4:0x3ffcdef0 0x40090d52:0x3ffcdf10
Rebooting...

I'm recompiling OMG without *2 to test.

BTW, if source char* is the same size as max len strncpy does not terminate the dest char* with \0, so all static allocation should be at least len+1 and last char forced to \0.

Bad

[h2zero]

Thanks for pointing that out, any idea @h2zero ?

Bad looks to have sorted it out 😄

[BadWolf42]

Hello Guys,

I had quite a struggle with PlatformIO... but anyway:

As I suspected, gw & topic change is working without the *2, but still some overflow with long names:

N: [ MQTT->OMG ]: {"gateway_name":"omg34567890123456789012345678901234567890123456789012345678901234567890123456789","mqtt_topic":"home/"}
{
  "mqtt_server": "192.168.11.3",
  "mqtt_port": "1883",
  "mqtt_user": "your_username",
  "mqtt_pass": "your_password",
  "mqtt_topic": "home/",
  "gateway_name": "omg345678901234567890123456789012345678901234567890123456789home/",
  "mqtt_broker_secure": false,
  "mqtt_broker_cert": "",
  "mqtt_ss_index": 0,
  "ota_server_cert": ""
}

There are a lot of unsafe strcpy/strcat/strcmp especially regarding mqtt_topic & gateway_name...

But I will suggest a PR to fix explicitly this issue first by:

• limiting mqtt_topic & gateway_name to parameters_size instead of mqtt_topic_max_size, and
• declaring mqtt_topic & gateway_name with a size of parameters_size + 1,
• increasing mqtt_topic_max_size from 100 to 150 for common platforms (ESP8266, ESP32, __AVR_ATmega2560__ & __AVR_ATmega1280__) and from 50 to 75 for others (even if max size for a MQTT topic is 65536 bytes). So that at least <max mqtt_topic><max gateway_name>/commands/MQTTtoSYS/config could fit in it.

With that even with a very large mqtt_topic & gateway_name, topic can still be redefined:

N: [ MQTT->OMG ]: {"gateway_name":"omg34567890123456789012345678901234567890123456789012345678901234567890123456789","mqtt_topic":"01234567890123456789012345678901234567890123456789012345678/"}
{
  "mqtt_server": "192.168.11.3",
  "mqtt_port": "1883",
  "mqtt_user": "your_username",
  "mqtt_pass": "your_password",
  "mqtt_topic": "01234567890123456789012345678901234567890123456789012345678/",
  "gateway_name": "omg345678901234567890123456789012345678901234567890123456789",
  "mqtt_broker_secure": false,
  "mqtt_broker_cert": "",
  "mqtt_ss_index": 0,
  "ota_server_cert": ""
}

And is correctly truncated:

N: [ MQTT->OMG ]: {"gateway_name":"omg345678901234567890123456789012345678901234567890123456789012345678901234567890123456789","mqtt_topic":"0123456789012345678901234567890123456789012345678901234567890123456789/"}
{
  "mqtt_server": "192.168.11.3",
  "mqtt_port": "1883",
  "mqtt_user": "your_username",
  "mqtt_pass": "your_password",
  "mqtt_topic": "012345678901234567890123456789012345678901234567890123456789",
  "gateway_name": "omg345678901234567890123456789012345678901234567890123456789",
  "mqtt_broker_secure": false,
  "mqtt_broker_cert": "",
  "mqtt_ss_index": 0,
  "ota_server_cert": ""
}

Are you ok with that, or should I address at the same time unsafe strcpy/strcat/strcmp ?

I would also suggest to reduce parameters_size, or (m)allocate it on the heap, or simply use C++ String.

Bad

[BadWolf42]

Hello,

PR #1242 has been merged and integrated in v0.9.15.

It handles almost all points:

• limiting mqtt_topic & gateway_name to parameters_size instead of mqtt_topic_max_size,
• declaring mqtt_topic & gateway_name with a size of parameters_size + 1,
• increasing mqtt_topic_max_size from 100 to 150 for common platforms and from 50 to 75 for others,
• rewrite cmpToMainTopic() function to be safer (and faster) when comparing C-strings.

But not:

• unsafe strcpy/strcat especially regarding mqtt_topic & gateway_name,
• handle better mqtt_topic & gateway_name by (m)allocating it on the heap, or using C++ String.

Those points are minor from my perspective. So, should we close this issue or do you want me to address them in a new PR?

Bad

[1technophile]

Hello,

@h2zero what are your thoughts here?

[github-actions[bot]]

This issue is stale because it has been open for 30 days with no activity.

[github-actions[bot]]

This issue was closed because it has been inactive for 14 days since being marked as stale.