From ce0d81c0dad1471658541fc47cbe4f6608f54b81 Mon Sep 17 00:00:00 2001 From: Felipe Zimmerle Date: Mon, 10 Aug 2015 00:08:02 -0300 Subject: [PATCH] Adds sanity check for inputs --- headers/modsecurity/assay.h | 2 +- headers/modsecurity/rules.h | 6 ++-- src/assay.cc | 59 +++++++++++++++++++++++++------------ src/audit_log.cc | 35 ++++++++++++++++++++-- src/audit_log.h | 29 ++---------------- 5 files changed, 81 insertions(+), 50 deletions(-) diff --git a/headers/modsecurity/assay.h b/headers/modsecurity/assay.h index 01a58fb7..553f8769 100644 --- a/headers/modsecurity/assay.h +++ b/headers/modsecurity/assay.h @@ -231,7 +231,7 @@ class Assay { std::string* resolve_variable_first(const std::string collectionName, std::string var); - void store_variable(std::string, const std::string &value); + void store_variable(std::string, std::string value); bool update_variable_first(std::string var, const std::string &value); void delete_variable(std::string key); diff --git a/headers/modsecurity/rules.h b/headers/modsecurity/rules.h index 0e495972..25a00201 100644 --- a/headers/modsecurity/rules.h +++ b/headers/modsecurity/rules.h @@ -50,7 +50,8 @@ class Driver; class Rules : public RulesProperties { public: Rules() - : RulesProperties(NULL), + : debugLog(NULL), + RulesProperties(NULL), unicode_codepage(0) { unicode_map_table = reinterpret_cast( malloc(sizeof(int)*65536)); @@ -58,7 +59,8 @@ class Rules : public RulesProperties { } explicit Rules(DebugLog *customLog) - : unicode_codepage(0), + : debugLog(NULL), + unicode_codepage(0), RulesProperties(customLog) { unicode_map_table = reinterpret_cast( malloc(sizeof(int)*65536)); diff --git a/src/assay.cc b/src/assay.cc index 5f58c62b..9a822ae1 100644 --- a/src/assay.cc +++ b/src/assay.cc @@ -156,6 +156,10 @@ Assay::~Assay() { * */ void Assay::debug(int level, std::string message) { + if (m_rules == NULL) { + return; + } + m_rules->debug(level, message); } @@ -233,18 +237,18 @@ int Assay::processURI(const char *uri, const char *protocol, m_uri = uri; m_uri_decoded = uri_decode(uri); - const char *pos = strchr(m_uri_decoded.c_str(), '?'); + size_t pos = m_uri_decoded.find("?"); store_variable("REQUEST_LINE", std::string(protocol) + " " + std::string(uri) + " HTTP/" + std::string(http_version)); std::string path_info; - if (pos == NULL) { + if (pos == std::string::npos) { path_info = std::string(m_uri_decoded, 0); } else { - path_info = std::string(m_uri_decoded, 0, - pos - m_uri_decoded.c_str()); - store_variable("QUERY_STRING", std::string(strchr(m_uri, '?'))); + path_info = std::string(m_uri_decoded, 0, pos); + store_variable("QUERY_STRING", std::string(m_uri_decoded, pos + 1, + m_uri_decoded.length() - (pos + 1))); } store_variable("PATH_INFO", path_info); store_variable("REQUEST_FILENAME", path_info); @@ -260,7 +264,7 @@ int Assay::processURI(const char *uri, const char *protocol, store_variable("REQUEST_URI", uri); store_variable("REQUEST_URI_RAW", uri); - if (pos != NULL && strlen(pos) > 2) { + if (pos != std::string::npos && (m_uri_decoded.length() - pos) > 2) { /** * FIXME: * @@ -269,10 +273,11 @@ int Assay::processURI(const char *uri, const char *protocol, * */ char sep1 = '&'; + std::string sets(m_uri_decoded, pos + 1, m_uri_decoded.length() - + (pos + 1)); + std::vector key_value_sets = split(sets, sep1); - std::vector key_value = split(pos+1, sep1); - - for (std::string t : key_value) { + for (std::string t : key_value_sets) { /** * FIXME: * @@ -282,27 +287,41 @@ int Assay::processURI(const char *uri, const char *protocol, char sep2 = '='; std::vector key_value = split(t, sep2); - store_variable("ARGS:" + key_value[0], key_value[1]); - store_variable("ARGS_GET:" + key_value[0], key_value[1]); + if (key_value.size() <= 1) { + /** TODO: Verify what ModSecurity 2.9.0 does when there is a + * key without an argument + */ + continue; + } + std::string key = key_value[0]; + std::string value = key_value[1]; + int i = key_value.size() - 1; + while (i > 2) { + value = value + sep2 + key_value[i]; + i--; + } + + store_variable("ARGS:" + key, value); + store_variable("ARGS_GET:" + key, value); if (m_namesArgs->empty()) { - m_namesArgs->assign(key_value[0]); + m_namesArgs->assign(key); } else { - m_namesArgs->assign(*m_namesArgs + " " + key_value[0]); + m_namesArgs->assign(*m_namesArgs + " " + key); } if (m_namesArgsGet->empty()) { - m_namesArgsGet->assign(key_value[0]); + m_namesArgsGet->assign(key); } else { - m_namesArgsGet->assign(*m_namesArgsGet + " " + key_value[0]); + m_namesArgsGet->assign(*m_namesArgsGet + " " + key); } this->m_ARGScombinedSize = this->m_ARGScombinedSize + \ - key_value[0].length() + key_value[1].length(); + key.length() + value.length(); this->m_ARGScombinedSizeStr->assign( std::to_string(this->m_ARGScombinedSize)); debug(4, "Adding request argument (QUERY_STRING): name \"" + \ - key_value[0] + "\", value \"" + key_value[1] + "\""); + key + "\", value \"" + value + "\""); } } return true; @@ -939,7 +958,9 @@ int Assay::processLogging(int returned_code) { this->m_rules->evaluate(ModSecurity::LoggingPhase, this); /* If relevant, save this assay information at the audit_logs */ - this->m_rules->audit_log->saveIfRelevant(this); + if (m_rules != NULL && m_rules->audit_log != NULL) { + this->m_rules->audit_log->saveIfRelevant(this); + } return 0; } @@ -1277,7 +1298,7 @@ std::string Assay::to_json(int parts) { } -void Assay::store_variable(std::string key, const std::string &value) { +void Assay::store_variable(std::string key, std::string value) { this->m_variables_strings.emplace(key, value); } diff --git a/src/audit_log.cc b/src/audit_log.cc index e669b647..46e212e7 100644 --- a/src/audit_log.cc +++ b/src/audit_log.cc @@ -33,11 +33,37 @@ namespace ModSecurity { +AuditLog::AuditLog() + : m_status(OffAuditLogStatus), + m_path1(""), + m_path2(""), + m_storage_dir(""), + m_parts(AAuditLogPart | BAuditLogPart | CAuditLogPart | FAuditLogPart + | HAuditLogPart | ZAuditLogPart), + m_type(ParallelAuditLogType), + m_writer(NULL), + m_relevant(""), + filePermission(0600), + directoryPermission(0600), + m_refereceCount(0) { } AuditLog::~AuditLog() { - m_writer->refCountDecreaseAndCheck(); + if (m_writer) { + m_writer->refCountDecreaseAndCheck(); + } } +void AuditLog::refCountIncrease() { + m_refereceCount++; +} + + +void AuditLog::refCountDecreaseAndCheck() { + m_refereceCount--; + if (m_refereceCount == 0) { + delete this; + } +} bool AuditLog::setStorageDirMode(int permission) { this->directoryPermission = permission; @@ -134,8 +160,13 @@ bool AuditLog::init() { bool AuditLog::isRelevant(int status) { std::string sstatus = std::to_string(status); + + if (m_relevant.empty()) { + return false; + } + return Utils::regex_search(sstatus, - Utils::Regex(this->m_relevant)) != 0; + Utils::Regex(m_relevant)) != 0; } diff --git a/src/audit_log.h b/src/audit_log.h index 74158c9e..d6703aaa 100644 --- a/src/audit_log.h +++ b/src/audit_log.h @@ -32,34 +32,11 @@ namespace ModSecurity { /** @ingroup ModSecurity_CPP_API */ class AuditLog { public: - AuditLog() - : m_status(OffAuditLogStatus), - m_path1(""), - m_path2(""), - m_storage_dir(""), - m_parts(AAuditLogPart | BAuditLogPart | CAuditLogPart | FAuditLogPart - | HAuditLogPart | ZAuditLogPart), - m_type(ParallelAuditLogType), - m_writer(NULL), - m_relevant(""), - filePermission(0600), - directoryPermission(0600), - m_refereceCount(0) - { } - + AuditLog(); ~AuditLog(); - void refCountIncrease() { - m_refereceCount++; - } - - - void refCountDecreaseAndCheck() { - m_refereceCount--; - if (m_refereceCount == 0) { - delete this; - } - } + void refCountIncrease(); + void refCountDecreaseAndCheck(); enum AuditLogType { SerialAuditLogType,