From 60a66a3dcdb0d9c94dc4c2e6f578af0103198c4d Mon Sep 17 00:00:00 2001 From: Wiebe Cazemier Date: Wed, 28 Feb 2024 10:16:21 +0100 Subject: [PATCH] Add 'zero_byte_username_is_anonymous' config option There are many clients out there who set the 'username' flag in connect and then set an empty username. The default is to not have two methods of signalling anonymous clients, but this can be overriden. https://github.com/halfgaar/FlashMQ/issues/91 --- FlashMQTests/tst_maintests.cpp | 29 +++++++++++++++++++++++++++++ bridgeconfig.cpp | 3 +++ bridgeconfig.h | 7 ++++--- client.cpp | 2 +- configfileparser.cpp | 6 ++++++ man/flashmq.1 | 2 +- man/flashmq.conf.5 | 13 ++++++++++++- man/flashmq.conf.5.dbk5 | 23 +++++++++++++++++++++++ man/flashmq.conf.5.html | 23 +++++++++++++++++++++++ mqttpacket.cpp | 32 ++++++++++++++++++++------------ settings.h | 1 + types.cpp | 8 ++++---- types.h | 5 +++-- 13 files changed, 130 insertions(+), 24 deletions(-) diff --git a/FlashMQTests/tst_maintests.cpp b/FlashMQTests/tst_maintests.cpp index 68294246..a71f2dd9 100644 --- a/FlashMQTests/tst_maintests.cpp +++ b/FlashMQTests/tst_maintests.cpp @@ -2347,6 +2347,20 @@ listen { ConnAckData ackData = ack.parseConnAckData(); QCOMPARE(ackData.reasonCode, ReasonCodes::NotAuthorized); } + + // Test empty password for existing user. It should not think it's anonymous. + { + FlashMQTestClient client; + client.start(); + client.connectClient(ProtocolVersion::Mqtt5, false, 120, [](Connect &connect) { + connect.username = "one"; + connect.password = ""; + }, 2883); // allow_anonymous true global setting. + + auto ack = client.receivedPackets.front(); + ConnAckData ackData = ack.parseConnAckData(); + QCOMPARE(ackData.reasonCode, ReasonCodes::NotAuthorized); + } } void MainTests::testOverrideAllowAnonymousToFalse() @@ -2360,6 +2374,7 @@ void MainTests::testOverrideAllowAnonymousToFalse() ConfFileTemp confFile; confFile.writeLine(formatString("mosquitto_password_file %s", passwd_file.getFilePath().c_str())); confFile.writeLine("allow_anonymous true"); + confFile.writeLine("zero_byte_username_is_anonymous true"); confFile.writeLine(R"( listen { protocol mqtt @@ -2427,6 +2442,20 @@ listen { QCOMPARE(ackData.reasonCode, ReasonCodes::Success); } + // Test 'zero_byte_username_is_anonymous true' + { + FlashMQTestClient client; + client.start(); + client.connectClient(ProtocolVersion::Mqtt311, false, 120, [](Connect &connect) { + connect.username = ""; + connect.password = "wrong"; + }, 2884); + + auto ack = client.receivedPackets.front(); + ConnAckData ackData = ack.parseConnAckData(); + QCOMPARE(ackData.reasonCode, ReasonCodes::Success); + } + { FlashMQTestClient client; client.start(); diff --git a/bridgeconfig.cpp b/bridgeconfig.cpp index e71485f2..50bbf8d6 100644 --- a/bridgeconfig.cpp +++ b/bridgeconfig.cpp @@ -171,6 +171,9 @@ void BridgeConfig::isValid() if (clientidPrefix.length() > 10) throw ConfigFileException("clientidPrefix can't be longer than 10"); + if (protocolVersion <= ProtocolVersion::Mqtt311 && remote_password.has_value() && !remote_username.has_value()) + throw ConfigFileException("MQTT 3.1.1 and lower require a username when you set a password."); + setClientId(); } diff --git a/bridgeconfig.h b/bridgeconfig.h index 0bc68897..dbb757c8 100644 --- a/bridgeconfig.h +++ b/bridgeconfig.h @@ -4,6 +4,7 @@ #include #include #include +#include #include "session.h" #include "dnsresolver.h" #include "sslctxmanager.h" @@ -40,9 +41,9 @@ class BridgeConfig ProtocolVersion protocolVersion = ProtocolVersion::Mqtt311; bool bridgeProtocolBit = true; std::string clientidPrefix = "fmqbridge"; - std::string local_username; - std::string remote_username; - std::string remote_password; + std::optional local_username; + std::optional remote_username; + std::optional remote_password; bool remoteCleanStart = true; uint32_t remoteSessionExpiryInterval = 0; bool localCleanStart = true; diff --git a/client.cpp b/client.cpp index 23930802..2b5bf820 100644 --- a/client.cpp +++ b/client.cpp @@ -687,7 +687,7 @@ void Client::setBridgeState(std::shared_ptr bridgeState) this->address = bridgeState->c.address; this->clean_start = bridgeState->c.localCleanStart; this->clientid = bridgeState->c.getClientid(); - this->username = bridgeState->c.local_username; + this->username = bridgeState->c.local_username.value_or(std::string()); this->keepalive = bridgeState->c.keepalive; // Not setting maxOutgoingTopicAliasValue, because that must remain 0 until the other side says (in the connack) we can uses aliases. diff --git a/configfileparser.cpp b/configfileparser.cpp index 8cf523aa..ee3eff2e 100644 --- a/configfileparser.cpp +++ b/configfileparser.cpp @@ -206,6 +206,7 @@ ConfigFileParser::ConfigFileParser(const std::string &path) : validKeys.insert("rebuild_subscription_tree_interval_seconds"); validKeys.insert("minimum_wildcard_subscription_depth"); validKeys.insert("wildcard_subscription_deny_mode"); + validKeys.insert("zero_byte_username_is_anonymous"); validListenKeys.insert("port"); validListenKeys.insert("protocol"); @@ -991,6 +992,11 @@ void ConfigFileParser::loadFile(bool test) else throw ConfigFileException(formatString("Value '%s' for '%s' is invalid.", value.c_str(), key.c_str())); } + + if (testKeyValidity(key, "zero_byte_username_is_anonymous", validKeys)) + { + tmpSettings.zeroByteUsernameIsAnonymous = stringTruthiness(value); + } } } catch (std::invalid_argument &ex) // catch for the stoi() diff --git a/man/flashmq.1 b/man/flashmq.1 index 435d664f..0423925a 100644 --- a/man/flashmq.1 +++ b/man/flashmq.1 @@ -5,7 +5,7 @@ \\$2 \(la\\$1\(ra\\$3 .. .if \n(.g .mso www.tmac -.TH flashmq 1 "16 November 2023" "" "" +.TH flashmq 1 "4 March 2024" "" "" .SH NAME flashmq \- A fast light-weight scalable MQTT server .SH SYNOPSIS diff --git a/man/flashmq.conf.5 b/man/flashmq.conf.5 index df149784..974a0e3d 100644 --- a/man/flashmq.conf.5 +++ b/man/flashmq.conf.5 @@ -5,7 +5,7 @@ \\$2 \(la\\$1\(ra\\$3 .. .if \n(.g .mso www.tmac -.TH flashmq.conf 5 "2 March 2024" "" "" +.TH flashmq.conf 5 "16 March 2024" "" "" .SH NAME flashmq.conf \- FlashMQ configuration file format .SH SYNOPSIS @@ -119,6 +119,17 @@ This option can be overriden on a per-listener basis; see .URL #listen__allow_anonymous listener.allow_anonymous \&. +Default value: \*(T +.TP +\*(T<\fBzero_byte_username_is_anonymous\fR\*(T> \fItrue/false\fR +The proper way to signal an anonymous client is by setting the 'username present' flag in the CONNECT packet to 0, which in MQTT3 also demands the absence of a password. However, there are also clients out there that set the 'username present' flag to 1 and then give an empty username. This is an undesirable situation, because it means there are two ways to identify an anonymous client. + +Anonymous clients are not authenticated against a loaded plugin when \*(T<\fBallow_anonymous\fR\*(T> is true. With this option enabled, that means users with empty string as usernames also aren't. + +With this option disabled, clients connecting with an empty username will be reject with 'bad username or password' as MQTT error code. + +The default is to be unambigious, but this can be overridden with this option. + Default value: \*(T .TP \*(T<\fBrlimit_nofile\fR\*(T> \fInumber\fR diff --git a/man/flashmq.conf.5.dbk5 b/man/flashmq.conf.5.dbk5 index fee51985..41698553 100644 --- a/man/flashmq.conf.5.dbk5 +++ b/man/flashmq.conf.5.dbk5 @@ -240,6 +240,29 @@ + + true/false + + + The proper way to signal an anonymous client is by setting the 'username present' flag in the CONNECT packet to 0, which in MQTT3 also demands the absence of a password. However, there are also clients out there that set the 'username present' flag to 1 and then give an empty username. This is an undesirable situation, because it means there are two ways to identify an anonymous client. + + + + Anonymous clients are not authenticated against a loaded plugin when is true. With this option enabled, that means users with empty string as usernames also aren't. + + + + With this option disabled, clients connecting with an empty username will be reject with 'bad username or password' as MQTT error code. + + + + The default is to be unambigious, but this can be overridden with this option. + + + Default value: false + + + number diff --git a/man/flashmq.conf.5.html b/man/flashmq.conf.5.html index 9d49e8ae..81cb5797 100644 --- a/man/flashmq.conf.5.html +++ b/man/flashmq.conf.5.html @@ -296,6 +296,29 @@ +
zero_byte_username_is_anonymous true/false#
+
+

+ The proper way to signal an anonymous client is by setting the 'username present' flag in the CONNECT packet to 0, which in MQTT3 also demands the absence of a password. However, there are also clients out there that set the 'username present' flag to 1 and then give an empty username. This is an undesirable situation, because it means there are two ways to identify an anonymous client. +

+ +

+ Anonymous clients are not authenticated against a loaded plugin when allow_anonymous is true. With this option enabled, that means users with empty string as usernames also aren't. +

+ +

+ With this option disabled, clients connecting with an empty username will be reject with 'bad username or password' as MQTT error code. +

+ +

+ The default is to be unambigious, but this can be overridden with this option. +

+

+ Default value: false +

+
+ +
rlimit_nofile number#

diff --git a/mqttpacket.cpp b/mqttpacket.cpp index 7961ed65..be48fecf 100644 --- a/mqttpacket.cpp +++ b/mqttpacket.cpp @@ -263,8 +263,8 @@ MqttPacket::MqttPacket(const Connect &connect) : writeByte(protocolVersionByte); uint8_t flags = connect.clean_start << 1; - flags |= !connect.username.empty() << 7; - flags |= !connect.password.empty() << 6; + flags |= static_cast(connect.username.has_value()) << 7; + flags |= static_cast(connect.password.has_value()) << 6; if (connect.will) { @@ -295,10 +295,10 @@ MqttPacket::MqttPacket(const Connect &connect) : writeString(connect.will->payload); } - if (!connect.username.empty()) - writeString(connect.username); - if (!connect.password.empty()) - writeString(connect.password); + if (connect.username.has_value()) + writeString(connect.username.value()); + if (connect.password.has_value()) + writeString(connect.password.value()); calculateRemainingLength(); } @@ -679,11 +679,22 @@ ConnectData MqttPacket::parseConnectData() if (user_name_flag) { + // Usernames must be UTF-8, but we defer that check so we can give proper a CONNACK, and continue parsing. result.username = readBytesToString(false); - result.willpublish.username = result.username.value(); if (result.username.value().empty()) - throw ProtocolError("Username flagged as present, but it's 0 bytes.", ReasonCodes::MalformedPacket); + { + if (settings.zeroByteUsernameIsAnonymous) + result.username.reset(); + else + throw ProtocolError("Attempting anonymous login with zero byte username. See config option 'zero_byte_username_is_anonymous'.", + ReasonCodes::BadUserNameOrPassword); + } + } + + if (result.username) + { + result.willpublish.username = result.username.value(); if (!settings.allowUnsafeUsernameChars && containsDangerousCharacters(result.username.value())) throw ProtocolError(formatString("Username '%s' contains unsafe characters and 'allow_unsafe_username_chars' is false.", result.username.value().c_str()), @@ -692,15 +703,12 @@ ConnectData MqttPacket::parseConnectData() if (result.password_flag) { - if (this->protocolVersion <= ProtocolVersion::Mqtt311 && !result.username) + if (this->protocolVersion <= ProtocolVersion::Mqtt311 && !user_name_flag) { throw ProtocolError("MQTT 3.1.1: If the User Name Flag is set to 0, the Password Flag MUST be set to 0.", ReasonCodes::MalformedPacket); } result.password = readBytesToString(false); - - if (result.password.empty()) - throw ProtocolError("Password flagged as present, but it's 0 bytes.", ReasonCodes::MalformedPacket); } return result; diff --git a/settings.h b/settings.h index 16e2b91e..20713d03 100644 --- a/settings.h +++ b/settings.h @@ -91,6 +91,7 @@ class Settings SharedSubscriptionTargeting sharedSubscriptionTargeting = SharedSubscriptionTargeting::RoundRobin; uint16_t minimumWildcardSubscriptionDepth = 0; WildcardSubscriptionDenyMode wildcardSubscriptionDenyMode = WildcardSubscriptionDenyMode::DenyAll; + bool zeroByteUsernameIsAnonymous = false; std::list> listeners; // Default one is created later, when none are defined. std::list setRealIpFrom; diff --git a/types.cpp b/types.cpp index 5366a3b3..fe4ff711 100644 --- a/types.cpp +++ b/types.cpp @@ -471,11 +471,11 @@ size_t Connect::getLengthWithoutFixedHeader() const result += will->payload.length() + 2; } - if (!username.empty()) - result += username.size() + 2; + if (username.has_value()) + result += username->size() + 2; - if (!password.empty()) - result += password.size() + 2; + if (password.has_value()) + result += password->size() + 2; return result; } diff --git a/types.h b/types.h index c39e57dd..e4746a2d 100644 --- a/types.h +++ b/types.h @@ -17,6 +17,7 @@ See LICENSE for license details. #include #include #include +#include #include "forward_declarations.h" @@ -311,8 +312,8 @@ struct Connect bool clean_start = true; bool bridgeProtocolBit = false; std::string clientid; - std::string username; - std::string password; + std::optional username; + std::optional password; uint16_t keepalive = 60; std::shared_ptr will; std::shared_ptr propertyBuilder;