From f7e9039f20dc9d43da81d921ca8efd2305b03d62 Mon Sep 17 00:00:00 2001 From: Felipe Zimmerle Date: Wed, 29 Apr 2020 10:19:49 -0300 Subject: [PATCH] Cosmetics: fix some cppcheck complains --- Makefile.am | 2 +- headers/modsecurity/actions/action.h | 9 ++++ headers/modsecurity/rule.h | 7 ++++ headers/modsecurity/rule_message.h | 13 ++++++ headers/modsecurity/rule_with_actions.h | 50 +++++++++++++++++++++-- headers/modsecurity/rule_with_operator.h | 7 ++++ headers/modsecurity/rules.h | 2 +- headers/modsecurity/transaction.h | 2 +- src/actions/transformations/url_decode.cc | 2 +- src/engine/lua.cc | 27 ++++++------ src/engine/lua.h | 3 +- src/modsecurity.cc | 6 +-- src/operators/detect_sqli.cc | 37 +++++++++-------- src/operators/rx.cc | 2 +- src/rule_script.h | 6 +++ src/transaction.cc | 1 - src/utils/geo_lookup.cc | 11 ++++- src/utils/shared_files.h | 2 +- src/variables/web_app_id.h | 1 - test/cppcheck_suppressions.txt | 19 ++++++++- test/fuzzer/afl_fuzzer.cc | 7 +++- 21 files changed, 164 insertions(+), 52 deletions(-) diff --git a/Makefile.am b/Makefile.am index 734b6f35..cce97ec6 100644 --- a/Makefile.am +++ b/Makefile.am @@ -57,7 +57,7 @@ parser: cppcheck: @cppcheck -U YYSTYPE -U MBEDTLS_MD5_ALT -U MBEDTLS_SHA1_ALT \ - -D MS_CPPCHECK_DISABLED_FOR_PARSER \ + -D MS_CPPCHECK_DISABLED_FOR_PARSER -U YY_USER_INIT \ --suppressions-list=./test/cppcheck_suppressions.txt \ --enable=all \ --inconclusive \ diff --git a/headers/modsecurity/actions/action.h b/headers/modsecurity/actions/action.h index 4d08a54b..1f143e31 100644 --- a/headers/modsecurity/actions/action.h +++ b/headers/modsecurity/actions/action.h @@ -63,6 +63,15 @@ class Action { m_name(a.m_name), m_parser_payload(a.m_parser_payload) { } + Action &operator=(const Action& a) { + m_isNone = a.m_isNone; + temporaryAction = a.temporaryAction; + action_kind = a.action_kind; + m_name = a.m_name; + m_parser_payload = a.m_parser_payload; + return *this; + } + virtual ~Action() { } virtual std::string execute(const std::string &exp, diff --git a/headers/modsecurity/rule.h b/headers/modsecurity/rule.h index ccaeb997..a81bdd16 100644 --- a/headers/modsecurity/rule.h +++ b/headers/modsecurity/rule.h @@ -56,6 +56,13 @@ class Rule { } + Rule &operator=(const Rule& other) { + m_fileName = other.m_fileName; + m_lineNumber = other.m_lineNumber; + m_phase = other.m_phase; + return *this; + } + virtual bool evaluate(Transaction *transaction) = 0; std::shared_ptr getFileName() const { diff --git a/headers/modsecurity/rule_message.h b/headers/modsecurity/rule_message.h index 4352c503..61eeecea 100644 --- a/headers/modsecurity/rule_message.h +++ b/headers/modsecurity/rule_message.h @@ -55,6 +55,19 @@ class RuleMessage { { } + RuleMessage &operator=(const RuleMessage& ruleMessage) { + m_severity = ruleMessage.m_severity; + m_tags = ruleMessage.m_tags; + m_data = ruleMessage.m_data; + m_match = ruleMessage.m_match; + m_message = ruleMessage.m_message; + m_reference = ruleMessage.m_reference; + m_transaction = ruleMessage.m_transaction; + m_rule = ruleMessage.m_rule; + return *this; + } + + explicit RuleMessage(Transaction *transaction) : m_severity(0), m_tags(), diff --git a/headers/modsecurity/rule_with_actions.h b/headers/modsecurity/rule_with_actions.h index b0d54e07..50435847 100644 --- a/headers/modsecurity/rule_with_actions.h +++ b/headers/modsecurity/rule_with_actions.h @@ -124,6 +124,50 @@ class RuleWithActions : public Rule { m_defaultContainsStaticBlockAction(r.m_defaultContainsStaticBlockAction), m_isChained(r.m_isChained) { }; + RuleWithActions &operator=(const RuleWithActions& r) { + Rule::operator = (r); + m_ruleId = r.m_ruleId; + m_chainedRuleChild = r.m_chainedRuleChild; + m_chainedRuleParent = r.m_chainedRuleParent; + m_disruptiveAction = r.m_disruptiveAction; + m_logData = r.m_logData; + m_msg = r.m_msg; + m_actionsRuntimePos = r.m_actionsRuntimePos; + m_actionsSetVar = r.m_actionsSetVar; + m_actionsTag = r.m_actionsTag; + m_XmlNSs = r.m_XmlNSs; + m_defaultActionDisruptiveAction = r.m_defaultActionDisruptiveAction; + m_defaultActionLogData = r.m_defaultActionLogData; + m_defaultActionMsg = r.m_defaultActionMsg; + m_defaultActionActionsRuntimePos = r.m_defaultActionActionsRuntimePos; + m_defaultActionActionsSetVar = r.m_defaultActionActionsSetVar; + m_defaultActionActionsTag = r.m_defaultActionActionsTag; + m_transformations = r.m_transformations; + m_defaultTransformations = r.m_defaultTransformations; + m_severity = r.m_severity; + m_revision = r.m_revision; + m_version = r.m_version; + m_accuracy = r.m_accuracy; + m_maturity = r.m_maturity; + m_containsCaptureAction = r.m_containsCaptureAction; + m_containsLogAction = r.m_containsLogAction; + m_containsNoLogAction = r.m_containsNoLogAction; + m_containsMultiMatchAction = r.m_containsMultiMatchAction; + m_containsStaticBlockAction = r.m_containsStaticBlockAction; + m_defaultSeverity = r.m_defaultSeverity; + m_defaultRevision = r.m_defaultRevision; + m_defaultVersion = r.m_defaultVersion; + m_defaultAccuracy = r.m_defaultAccuracy; + m_defaultMaturity = r.m_defaultMaturity; + m_defaultContainsCaptureAction = r.m_defaultContainsCaptureAction; + m_defaultContainsLogAction = r.m_defaultContainsLogAction; + m_defaultContainsNoLogAction = r.m_defaultContainsNoLogAction; + m_defaultContainsMultiMatchAction = r.m_defaultContainsMultiMatchAction; + m_defaultContainsStaticBlockAction = r.m_defaultContainsStaticBlockAction; + m_isChained = r.m_isChained; + return *this; + } + virtual bool evaluate(Transaction *transaction) override; @@ -263,7 +307,7 @@ class RuleWithActions : public Rule { inline bool hasCaptureAction() const { return m_containsCaptureAction || m_defaultContainsCaptureAction; } inline bool hasDisruptiveAction() const { return m_disruptiveAction != nullptr || m_defaultActionDisruptiveAction != nullptr; } - inline void setDisruptiveAction(std::shared_ptr a) { m_disruptiveAction = a; } + inline void setDisruptiveAction(const std::shared_ptr &a) { m_disruptiveAction = a; } inline std::shared_ptr getDisruptiveAction() const { return m_disruptiveAction; } inline bool hasBlockAction() const { return m_containsStaticBlockAction || m_defaultContainsStaticBlockAction; } @@ -279,11 +323,11 @@ class RuleWithActions : public Rule { inline bool hasLogDataAction() const { return m_logData != nullptr || m_defaultActionLogData != nullptr; } inline std::shared_ptr getLogDataAction() const { return m_logData; } std::string getLogData(/*const */Transaction *t); - inline void setLogDataAction(std::shared_ptr data) { m_logData = data; } + inline void setLogDataAction(const std::shared_ptr &data) { m_logData = data; } inline bool hasMessageAction() const { return m_msg != nullptr || m_defaultActionMsg != nullptr; } inline std::shared_ptr getMessageAction() const { return m_msg; } - inline void setMessageAction(std::shared_ptr msg) { m_msg = msg; } + inline void setMessageAction(const std::shared_ptr &msg) { m_msg = msg; } std::string getMessage(/*const */Transaction *t); diff --git a/headers/modsecurity/rule_with_operator.h b/headers/modsecurity/rule_with_operator.h index 13860b27..f476ec1e 100644 --- a/headers/modsecurity/rule_with_operator.h +++ b/headers/modsecurity/rule_with_operator.h @@ -50,6 +50,13 @@ class RuleWithOperator : public RuleWithActions { m_variables(op.m_variables), m_operator(op.m_operator) { }; + RuleWithOperator &operator=(const RuleWithOperator& r) { + RuleWithActions::operator = (r); + m_variables = r.m_variables; + m_operator = r.m_operator; + return *this; + } + virtual ~RuleWithOperator(); bool evaluate(Transaction *transaction) override; diff --git a/headers/modsecurity/rules.h b/headers/modsecurity/rules.h index e98609c2..1ed88ebc 100644 --- a/headers/modsecurity/rules.h +++ b/headers/modsecurity/rules.h @@ -63,7 +63,7 @@ class Rules { return j; } - bool insert(std::shared_ptr rule) { + bool insert(const std::shared_ptr &rule) { return insert(rule, nullptr, nullptr); } diff --git a/headers/modsecurity/transaction.h b/headers/modsecurity/transaction.h index c986a0da..a99e9bd0 100644 --- a/headers/modsecurity/transaction.h +++ b/headers/modsecurity/transaction.h @@ -309,7 +309,7 @@ class TransactionSecMarkerManagement { m_marker.reset(); } - void addMarker(std::shared_ptr name) { + void addMarker(const std::shared_ptr &name) { m_marker = name; } diff --git a/src/actions/transformations/url_decode.cc b/src/actions/transformations/url_decode.cc index 1e241ac4..87b7aad9 100644 --- a/src/actions/transformations/url_decode.cc +++ b/src/actions/transformations/url_decode.cc @@ -39,7 +39,7 @@ UrlDecode::UrlDecode(const std::string &action) std::string UrlDecode::execute(const std::string &value, Transaction *transaction) { - unsigned char *val = NULL; + unsigned char *val(NULL); int invalid_count = 0; int changed; diff --git a/src/engine/lua.cc b/src/engine/lua.cc index 48a32d45..6addf0f5 100644 --- a/src/engine/lua.cc +++ b/src/engine/lua.cc @@ -65,8 +65,7 @@ bool Lua::isCompatible(const std::string &script, Lua *l, std::string *error) { bool Lua::load(const std::string &script, std::string *err) { #ifdef WITH_LUA - lua_State *L = NULL; - L = luaL_newstate(); + lua_State *L = luaL_newstate(); luaL_openlibs(L); m_scriptName = script; @@ -234,7 +233,7 @@ err: #ifdef WITH_LUA int Lua::log(lua_State *L) { - const Transaction *t = NULL; + const Transaction *t(NULL); const char *text; int level; @@ -256,9 +255,9 @@ int Lua::log(lua_State *L) { int Lua::getvar(lua_State *L) { - const char *varname = NULL; - Transaction *t = NULL; - void *z = NULL; + const char *varname(NULL); + Transaction *t(NULL); + void *z(NULL); /* Retrieve parameters. */ varname = reinterpret_cast(luaL_checkstring(L, 1)); @@ -282,9 +281,9 @@ int Lua::getvar(lua_State *L) { int Lua::getvars(lua_State *L) { - const char *varname = NULL; - Transaction *t = NULL; - void *z = NULL; + const char *varname(NULL); + Transaction *t(NULL); + void *z(NULL); std::vector l; int idx = 1; @@ -323,16 +322,16 @@ int Lua::getvars(lua_State *L) { int Lua::setvar(lua_State *L) { - Transaction *t = NULL; - const char *var_value = NULL; - const char *var_name = NULL; + Transaction *t(NULL); + const char *var_value(NULL); + const char *var_name(NULL); std::string vname; std::string collection; std::string variableName; int nargs = lua_gettop(L); char *chr = NULL; size_t pos; - void *z = NULL; + void *z(NULL); lua_getglobal(L, "__transaction"); z = const_cast(lua_topointer(L, -1)); @@ -453,7 +452,7 @@ std::string Lua::applyTransformations(lua_State *L, Transaction *t, } if (lua_isstring(L, idx)) { - const char *name = NULL; + const char *name(NULL); name = reinterpret_cast(luaL_checkstring(L, idx)); actions::transformations::Transformation *tfn = \ diff --git a/src/engine/lua.h b/src/engine/lua.h index d2c209f3..02882e84 100644 --- a/src/engine/lua.h +++ b/src/engine/lua.h @@ -45,8 +45,7 @@ class LuaScriptBlob { void write(const void *data, size_t len) { - unsigned char *d = NULL; - d = (unsigned char *)realloc((unsigned char *)m_data, len + m_len); + unsigned char *d = (unsigned char *)realloc((unsigned char *)m_data, len + m_len); std::memcpy(d + m_len, data, len); m_len = m_len + len; m_data = d; diff --git a/src/modsecurity.cc b/src/modsecurity.cc index 4d61fbd0..e10f0f36 100644 --- a/src/modsecurity.cc +++ b/src/modsecurity.cc @@ -256,7 +256,7 @@ int ModSecurity::processContentOffset(const char *content, size_t len, strlen("highlight")); yajl_gen_array_open(g); - while (vars.size() > 0) { + while (!vars.empty()) { std::string value; yajl_gen_map_open(g); vars.pop_back(); @@ -303,7 +303,7 @@ int ModSecurity::processContentOffset(const char *content, size_t len, varValue.size()); yajl_gen_map_close(g); - while (trans.size() > 0) { + while (!trans.empty()) { modsecurity::actions::transformations::Transformation *t; std::string varValueRes; yajl_gen_map_open(g); @@ -338,7 +338,7 @@ int ModSecurity::processContentOffset(const char *content, size_t len, yajl_gen_map_open(g); - while (ops.size() > 0) { + while (!ops.empty()) { std::string value; yajl_gen_string(g, reinterpret_cast("highlight"), strlen("highlight")); diff --git a/src/operators/detect_sqli.cc b/src/operators/detect_sqli.cc index e036599f..232ef9f4 100644 --- a/src/operators/detect_sqli.cc +++ b/src/operators/detect_sqli.cc @@ -32,26 +32,27 @@ bool DetectSQLi::evaluate(Transaction *t, RuleWithActions *rule, issqli = libinjection_sqli(input.c_str(), input.length(), fingerprint); - if (issqli) { - if (t) { - t->m_matched.push_back(fingerprint); - ms_dbg_a(t, 4, "detected SQLi using libinjection with " \ - "fingerprint '" + std::string(fingerprint) + "' at: '" + - input + "'"); - if (rule && t && rule->hasCaptureAction()) { - t->m_collections.m_tx_collection->storeOrUpdateFirst( - "0", std::string(fingerprint)); - ms_dbg_a(t, 7, "Added DetectSQLi match TX.0: " + \ - std::string(fingerprint)); - } - } - } else { - if (t) { - ms_dbg_a(t, 9, "detected SQLi: not able to find an " \ - "inject on '" + input + "'"); - } + if (!t) { + goto tisempty; } + if (issqli) { + t->m_matched.push_back(fingerprint); + ms_dbg_a(t, 4, "detected SQLi using libinjection with " \ + "fingerprint '" + std::string(fingerprint) + "' at: '" + + input + "'"); + if (rule && rule->hasCaptureAction()) { + t->m_collections.m_tx_collection->storeOrUpdateFirst( + "0", std::string(fingerprint)); + ms_dbg_a(t, 7, "Added DetectSQLi match TX.0: " + \ + std::string(fingerprint)); + } + } else { + ms_dbg_a(t, 9, "detected SQLi: not able to find an " \ + "inject on '" + input + "'"); + } + +tisempty: return issqli != 0; } diff --git a/src/operators/rx.cc b/src/operators/rx.cc index d561bec5..84ca30af 100644 --- a/src/operators/rx.cc +++ b/src/operators/rx.cc @@ -74,7 +74,7 @@ bool Rx::evaluate(Transaction *transaction, RuleWithActions *rule, delete re; } - if (matches.size() > 0) { + if (!matches.empty()) { return true; } diff --git a/src/rule_script.h b/src/rule_script.h index 352fa483..2e4c2086 100644 --- a/src/rule_script.h +++ b/src/rule_script.h @@ -58,6 +58,12 @@ class RuleScript : public RuleWithActions { m_name(rs.m_name), m_lua(rs.m_lua) { } + RuleScript &operator=(const RuleScript& r) { + RuleWithActions::operator = (r); + m_name = r.m_name; + m_lua = r.m_lua; + return *this; + } bool init(std::string *err); bool evaluate(Transaction *trans) override; diff --git a/src/transaction.cc b/src/transaction.cc index 0c4a1727..20f8cd76 100644 --- a/src/transaction.cc +++ b/src/transaction.cc @@ -368,7 +368,6 @@ bool Transaction::extractArguments(const std::string &orig, for (std::string t : key_value_sets) { char sep2 = '='; - int i = 0; size_t key_s = 0; size_t value_s = 0; int invalid = 0; diff --git a/src/utils/geo_lookup.cc b/src/utils/geo_lookup.cc index e0801f83..1092c91b 100644 --- a/src/utils/geo_lookup.cc +++ b/src/utils/geo_lookup.cc @@ -56,8 +56,12 @@ void GeoLookup::cleanUp() { bool GeoLookup::setDataBase(const std::string& filePath, std::string *err) { +#ifdef WITH_MAXMIND std::string intMax; +#endif +#ifdef WITH_GEOIP std::string intGeo; +#endif #ifdef WITH_MAXMIND int status = MMDB_open(filePath.c_str(), MMDB_MODE_MMAP, &mmdb); @@ -85,19 +89,22 @@ bool GeoLookup::setDataBase(const std::string& filePath, #ifdef WITH_MAXMIND err->append(" libMaxMind"); #endif - #ifdef WITH_GEOIP err->append(" GeoIP"); #endif err->append("."); +#ifdef WITH_MAXMIND if (intMax.size() > 0) { err->append(" " + intMax); - } +#endif +#ifdef WITH_GEOIP if (intGeo.size() > 0) { err->append(" " + intGeo); } +#endif + return false; } diff --git a/src/utils/shared_files.h b/src/utils/shared_files.h index 92c8d257..b5b72f3e 100644 --- a/src/utils/shared_files.h +++ b/src/utils/shared_files.h @@ -81,7 +81,7 @@ class SharedFiles { { #ifdef MODSEC_USE_GENERAL_LOCK int shm_id; - bool toBeCreated; + bool toBeCreated(false); bool err = false; m_memKeyStructure = ftok(".", 1); diff --git a/src/variables/web_app_id.h b/src/variables/web_app_id.h index d5f1aa54..47cf4229 100644 --- a/src/variables/web_app_id.h +++ b/src/variables/web_app_id.h @@ -38,7 +38,6 @@ class WebAppId : public Variable { void evaluate(Transaction *transaction, RuleWithActions *rule, std::vector *l) override { - const std::string name("WEBAPPID"); const std::string rname = transaction->m_rules->m_secWebAppId.m_value; l->push_back(new VariableValue(&m_name, &rname)); } diff --git a/test/cppcheck_suppressions.txt b/test/cppcheck_suppressions.txt index 43fe9afd..d7a54b78 100644 --- a/test/cppcheck_suppressions.txt +++ b/test/cppcheck_suppressions.txt @@ -41,8 +41,8 @@ functionStatic:headers/modsecurity/transaction.h:453 duplicateBranch:src/audit_log/audit_log.cc:223 unreadVariable:src/request_body_processor/multipart.cc:435 stlcstrParam:src/audit_log/writer/parallel.cc:145 +functionStatic:src/engine/lua.h:70 functionStatic:src/engine/lua.h:71 -functionStatic:src/engine/lua.h:72 functionConst:src/utils/geo_lookup.h:49 useInitializationList:src/operators/rbl.h:69 constStatement:test/common/modsecurity_test.cc:82 @@ -53,6 +53,9 @@ duplicateBranch:src/request_body_processor/multipart.cc:91 syntaxError:src/transaction.cc:62 noConstructor:src/variables/variable.h:152 duplicateBranch:src/request_body_processor/multipart.cc:93 +knownConditionTrueFalse:src/operators/validate_url_encoding.cc:77 +knownConditionTrueFalse:src/operators/verify_svnr.cc:87 + noExplicitConstructor:seclang-parser.hh @@ -63,3 +66,17 @@ preprocessorErrorDirective funcArgNamesDifferent unmatchedSuppression missingInclude +unusedFunction +missingIncludeSystem +useStlAlgorithm +preprocessorErrorDirective +funcArgNamesDifferent +unmatchedSuppression +missingInclude + +purgedConfiguration + + +// Examples +memleak:examples/reading_logs_via_rule_message/reading_logs_via_rule_message.h:147 +memleak:examples/using_bodies_in_chunks/simple_request.cc \ No newline at end of file diff --git a/test/fuzzer/afl_fuzzer.cc b/test/fuzzer/afl_fuzzer.cc index 1df5164c..dd4ef487 100644 --- a/test/fuzzer/afl_fuzzer.cc +++ b/test/fuzzer/afl_fuzzer.cc @@ -128,7 +128,9 @@ inline void op_test(const std::string &opName, const std::string &s) { int main(int argc, char** argv) { uint8_t buf[128]; +#if 0 std::string lastString; +#endif while (__AFL_LOOP(1000)) { ssize_t read_bytes; @@ -138,7 +140,9 @@ int main(int argc, char** argv) { std::string currentString = std::string(read_bytes, 128); std::string s = currentString; +#if 0 std::string z = lastString; +#endif ModSecurity *ms = new ModSecurity(); RulesSet *rules = new RulesSet(); @@ -266,8 +270,9 @@ op_test("Within", s); delete t; delete rules; delete ms; - +#if 0 lastString = currentString; +#endif } return 0; }