From d7e80385526bd0ee60c51a99479e790e8fb4733c Mon Sep 17 00:00:00 2001 From: quinox Date: Fri, 1 Mar 2024 19:34:18 +0100 Subject: [PATCH] Tighten config parser The config parser was rather liberal in what it accepted, for example these three settings would all be accepted and interpreted as 180 seconds: expire_sessions_after_seconds 180s expire_sessions_after_seconds 180h expire_sessions_after_seconds 180 hours This changeset will cause these things to get rejected. --- FlashMQTests/tst_maintests.cpp | 118 +++++++++++++++++++++++++++++++++ FlashMQTests/tst_maintests.h | 2 + configfileparser.cpp | 99 ++++++++++++++++++++------- configfileparser.h | 1 + 4 files changed, 197 insertions(+), 23 deletions(-) diff --git a/FlashMQTests/tst_maintests.cpp b/FlashMQTests/tst_maintests.cpp index 2334c8e..74057dc 100644 --- a/FlashMQTests/tst_maintests.cpp +++ b/FlashMQTests/tst_maintests.cpp @@ -9,6 +9,9 @@ See LICENSE for license details. */ #include "tst_maintests.h" +#include "configfileparser.h" +#include "exceptions.h" +#include "settings.h" #include #include @@ -505,6 +508,121 @@ void MainTests::test_loading_acl_file() QVERIFY(true); } +void MainTests::test_loading_second_value() +{ + /* this is expected to work*/ + { + ConfFileTemp config; + config.writeLine("bridge {"); + config.writeLine(" address localhost"); + config.writeLine(" publish send/this 1"); // this value should be different from the default (0) + config.writeLine("}"); + config.closeFile(); + + ConfigFileParser *parser = new ConfigFileParser(config.getFilePath()); + parser->loadFile(false); + + Settings settings = parser->getSettings(); + + std::shared_ptr bridge = settings.stealBridges().front(); + + QCOMPARE(bridge->publishes[0].topic, "send/this"); + QCOMPARE(bridge->publishes[0].qos, (uint8_t)1); + } + + /* this is expecte to fail because "address" doesn't take a second value */ + { + ConfFileTemp config; + config.writeLine("bridge {"); + config.writeLine(" address localhost thisisnotok"); + config.writeLine(" publish send/this 1"); + config.writeLine("}"); + config.closeFile(); + + ConfigFileParser *parser = new ConfigFileParser(config.getFilePath()); + try + { + parser->loadFile(false); + QFAIL("The config parser is too liberal"); + } + catch (ConfigFileException ex) + { + /* Excellent, what we wanted */ + } + } +} + +void MainTests::test_parsing_numbers() +{ + /* this should work: 180 */ + { + ConfFileTemp config; + config.writeLine("expire_sessions_after_seconds 180"); + config.closeFile(); + + ConfigFileParser *parser = new ConfigFileParser(config.getFilePath()); + parser->loadFile(false); + + Settings settings = parser->getSettings(); + + QCOMPARE(settings.expireSessionsAfterSeconds, (uint32_t)180); + } + + /* this should fail: 180days */ + { + ConfFileTemp config; + config.writeLine("expire_sessions_after_seconds 180days"); + config.closeFile(); + + ConfigFileParser *parser = new ConfigFileParser(config.getFilePath()); + try + { + parser->loadFile(false); + QFAIL("The parser was too liberal"); + } + catch (ConfigFileException) + { + /* Good! This is where we want to end up in */ + } + } + + /* this should also fail: 180 days */ + { + ConfFileTemp config; + config.writeLine("expire_sessions_after_seconds 180 days"); + config.closeFile(); + + ConfigFileParser *parser = new ConfigFileParser(config.getFilePath()); + try + { + parser->loadFile(false); + QFAIL("The parser was too liberal"); + } + catch (ConfigFileException) + { + /* Good! This is where we want to end up in */ + } + } + + /* Last one that should fail: 180 days and a bit */ + { + ConfFileTemp config; + config.writeLine("expire_sessions_after_seconds 180 days and a bit more"); + config.closeFile(); + + ConfigFileParser *parser = new ConfigFileParser(config.getFilePath()); + try + { + parser->loadFile(false); + QFAIL("The parser was too liberal"); + } + catch (ConfigFileException) + { + /* Good! This is where we want to end up in */ + } + } +} + #ifndef FMQ_NO_SSE void MainTests::test_sse_split() { diff --git a/FlashMQTests/tst_maintests.h b/FlashMQTests/tst_maintests.h index 1801339..368fabb 100644 --- a/FlashMQTests/tst_maintests.h +++ b/FlashMQTests/tst_maintests.h @@ -94,6 +94,8 @@ private slots: void test_acl_patterns_clientid(); void test_loading_acl_file(); + void test_loading_second_value(); + void test_parsing_numbers(); void test_validUtf8Generic(); #ifndef FMQ_NO_SSE void test_sse_split(); diff --git a/configfileparser.cpp b/configfileparser.cpp index cad019c..dc7c859 100644 --- a/configfileparser.cpp +++ b/configfileparser.cpp @@ -24,6 +24,52 @@ See LICENSE for license details. #include "utils.h" #include "globber.h" +/** + * @brief Like std::stoi, but demands that the entire value is consumed + * @param key Unused except for informing the user in case of problems + * @param value the string to parse + * @return the parsed integer + */ +int full_stoi(const std::string &key, const std::string &value) +{ + size_t ptr; + int newVal = std::stoi(value, &ptr); + if (ptr != value.length()) + { + throw ConfigFileException(formatString("%s's value of '%s' can't be parsed to a number", key.c_str(), value.c_str())); + } + return newVal; +} + +/** + * @brief Like std::stoul, but demands that the entire value is consumed + * @param key Unused except for informing the user in case of problems + * @param value the string to parse + * @return the parsed unsigned long + **/ +unsigned long full_stoul(const std::string &key, const std::string &value) +{ + size_t ptr; + unsigned long newVal = std::stoul(value, &ptr); + if (ptr != value.length()) + { + throw ConfigFileException(formatString("%s's value of '%s' can't be parsed to a number", key.c_str(), value.c_str())); + } + return newVal; +} + +void ConfigFileParser::testCorrectNumberOfValues(const std::string &key, int expected_values, std::smatch &matches) +{ + int encountered_values = matches.size() - 2; // 0 = full line, 1 = key, 2 = first value, 3 = second value etc. + if (encountered_values != expected_values) + { + const std::string &rest = matches[expected_values + 2]; + if (!rest.empty()) + { + throw ConfigFileException(formatString("Option %s expected %d, got %d arguments (%s ...)", key.c_str(), expected_values, encountered_values, rest.c_str())); + } + } +} /** * @brief ConfigFileParser::testKeyValidity tests if two strings match and whether it's a valid config key. @@ -366,6 +412,7 @@ void ConfigFileParser::loadFile(bool test) std::string key = matches[1].str(); const std::string value = matches[2].str(); + int number_of_expected_values = 1; // Most lines only accept 1 argument, a select few 2. std::string valueTrimmed = value; trim(valueTrimmed); @@ -381,7 +428,7 @@ void ConfigFileParser::loadFile(bool test) } else if (testKeyValidity(key, "port", validListenKeys)) { - curListener->port = std::stoi(value); + curListener->port = full_stoi(key, value); } else if (testKeyValidity(key, "fullchain", validListenKeys)) { @@ -438,6 +485,7 @@ void ConfigFileParser::loadFile(bool test) curListener->allowAnonymous = val ? AllowListenerAnonymous::Yes : AllowListenerAnonymous::No; } + testCorrectNumberOfValues(key, number_of_expected_values, matches); continue; } else if (curParseLevel == ConfigParseLevel::Bridge) @@ -460,7 +508,7 @@ void ConfigFileParser::loadFile(bool test) } if (testKeyValidity(key, "remote_session_expiry_interval", validBridgeKeys)) { - int64_t newVal = std::stoi(value); + int64_t newVal = full_stoi(key, value); if (newVal <= 0 || newVal > std::numeric_limits::max()) { throw ConfigFileException(formatString("Value '%d' doesn't fit in uint32_t.", newVal)); @@ -473,7 +521,7 @@ void ConfigFileParser::loadFile(bool test) } if (testKeyValidity(key, "local_session_expiry_interval", validBridgeKeys)) { - int64_t newVal = std::stoi(value); + int64_t newVal = full_stoi(key, value); if (newVal <= 0 || newVal > std::numeric_limits::max()) { throw ConfigFileException(formatString("Value '%d' doesn't fit in uint32_t.", newVal)); @@ -489,11 +537,12 @@ void ConfigFileParser::loadFile(bool test) if (matches.size() == 4) { + number_of_expected_values = 2; const std::string &qosstr = matches[3]; if (!qosstr.empty()) { - topicPath.qos = std::stoul(qosstr); + topicPath.qos = full_stoul(key, qosstr); if (!topicPath.isValidQos()) throw ConfigFileException(formatString("Qos '%s' is not a valid qos level", qosstr.c_str())); @@ -512,11 +561,12 @@ void ConfigFileParser::loadFile(bool test) if (matches.size() == 4) { + number_of_expected_values = 2; const std::string &qosstr = matches[3]; if (!qosstr.empty()) { - topicPath.qos = std::stoul(qosstr); + topicPath.qos = full_stoul(key, qosstr); if (!topicPath.isValidQos()) throw ConfigFileException(formatString("Qos '%s' is not a valid qos level", qosstr.c_str())); @@ -542,7 +592,7 @@ void ConfigFileParser::loadFile(bool test) } if (testKeyValidity(key, "port", validBridgeKeys)) { - const int64_t newVal = std::stoi(value); + const int64_t newVal = full_stoi(key, value); if (newVal <= 0 || newVal > std::numeric_limits::max()) { throw ConfigFileException(formatString("Value '%d' doesn't fit in uint16_t.", newVal)); @@ -570,7 +620,7 @@ void ConfigFileParser::loadFile(bool test) } if (testKeyValidity(key, "keepalive", validBridgeKeys)) { - int64_t newVal = std::stoi(value); + int64_t newVal = full_stoi(key, value); if (newVal < 10 || newVal > std::numeric_limits::max()) { throw ConfigFileException(formatString("Value '%d' doesn't fit in uint16_t and must be at least 10.", newVal)); @@ -604,7 +654,7 @@ void ConfigFileParser::loadFile(bool test) } if (testKeyValidity(key, "max_incoming_topic_aliases", validBridgeKeys)) { - const int64_t newVal = std::stoi(value); + const int64_t newVal = full_stoi(key, value); if (newVal < 0 || newVal > std::numeric_limits::max()) { throw ConfigFileException(formatString("Value '%d' doesn't fit in uint16_t.", newVal)); @@ -613,7 +663,7 @@ void ConfigFileParser::loadFile(bool test) } if (testKeyValidity(key, "max_outgoing_topic_aliases", validBridgeKeys)) { - const int64_t newVal = std::stoi(value); + const int64_t newVal = full_stoi(key, value); if (newVal < 0 || newVal > std::numeric_limits::max()) { throw ConfigFileException(formatString("Value '%d' doesn't fit in uint16_t.", newVal)); @@ -640,6 +690,7 @@ void ConfigFileParser::loadFile(bool test) curBridge->remoteRetainAvailable = stringTruthiness(value); } + testCorrectNumberOfValues(key, number_of_expected_values, matches); continue; } @@ -696,7 +747,7 @@ void ConfigFileParser::loadFile(bool test) if (testKeyValidity(key, "client_initial_buffer_size", validKeys)) { - int newVal = std::stoi(value); + int newVal = full_stoi(key, value); if (!isPowerOfTwo(newVal)) throw ConfigFileException("client_initial_buffer_size value " + value + " is not a power of two."); tmpSettings.clientInitialBufferSize = newVal; @@ -704,7 +755,7 @@ void ConfigFileParser::loadFile(bool test) if (testKeyValidity(key, "max_packet_size", validKeys)) { - int newVal = std::stoi(value); + int newVal = full_stoi(key, value); if (newVal > ABSOLUTE_MAX_PACKET_SIZE) { std::ostringstream oss; @@ -746,7 +797,7 @@ void ConfigFileParser::loadFile(bool test) if (testKeyValidity(key, "rlimit_nofile", validKeys)) { - int newVal = std::stoi(value); + int newVal = full_stoi(key, value); if (newVal <= 0) { throw ConfigFileException(formatString("Value '%d' is negative.", newVal)); @@ -756,7 +807,7 @@ void ConfigFileParser::loadFile(bool test) if (testKeyValidity(key, "expire_sessions_after_seconds", validKeys)) { - uint32_t newVal = std::stoi(value); + uint32_t newVal = full_stoi(key, value); if (newVal > 0 && newVal < 60) // 0 means disable { throw ConfigFileException(formatString("expire_sessions_after_seconds value '%d' is invalid. Valid values are 0, or 60 or higher.", newVal)); @@ -766,7 +817,7 @@ void ConfigFileParser::loadFile(bool test) if (testKeyValidity(key, "plugin_timer_period", validKeys)) { - int newVal = std::stoi(value); + int newVal = full_stoi(key, value); if (newVal < 0) { throw ConfigFileException(formatString("plugin_timer_period value '%d' is invalid. Valid values are 0 or higher. 0 means disabled.", newVal)); @@ -784,7 +835,7 @@ void ConfigFileParser::loadFile(bool test) if (testKeyValidity(key, "thread_count", validKeys)) { - int newVal = std::stoi(value); + int newVal = full_stoi(key, value); if (newVal < 0) { throw ConfigFileException(formatString("thread_count value '%d' is invalid. Valid values are 0 or higher. 0 means auto.", newVal)); @@ -794,7 +845,7 @@ void ConfigFileParser::loadFile(bool test) if (testKeyValidity(key, "max_qos_msg_pending_per_client", validKeys)) { - int newVal = std::stoi(value); + int newVal = full_stoi(key, value); if (newVal < 32 || newVal > 65535) { throw ConfigFileException(formatString("max_qos_msg_pending_per_client value '%d' is invalid. Valid values between 32 and 65535.", newVal)); @@ -804,7 +855,7 @@ void ConfigFileParser::loadFile(bool test) if (testKeyValidity(key, "max_qos_bytes_pending_per_client", validKeys)) { - int newVal = std::stoi(value); + int newVal = full_stoi(key, value); if (newVal < 4096) { throw ConfigFileException(formatString("max_qos_bytes_pending_per_client value '%d' is invalid. Valid values are 4096 or higher.", newVal)); @@ -814,7 +865,7 @@ void ConfigFileParser::loadFile(bool test) if (testKeyValidity(key, "max_incoming_topic_alias_value", validKeys)) { - int newVal = std::stoi(value); + int newVal = full_stoi(key, value); if (newVal < 0 || newVal > 0xFFFF) { throw ConfigFileException(formatString("max_incoming_topic_alias_value value '%d' is invalid. Valid values are between 0 and 65535.", newVal)); @@ -824,7 +875,7 @@ void ConfigFileParser::loadFile(bool test) if (testKeyValidity(key, "max_outgoing_topic_alias_value", validKeys)) { - int newVal = std::stoi(value); + int newVal = full_stoi(key, value); if (newVal < 0 || newVal > 0xFFFF) { throw ConfigFileException(formatString("max_outgoing_topic_alias_value value '%d' is invalid. Valid values are between 0 and 65535.", newVal)); @@ -856,7 +907,7 @@ void ConfigFileParser::loadFile(bool test) if (testKeyValidity(key, "expire_retained_messages_after_seconds", validKeys)) { - uint32_t newVal = std::stoi(value); + uint32_t newVal = full_stoi(key, value); if (newVal < 1) { throw ConfigFileException(formatString("expire_retained_messages_after_seconds value '%d' is invalid. Valid values are between 1 and 4294967296.", newVal)); @@ -890,7 +941,7 @@ void ConfigFileParser::loadFile(bool test) if (testKeyValidity(key, "client_max_write_buffer_size", validKeys)) { const uint32_t minVal = 4096; - uint32_t newVal = std::stoul(value); + uint32_t newVal = full_stoul(key, value); if (newVal < minVal) throw ConfigFileException(formatString("Value '%s' for '%s' is too low. It must be at least %d.", value.c_str(), key.c_str(), minVal)); @@ -905,7 +956,7 @@ void ConfigFileParser::loadFile(bool test) if (testKeyValidity(key, "retained_messages_node_limit", validKeys)) { - const uint32_t newVal = std::stoul(value); + const uint32_t newVal = full_stoul(key, value); if (newVal == 0) throw ConfigFileException("Set '" + key + "' higher than 0, or use 'retained_messages_mode'."); @@ -915,7 +966,7 @@ void ConfigFileParser::loadFile(bool test) if (testKeyValidity(key, "minimum_wildcard_subscription_depth", validKeys)) { - const unsigned long newVal = std::stoul(value); + const unsigned long newVal = full_stoul(key, value); if (newVal > 0xFFFF) throw ConfigFileException("Option '" + key + "' must be between 0 and 65535."); @@ -940,6 +991,8 @@ void ConfigFileParser::loadFile(bool test) { throw ConfigFileException(ex.what()); } + + testCorrectNumberOfValues(key, number_of_expected_values, matches); } tmpSettings.checkUniqueBridgeNames(); diff --git a/configfileparser.h b/configfileparser.h index 676ebaf..5daef73 100644 --- a/configfileparser.h +++ b/configfileparser.h @@ -42,6 +42,7 @@ class ConfigFileParser Settings settings; + void testCorrectNumberOfValues(const std::string &key, int expected_values, std::smatch &matches); bool testKeyValidity(const std::string &key, const std::string &matchKey, const std::set &validKeys) const; void static checkFileExistsAndReadable(const std::string &key, const std::string &pathToCheck, ssize_t max_size = std::numeric_limits::max()); void static checkFileOrItsDirWritable(const std::string &filepath);