From 7648391f91f2b84e367ae2b38220b30936fb45b1 Mon Sep 17 00:00:00 2001 From: Eric Severance Date: Sun, 16 Feb 2025 05:15:30 -0800 Subject: [PATCH] Reject invalid configuration for the default MQTT server (#6066) * Sanity check configuration for the default MQTT server * Skip for MESHTASTIC_EXCLUDE_MQTT --------- Co-authored-by: Ben Meadors --- src/modules/AdminModule.cpp | 17 ++++++++++++++--- src/modules/AdminModule.h | 2 +- src/mqtt/MQTT.cpp | 27 +++++++++++++++++++++++++-- src/mqtt/MQTT.h | 2 ++ test/test_mqtt/MQTT.cpp | 36 ++++++++++++++++++++++++++++++++++++ 5 files changed, 78 insertions(+), 6 deletions(-) diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index 7906b410b..530d0b82e 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -162,7 +162,9 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta case meshtastic_AdminMessage_set_module_config_tag: LOG_INFO("Client set module config"); - handleSetModuleConfig(r->set_module_config); + if (!handleSetModuleConfig(r->set_module_config)) { + myReply = allocErrorResponse(meshtastic_Routing_Error_BAD_REQUEST, &mp); + } break; case meshtastic_AdminMessage_set_channel_tag: @@ -648,15 +650,23 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c) saveChanges(changes, requiresReboot); } -void AdminModule::handleSetModuleConfig(const meshtastic_ModuleConfig &c) +bool AdminModule::handleSetModuleConfig(const meshtastic_ModuleConfig &c) { if (!hasOpenEditTransaction) disableBluetooth(); switch (c.which_payload_variant) { case meshtastic_ModuleConfig_mqtt_tag: +#if MESHTASTIC_EXCLUDE_MQTT + LOG_WARN("Set module config: MESHTASTIC_EXCLUDE_MQTT is defined. Not setting MQTT config"); + return false; +#else LOG_INFO("Set module config: MQTT"); + if (!MQTT::isValidConfig(c.payload_variant.mqtt)) { + return false; + } moduleConfig.has_mqtt = true; moduleConfig.mqtt = c.payload_variant.mqtt; +#endif break; case meshtastic_ModuleConfig_serial_tag: LOG_INFO("Set module config: Serial"); @@ -724,6 +734,7 @@ void AdminModule::handleSetModuleConfig(const meshtastic_ModuleConfig &c) break; } saveChanges(SEGMENT_MODULECONFIG); + return true; } void AdminModule::handleSetChannel(const meshtastic_Channel &cc) @@ -1160,4 +1171,4 @@ void disableBluetooth() nrf52Bluetooth->shutdown(); #endif #endif -} +} \ No newline at end of file diff --git a/src/modules/AdminModule.h b/src/modules/AdminModule.h index ee2ebfd96..12c857e04 100644 --- a/src/modules/AdminModule.h +++ b/src/modules/AdminModule.h @@ -50,7 +50,7 @@ class AdminModule : public ProtobufModule, public Obser void handleSetOwner(const meshtastic_User &o); void handleSetChannel(const meshtastic_Channel &cc); void handleSetConfig(const meshtastic_Config &c); - void handleSetModuleConfig(const meshtastic_ModuleConfig &c); + bool handleSetModuleConfig(const meshtastic_ModuleConfig &c); void handleSetChannel(); void handleSetHamMode(const meshtastic_HamParameters &req); void handleStoreDeviceUIConfig(const meshtastic_DeviceUIConfig &uicfg); diff --git a/src/mqtt/MQTT.cpp b/src/mqtt/MQTT.cpp index 6043daa34..67eba82a6 100644 --- a/src/mqtt/MQTT.cpp +++ b/src/mqtt/MQTT.cpp @@ -41,6 +41,7 @@ MQTT *mqtt; namespace { constexpr int reconnectMax = 5; +constexpr uint16_t mqttPort = 1883; // FIXME - this size calculation is super sloppy, but it will go away once we dynamically alloc meshpackets static uint8_t bytes[meshtastic_MqttClientProxyMessage_size + 30]; // 12 for channel name and 16 for nodeid @@ -245,6 +246,11 @@ std::pair parseHostAndPort(String server, uint16_t port = 0) } return std::make_pair(std::move(server), port); } + +bool isDefaultServer(const String &host) +{ + return host.length() == 0 || host == default_mqtt_address; +} } // namespace void MQTT::mqttCallback(char *topic, byte *payload, unsigned int length) @@ -324,7 +330,7 @@ MQTT::MQTT() : concurrency::OSThread("mqtt"), mqttQueue(MAX_MQTT_QUEUE) } String host = parseHostAndPort(moduleConfig.mqtt.address).first; - isConfiguredForDefaultServer = host.length() == 0 || host == default_mqtt_address; + isConfiguredForDefaultServer = isDefaultServer(host); IPAddress ip; isMqttServerAddressPrivate = ip.fromString(host.c_str()) && isPrivateIpAddress(ip); @@ -408,7 +414,7 @@ void MQTT::reconnect() } #if HAS_NETWORKING // Defaults - int serverPort = 1883; + int serverPort = mqttPort; const char *serverAddr = default_mqtt_address; const char *mqttUsername = default_mqtt_username; const char *mqttPassword = default_mqtt_password; @@ -561,6 +567,23 @@ int32_t MQTT::runOnce() return 30000; } +bool MQTT::isValidConfig(const meshtastic_ModuleConfig_MQTTConfig &config) +{ + String host; + uint16_t port; + std::tie(host, port) = parseHostAndPort(config.address, mqttPort); + const bool defaultServer = isDefaultServer(host); + if (defaultServer && config.tls_enabled) { + LOG_ERROR("Invalid MQTT config: TLS was enabled, but the default server does not support TLS"); + return false; + } + if (defaultServer && port != mqttPort) { + LOG_ERROR("Invalid MQTT config: Unsupported port '%d' for the default MQTT server", port); + return false; + } + return true; +} + void MQTT::publishNodeInfo() { // TODO: NodeInfo broadcast over MQTT only (NODENUM_BROADCAST_NO_LORA) diff --git a/src/mqtt/MQTT.h b/src/mqtt/MQTT.h index 42157fda9..f7e3864f8 100644 --- a/src/mqtt/MQTT.h +++ b/src/mqtt/MQTT.h @@ -61,6 +61,8 @@ class MQTT : private concurrency::OSThread bool isUsingDefaultServer() { return isConfiguredForDefaultServer; } + static bool isValidConfig(const meshtastic_ModuleConfig_MQTTConfig &config); + protected: struct QueueEntry { std::string topic; diff --git a/test/test_mqtt/MQTT.cpp b/test/test_mqtt/MQTT.cpp index 3a4625aed..c00922548 100644 --- a/test/test_mqtt/MQTT.cpp +++ b/test/test_mqtt/MQTT.cpp @@ -800,6 +800,38 @@ void test_customMqttRoot(void) [] { return pubsub->subscriptions_.count("custom/2/e/test/+") && pubsub->subscriptions_.count("custom/2/e/PKI/+"); })); } +// Empty configuration is valid. +void test_configurationEmptyIsValid(void) +{ + meshtastic_ModuleConfig_MQTTConfig config; + + TEST_ASSERT_TRUE(MQTT::isValidConfig(config)); +} + +// Configuration with the default server is valid. +void test_configWithDefaultServer(void) +{ + meshtastic_ModuleConfig_MQTTConfig config = {.address = default_mqtt_address}; + + TEST_ASSERT_TRUE(MQTT::isValidConfig(config)); +} + +// Configuration with the default server and port 8888 is invalid. +void test_configWithDefaultServerAndInvalidPort(void) +{ + meshtastic_ModuleConfig_MQTTConfig config = {.address = default_mqtt_address ":8888"}; + + TEST_ASSERT_FALSE(MQTT::isValidConfig(config)); +} + +// Configuration with the default server and tls_enabled = true is invalid. +void test_configWithDefaultServerAndInvalidTLSEnabled(void) +{ + meshtastic_ModuleConfig_MQTTConfig config = {.tls_enabled = true}; + + TEST_ASSERT_FALSE(MQTT::isValidConfig(config)); +} + void setup() { initializeTestEnvironment(); @@ -843,6 +875,10 @@ void setup() RUN_TEST(test_enabled); RUN_TEST(test_disabled); RUN_TEST(test_customMqttRoot); + RUN_TEST(test_configurationEmptyIsValid); + RUN_TEST(test_configWithDefaultServer); + RUN_TEST(test_configWithDefaultServerAndInvalidPort); + RUN_TEST(test_configWithDefaultServerAndInvalidTLSEnabled); exit(UNITY_END()); } #else