From 7ac6bf7241768021c5bd83c144122cbfd159c1cb Mon Sep 17 00:00:00 2001 From: Felipe Zimmerle Date: Sun, 27 Aug 2017 19:00:30 -0300 Subject: [PATCH] Fix memory issues while resolving variables --- headers/modsecurity/anchored_variable.h | 4 +- headers/modsecurity/collection/variable.h | 42 ++++++------ src/anchored_set_variable.cc | 14 ++-- src/anchored_variable.cc | 13 ++-- src/rule.cc | 81 +++++++++-------------- src/transaction.cc | 29 ++++---- src/variables/remote_user.cc | 27 +++++--- src/variables/variable.h | 16 ++--- src/variables/xml.cc | 4 +- src/variables/xml.h | 3 +- 10 files changed, 116 insertions(+), 117 deletions(-) diff --git a/headers/modsecurity/anchored_variable.h b/headers/modsecurity/anchored_variable.h index 7810eb9e..0b28b148 100644 --- a/headers/modsecurity/anchored_variable.h +++ b/headers/modsecurity/anchored_variable.h @@ -58,10 +58,12 @@ class AnchoredVariable { std::unique_ptr resolveFirst(); Transaction *m_transaction; - collection::Variable *m_var; int m_offset; std::string m_name; std::string m_value; + + private: + collection::Variable *m_var; }; } // namespace modsecurity diff --git a/headers/modsecurity/collection/variable.h b/headers/modsecurity/collection/variable.h index 5f499fdc..888a30f9 100644 --- a/headers/modsecurity/collection/variable.h +++ b/headers/modsecurity/collection/variable.h @@ -37,32 +37,36 @@ namespace collection { class Variable { public: explicit Variable(const std::string *key) : - m_dynamic(true) { - m_key.reset(new std::string(*key)); + m_key(""), + m_value("") { + m_key.assign(*key); } Variable(const std::string *key, const std::string *value) : - m_dynamic(true) { - m_key.reset(new std::string(*key)); - m_value.reset(new std::string(*value)); + m_key(""), + m_value("") { + m_key.assign(*key); + m_value.assign(*value); + } + Variable() : + m_key(""), + m_value("") { } + Variable(const Variable *o) : + m_key(""), + m_value("") { + m_key.assign(o->m_key); + m_value.assign(o->m_value); + for (auto &i : o->m_orign) { + std::unique_ptr origin(new VariableOrigin()); + origin->m_offset = i->m_offset; + origin->m_length = i->m_length; + m_orign.push_back(std::move(origin)); } - explicit Variable(const std::shared_ptr key) : - m_dynamic(true) { - m_key = key; - } - Variable(std::shared_ptr key, std::shared_ptr value) : - m_dynamic(true) { - m_key = key; - m_value = value; - - } - ~Variable() { } - std::shared_ptr m_key; - std::shared_ptr m_value; + std::string m_key; + std::string m_value; std::list> m_orign; - bool m_dynamic; }; } // namespace collection diff --git a/src/anchored_set_variable.cc b/src/anchored_set_variable.cc index daf73e5d..5ed96674 100644 --- a/src/anchored_set_variable.cc +++ b/src/anchored_set_variable.cc @@ -55,11 +55,12 @@ void AnchoredSetVariable::set(const std::string &key, std::string *v = new std::string(value); std::string *k = new std::string(m_name + ":" + key); collection::Variable *var = new collection::Variable(k, v); + delete v; + delete k; origin->m_offset = offset; origin->m_length = len; - var->m_dynamic = false; var->m_orign.push_back(std::move(origin)); emplace(key, var); } @@ -71,11 +72,12 @@ void AnchoredSetVariable::set(const std::string &key, std::string *v = new std::string(value); std::string *k = new std::string(m_name + ":" + key); collection::Variable *var = new collection::Variable(k, v); + delete v; + delete k; origin->m_offset = offset; origin->m_length = value.size(); - var->m_dynamic = false; var->m_orign.push_back(std::move(origin)); emplace(key, var); } @@ -84,7 +86,7 @@ void AnchoredSetVariable::set(const std::string &key, void AnchoredSetVariable::resolve( std::vector *l) { for (const auto& x : *this) { - l->insert(l->begin(), x.second); + l->insert(l->begin(), new collection::Variable(x.second)); } } @@ -93,7 +95,7 @@ void AnchoredSetVariable::resolve(const std::string &key, std::vector *l) { auto range = this->equal_range(key); for (auto it = range.first; it != range.second; ++it) { - l->push_back(it->second); + l->push_back(new collection::Variable(it->second)); } } @@ -103,7 +105,7 @@ std::unique_ptr AnchoredSetVariable::resolveFirst( auto range = equal_range(key); for (auto it = range.first; it != range.second; ++it) { std::unique_ptr b(new std::string()); - b->assign(*it->second->m_value); + b->assign(it->second->m_value); return b; } return nullptr; @@ -117,7 +119,7 @@ void AnchoredSetVariable::resolveRegularExpression(Utils::Regex *r, if (ret <= 0) { continue; } - l->insert(l->begin(), x.second); + l->insert(l->begin(), new collection::Variable(x.second)); } } diff --git a/src/anchored_variable.cc b/src/anchored_variable.cc index d80b415c..d90e9a1e 100644 --- a/src/anchored_variable.cc +++ b/src/anchored_variable.cc @@ -37,24 +37,19 @@ AnchoredVariable::AnchoredVariable(Transaction *t, m_value("") { m_name.append(name); m_var = new collection::Variable(&m_name); - m_var->m_dynamic = false; - m_var->m_value.reset(&m_value); } AnchoredVariable::~AnchoredVariable() { - /* if (m_var) { delete (m_var); m_var = NULL; } - */ } void AnchoredVariable::unset() { m_value.clear(); - m_var->m_orign.clear(); } @@ -116,11 +111,13 @@ void AnchoredVariable::append(const std::string &a, size_t offset, void AnchoredVariable::evaluate(std::vector *l) { - if (m_name.empty() || m_var == NULL || m_var->m_key == NULL - || m_var->m_value == NULL || m_var->m_key->empty()) { + if (m_name.empty() or m_value.empty()) { return; } - l->push_back(m_var); + + m_var->m_value.assign(m_value); + collection::Variable *m_var2 = new collection::Variable(m_var); + l->push_back(m_var2); } diff --git a/src/rule.cc b/src/rule.cc index 7fcb794e..83005eeb 100644 --- a/src/rule.cc +++ b/src/rule.cc @@ -431,9 +431,9 @@ std::list, std::vector> Rule::getFinalVars( Transaction *trans) { - std::list> exclusions; - std::list> exclusions_update_by_tag_remove; - std::list> exclusions_update_by_id_remove; + std::list exclusions; + std::list exclusions_update_by_tag_remove; + std::list exclusions_update_by_id_remove; std::vector variables; std::vector> finalVars; @@ -448,8 +448,9 @@ std::vector> Rule::getFinalVars( a.second->evaluateInternal(trans, this, &z); for (auto &y : z) { exclusions_update_by_tag_remove.push_back(y->m_key); + delete y; } - exclusions_update_by_tag_remove.push_back(std::make_shared(a.second->m_name)); + exclusions_update_by_tag_remove.push_back(a.second->m_name); } else { Variable *b = a.second.get(); @@ -466,8 +467,9 @@ std::vector> Rule::getFinalVars( a.second->evaluateInternal(trans, this, &z); for (auto &y : z) { exclusions_update_by_id_remove.push_back(y->m_key); + delete y; } - exclusions_update_by_id_remove.push_back(std::make_shared(a.second->m_name)); + exclusions_update_by_id_remove.push_back(a.second->m_name); } else { Variable *b = a.second.get(); variables.push_back(b); @@ -481,8 +483,9 @@ std::vector> Rule::getFinalVars( variable->evaluateInternal(trans, this, &z); for (auto &y : z) { exclusions.push_back(y->m_key); + delete y; } -// exclusions.push_back(std::make_shared(&variable->m_name)); + exclusions.push_back(variable->m_name); } } @@ -497,73 +500,68 @@ std::vector> Rule::getFinalVars( variable->evaluateInternal(trans, this, &e); for (const collection::Variable *v : e) { - const std::shared_ptr key = v->m_key; + std::string key = v->m_key; + if (std::find_if(exclusions.begin(), exclusions.end(), - [key](std::shared_ptr m) -> bool { return *key == *m.get(); }) + [key](std::string m) -> bool { return key == m; }) != exclusions.end()) { #ifndef NO_LOGS - trans->debug(9, "Variable: " + *key + + trans->debug(9, "Variable: " + key + " is part of the exclusion list, skipping..."); #endif - if (v->m_dynamic) { delete v; v = NULL; - } continue; } if (std::find_if(exclusions_update_by_tag_remove.begin(), exclusions_update_by_tag_remove.end(), - [key](std::shared_ptr m) -> bool { return *key == *m.get(); }) + [key](std::string m) -> bool { return key == m; }) != exclusions_update_by_tag_remove.end()) { #ifndef NO_LOGS - trans->debug(9, "Variable: " + *key + + trans->debug(9, "Variable: " + key + " is part of the exclusion list (from update by tag" + "), skipping..."); #endif - if (v->m_dynamic) { delete v; v = NULL; - } continue; } if (std::find_if(exclusions_update_by_id_remove.begin(), exclusions_update_by_id_remove.end(), - [key](std::shared_ptr m) -> bool { return *key == *m.get(); }) + [key](std::string m) -> bool { return key == m; }) != exclusions_update_by_id_remove.end()) { #ifndef NO_LOGS - trans->debug(9, "Variable: " + *key + + trans->debug(9, "Variable: " + key + " is part of the exclusion list (from update by ID), skipping..."); #endif - if (v->m_dynamic) { delete v; v = NULL; - } continue; } for (auto &i : trans->m_ruleRemoveTargetByTag) { std::string tag = i.first; std::string args = i.second; - size_t posa = key->find(":"); + size_t posa = key.find(":"); if (containsTag(tag, trans) == false) { continue; } - if (args == *key) { + if (args == key) { #ifndef NO_LOGS - trans->debug(9, "Variable: " + *key + + trans->debug(9, "Variable: " + key + " was excluded by ruleRemoteTargetByTag..."); #endif ignoreVariable = true; break; } if (posa != std::string::npos) { - std::string var = std::string(*key, posa); + std::string var = std::string(key, posa); if (var == args) { #ifndef NO_LOGS - trans->debug(9, "Variable: " + *key + + trans->debug(9, "Variable: " + key + " was excluded by ruleRemoteTargetByTag..."); #endif ignoreVariable = true; @@ -572,33 +570,31 @@ std::vector> Rule::getFinalVars( } } if (ignoreVariable) { - if (v->m_dynamic) { delete v; v = NULL; - } continue; } for (auto &i : trans->m_ruleRemoveTargetById) { int id = i.first; std::string args = i.second; - size_t posa = key->find(":"); + size_t posa = key.find(":"); if (m_ruleId != id) { continue; } - if (args == *key) { + if (args == key) { #ifndef NO_LOGS - trans->debug(9, "Variable: " + *key + + trans->debug(9, "Variable: " + key + " was excluded by ruleRemoveTargetById..."); #endif ignoreVariable = true; break; } if (posa != std::string::npos) { - if (key->size() > posa) { - std::string var = std::string(*key, 0, posa); + if (key.size() > posa) { + std::string var = std::string(key, 0, posa); if (var == args) { #ifndef NO_LOGS trans->debug(9, "Variable: " + var + @@ -611,27 +607,14 @@ std::vector> Rule::getFinalVars( } } if (ignoreVariable) { - if (v->m_dynamic) { delete v; v = NULL; - } continue; } - std::unique_ptr var(new collection::Variable( - new std::string(*v->m_key), - new std::string(*v->m_value))); - for (auto &i : v->m_orign) { - std::unique_ptr origin(new VariableOrigin()); - origin->m_offset = i->m_offset; - origin->m_length = i->m_length; - var->m_orign.push_back(std::move(origin)); - } - - if (v->m_dynamic) { - delete v; - v = NULL; - } + std::unique_ptr var(new collection::Variable(v)); + delete v; + v = NULL; finalVars.push_back(std::move(var)); } } @@ -772,8 +755,8 @@ bool Rule::evaluate(Transaction *trans, finalVars = getFinalVars(trans); for (auto &v : finalVars) { - const std::string value = *(v->m_value); - const std::string key = *(v->m_key); + const std::string value = v->m_value; + const std::string key = v->m_key; std::list, std::shared_ptr>> values; diff --git a/src/transaction.cc b/src/transaction.cc index 81ab4a10..e3e4f3a4 100644 --- a/src/transaction.cc +++ b/src/transaction.cc @@ -790,9 +790,10 @@ int Transaction::processRequestBody() { std::vector l; m_variableRequestHeaders.resolve(&l); for (auto &a : l) { - std::string z(*a->m_key, 16, a->m_key->length() - 16); - z = z + ": " + *a->m_value; + std::string z(a->m_key, 16, a->m_key.length() - 16); + z = z + ": " + a->m_value; fullRequest = fullRequest + z + "\n"; + delete a; } fullRequest = fullRequest + "\n\n"; @@ -1423,10 +1424,11 @@ std::string Transaction::toOldAuditLogFormat(int parts, audit_log << this->m_httpVersion << std::endl; m_variableRequestHeaders.resolve(&l); - for (auto h : l) { + for (auto &h : l) { size_t pos = strlen("REQUEST_HEADERS:"); - audit_log << h->m_key->c_str() + pos << ": "; - audit_log << h->m_value->c_str() << std::endl; + audit_log << h->m_key.c_str() + pos << ": "; + audit_log << h->m_value.c_str() << std::endl; + delete h; } audit_log << std::endl; } @@ -1459,10 +1461,11 @@ std::string Transaction::toOldAuditLogFormat(int parts, audit_log << "--" << trailer << "-" << "F--" << std::endl; audit_log << "HTTP/" << m_httpVersion << " " << this->m_httpCodeReturned << std::endl; m_variableResponseHeaders.resolve(&l); - for (auto h : l) { + for (auto &h : l) { size_t pos = strlen("RESPONSE_HEADERS:"); - audit_log << h->m_key->c_str() + pos << ": "; - audit_log << h->m_value->c_str() << std::endl; + audit_log << h->m_key.c_str() + pos << ": "; + audit_log << h->m_value.c_str() << std::endl; + delete h; } } audit_log << std::endl; @@ -1558,9 +1561,10 @@ std::string Transaction::toJSON(int parts) { yajl_gen_map_open(g); m_variableRequestHeaders.resolve(&l); - for (auto h : l) { + for (auto &h : l) { size_t pos = strlen("REQUEST_HEADERS:"); - LOGFY_ADD(h->m_key->c_str() + pos, h->m_value->c_str()); + LOGFY_ADD(h->m_key.c_str() + pos, h->m_value.c_str()); + delete h; } /* end: request headers */ @@ -1588,9 +1592,10 @@ std::string Transaction::toJSON(int parts) { yajl_gen_map_open(g); m_variableResponseHeaders.resolve(&l); - for (auto h : l) { + for (auto &h : l) { size_t pos = strlen("RESPONSE_HEADERS:"); - LOGFY_ADD(h->m_key->c_str() + pos, h->m_value->c_str()); + LOGFY_ADD(h->m_key.c_str() + pos, h->m_value.c_str()); + delete h; } /* end: response headers */ diff --git a/src/variables/remote_user.cc b/src/variables/remote_user.cc index db65be1d..ed502821 100644 --- a/src/variables/remote_user.cc +++ b/src/variables/remote_user.cc @@ -42,14 +42,16 @@ void RemoteUser::evaluate(Transaction *transaction, size_t pos; std::string base64; collection::Variable *var; + std::string header; - transaction->m_variableRequestHeaders.resolve("authorization", l); + std::vector *l2 = new std::vector(); + transaction->m_variableRequestHeaders.resolve("authorization", l2); - if (l->size() < 1) { - return; + if (l2->size() < 1) { + goto clear; } - std::string header(*l->at(0)->m_value); + header = std::string(l2->at(0)->m_value); if (header.compare(0, 6, "Basic ") == 0) { base64 = std::string(header, 6, header.length()); @@ -59,22 +61,27 @@ void RemoteUser::evaluate(Transaction *transaction, pos = base64.find(":"); if (pos == std::string::npos) { - return; + goto clear; } transaction->m_variableRemoteUser.assign(std::string(base64, 0, pos)); - var = new collection::Variable(l->at(0)->m_key, - std::make_shared(transaction->m_variableRemoteUser)); + var = new collection::Variable(&l2->at(0)->m_key, + &transaction->m_variableRemoteUser); - for (auto &i : l->at(0)->m_orign) { + for (auto &i : l2->at(0)->m_orign) { std::unique_ptr origin(new VariableOrigin()); origin->m_offset = i->m_offset; origin->m_length = i->m_length; var->m_orign.push_back(std::move(origin)); } - - l->clear(); l->push_back(var); + +clear: + for (auto &a : *l2) { + delete a; + } + l2->clear(); + delete l2; } diff --git a/src/variables/variable.h b/src/variables/variable.h index 0c5c135d..f8f1f369 100644 --- a/src/variables/variable.h +++ b/src/variables/variable.h @@ -126,24 +126,22 @@ class VariableModificatorCount : public Variable { Rule *rule, std::vector *l) { std::vector reslIn; - std::string *res = NULL; collection::Variable *val = NULL; int count = 0; m_var->evaluate(transaction, rule, &reslIn); for (const collection::Variable *a : reslIn) { count++; - if (a->m_dynamic) { - delete a; - a = NULL; - } + delete a; + a = NULL; } reslIn.clear(); - res = new std::string(std::to_string(count)); - - val = new collection::Variable(&m_name, res); - val->m_dynamic = true; + std::string *res = new std::string(std::to_string(count)); + std::string *name = new std::string(m_name); + val = new collection::Variable(name, res); + delete name; + delete res; l->push_back(val); return; diff --git a/src/variables/xml.cc b/src/variables/xml.cc index fbf269e1..5f17a319 100644 --- a/src/variables/xml.cc +++ b/src/variables/xml.cc @@ -125,8 +125,10 @@ void XML::evaluate(Transaction *t, content = reinterpret_cast( xmlNodeGetContent(nodes->nodeTab[i])); if (content != NULL) { + std::string *a = new std::string(content); collection::Variable *var = new collection::Variable(&m_name, - new std::string(content)); + a); + delete a; l->push_back(var); xmlFree(content); } diff --git a/src/variables/xml.h b/src/variables/xml.h index b2c67faf..bedab587 100644 --- a/src/variables/xml.h +++ b/src/variables/xml.h @@ -40,13 +40,12 @@ class XML_NoDictElement : public Variable { : Variable("XML"), m_plain("[XML document tree]"), m_var(&m_name, &m_plain) { - m_var.m_dynamic = false; } void evaluate(Transaction *transaction, Rule *rule, std::vector *l) override { - l->push_back(&m_var); + l->push_back(new collection::Variable(&m_var)); } std::string m_plain;