From 8acd9eda8c436755a0ecc8026c4f3f7ad82d40b2 Mon Sep 17 00:00:00 2001 From: Alexey Zelkin Date: Thu, 19 Jan 2017 12:07:11 +0000 Subject: [PATCH 1/2] - ensure that file descriptor is closed on all error cases - check fopen() return against NULL --- src/debug_log/debug_log_writer.cc | 5 +++-- src/utils/shared_files.cc | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/debug_log/debug_log_writer.cc b/src/debug_log/debug_log_writer.cc index 5ddf8c449..2edc0d360 100644 --- a/src/debug_log/debug_log_writer.cc +++ b/src/debug_log/debug_log_writer.cc @@ -55,7 +55,7 @@ debug_log_file_handler_t *DebugLogWriter::add_new_handler( FILE *fp; fp = fopen(fileName.c_str(), "a"); - if (fp == 0) { + if (fp == NULL) { error->assign("Failed to open file: " + fileName); goto err_fh; } @@ -134,14 +134,15 @@ debug_log_file_handler_t *DebugLogWriter::add_new_handler( } return new_debug_log; + err_shmget2: err_shmat2: shmdt(shm_ptr2); - fclose(new_debug_log->fp); err_shmget1: err_shmat1: shmdt(new_debug_log); err_mem_key: + fclose(fp); err_fh: return NULL; } diff --git a/src/utils/shared_files.cc b/src/utils/shared_files.cc index ef66aaa53..3cde78336 100644 --- a/src/utils/shared_files.cc +++ b/src/utils/shared_files.cc @@ -60,7 +60,7 @@ msc_file_handler_t *SharedFiles::add_new_handler( FILE *fp; fp = fopen(fileName.c_str(), "a"); - if (fp == 0) { + if (fp == NULL) { error->assign("Failed to open file: " + fileName); goto err_fh; } @@ -139,14 +139,15 @@ msc_file_handler_t *SharedFiles::add_new_handler( } return new_debug_log; + err_shmget2: err_shmat2: shmdt(shm_ptr2); - fclose(new_debug_log->fp); err_shmget1: err_shmat1: shmdt(new_debug_log); err_mem_key: + fclose(fp); err_fh: return NULL; } From b26d6e5f553f303d3086c5976614055dfdfc6e1a Mon Sep 17 00:00:00 2001 From: Alexey Zelkin Date: Thu, 19 Jan 2017 12:39:06 +0000 Subject: [PATCH 2/2] Fix cases when shared memory allocated by first instance of application being overwritten by another instance attaching same shared memory segments. It happens in case when 'nginx' server is running, but configuration is updated and 'nginx -t' is called in order to update configuration. Shared memory segment gets overriten by second instance of nginx, and any call to LogFile/SharedFile::find_handler() will cause crash. With this fix, every initialized block is being marked with feature specific signature assuming that block in use already, and can't be re-used yet. For second and later application instances yet another ftok() code is adviced to use (via recursive call to add_new_handler()). There's a side effect possible when nginx (or other user of libmodsecurity) gets crashed, shared memory block can be left in place with valid signature stored in there. For this case it's advised to use ipcs(1)/ipcrm(1) to cleanup stale blocks. --- src/debug_log/debug_log_writer.cc | 32 +++++++++++++++++++++++++++---- src/debug_log/debug_log_writer.h | 3 ++- src/utils/shared_files.cc | 27 ++++++++++++++++++++++---- src/utils/shared_files.h | 3 ++- 4 files changed, 55 insertions(+), 10 deletions(-) diff --git a/src/debug_log/debug_log_writer.cc b/src/debug_log/debug_log_writer.cc index 2edc0d360..5c4bcbcfe 100644 --- a/src/debug_log/debug_log_writer.cc +++ b/src/debug_log/debug_log_writer.cc @@ -27,6 +27,8 @@ #include +#define MODSEC_SM_LOG_SIGNATURE "m0d$log" + namespace modsecurity { namespace debug_log { @@ -45,29 +47,35 @@ debug_log_file_handler_t *DebugLogWriter::find_handler( debug_log_file_handler_t *DebugLogWriter::add_new_handler( - const std::string &fileName, std::string *error) { + const std::string &fileName, std::string *error, int pinc) { debug_log_file_handler_t *current = m_first; int shm_id; key_t mem_key_structure; key_t mem_key_file_name; debug_log_file_handler_t *new_debug_log; char *shm_ptr2; + static char memsign[] = MODSEC_SM_LOG_SIGNATURE; FILE *fp; + /* recursion protection */ + if (pinc > 10) { + return NULL; + } + fp = fopen(fileName.c_str(), "a"); if (fp == NULL) { error->assign("Failed to open file: " + fileName); goto err_fh; } - mem_key_structure = ftok(fileName.c_str(), 1); + mem_key_structure = ftok(fileName.c_str(), pinc); if (mem_key_structure < 0) { error->assign("Failed to select key for the shared memory (1): "); error->append(strerror(errno)); goto err_mem_key; } - mem_key_file_name = ftok(fileName.c_str(), 2); + mem_key_file_name = ftok(fileName.c_str(), pinc+1); if (mem_key_file_name < 0) { error->assign("Failed to select key for the shared memory (2): "); error->append(strerror(errno)); @@ -89,6 +97,19 @@ debug_log_file_handler_t *DebugLogWriter::add_new_handler( error->append(strerror(errno)); goto err_shmat1; } + + /* there's a crash scenario when add_new_handler() initializes shared memory block, + * and later another copy of application reinitializes it (repointing new_debug_log->file_name + * to new process' local memory. it causes first application instance to crash on any + * find_handler() invocation. Use-case is 'nginx -t' / 'nginx -s reload'. + */ + if (memcmp(new_debug_log->signature, memsign, strlen(memsign)) == 0) { + /* if shared memory block is in use, try allocating with another project_id in ftok() */ + fclose(fp); + shmdt(new_debug_log); + return add_new_handler(fileName, error, pinc+2); + } + memset(new_debug_log, '\0', sizeof(debug_log_file_handler_t)); pthread_mutex_init(&new_debug_log->lock, NULL); @@ -116,6 +137,9 @@ debug_log_file_handler_t *DebugLogWriter::add_new_handler( new_debug_log->file_name = shm_ptr2; + /* populate 'shared block in-use' signature */ + memcpy(new_debug_log->signature, memsign, sizeof(new_debug_log->signature)); + if (m_first == NULL) { m_first = new_debug_log; } else { @@ -151,7 +175,7 @@ debug_log_file_handler_t *DebugLogWriter::add_new_handler( int DebugLogWriter::open(const std::string& fileName, std::string *error) { debug_log_file_handler_t *a = find_handler(fileName); if (a == NULL) { - a = add_new_handler(fileName, error); + a = add_new_handler(fileName, error, 1); if (error->size() > 0) { return -1; } diff --git a/src/debug_log/debug_log_writer.h b/src/debug_log/debug_log_writer.h index ed8b0cda2..3d242ef2c 100644 --- a/src/debug_log/debug_log_writer.h +++ b/src/debug_log/debug_log_writer.h @@ -33,6 +33,7 @@ namespace debug_log { typedef struct debug_log_file_handler { + char signature[8]; char *file_name; FILE *fp; int file_handler; @@ -60,7 +61,7 @@ class DebugLogWriter { protected: debug_log_file_handler_t *find_handler(const std::string &fileName); debug_log_file_handler_t *add_new_handler(const std::string &fileName, - std::string *error); + std::string *error, int pinc); private: DebugLogWriter() : m_first(NULL) { } diff --git a/src/utils/shared_files.cc b/src/utils/shared_files.cc index 3cde78336..f2667527a 100644 --- a/src/utils/shared_files.cc +++ b/src/utils/shared_files.cc @@ -34,6 +34,7 @@ namespace modsecurity { namespace utils { +#define MODSEC_SM_SHF_SIGNATURE "m0d$shf" msc_file_handler_t *SharedFiles::find_handler( const std::string &fileName) { @@ -50,29 +51,35 @@ msc_file_handler_t *SharedFiles::find_handler( msc_file_handler_t *SharedFiles::add_new_handler( - const std::string &fileName, std::string *error) { + const std::string &fileName, std::string *error, int pinc) { msc_file_handler_t *current = m_first; int shm_id; key_t mem_key_structure; key_t mem_key_file_name; msc_file_handler_t *new_debug_log; char *shm_ptr2; + char memsign[] = MODSEC_SM_SHF_SIGNATURE; FILE *fp; + if (pinc > 10) { + /* recursion protection */ + return NULL; + } + fp = fopen(fileName.c_str(), "a"); if (fp == NULL) { error->assign("Failed to open file: " + fileName); goto err_fh; } - mem_key_structure = ftok(fileName.c_str(), 1); + mem_key_structure = ftok(fileName.c_str(), pinc); if (mem_key_structure < 0) { error->assign("Failed to select key for the shared memory (1): "); error->append(strerror(errno)); goto err_mem_key; } - mem_key_file_name = ftok(fileName.c_str(), 2); + mem_key_file_name = ftok(fileName.c_str(), pinc+1); if (mem_key_file_name < 0) { error->assign("Failed to select key for the shared memory (2): "); error->append(strerror(errno)); @@ -94,6 +101,15 @@ msc_file_handler_t *SharedFiles::add_new_handler( error->append(strerror(errno)); goto err_shmat1; } + + /* there's a crash scenario when add_new_handler(), described in src/debug_log/debug_log_writer.cc */ + if (memcmp(new_debug_log->signature, memsign, strlen(memsign)) == 0) { + /* if shared memory block is in use, try allocating with another project_id in ftok() */ + fclose(fp); + shmdt(new_debug_log); + return add_new_handler(fileName, error, pinc+2); + } + memset(new_debug_log, '\0', sizeof(msc_file_handler_t)); pthread_mutex_init(&new_debug_log->lock, NULL); @@ -121,6 +137,9 @@ msc_file_handler_t *SharedFiles::add_new_handler( new_debug_log->file_name = shm_ptr2; + /* populate 'shared block in-use' signature */ + memcpy(new_debug_log->signature, memsign, sizeof(new_debug_log->signature)); + if (m_first == NULL) { m_first = new_debug_log; } else { @@ -156,7 +175,7 @@ msc_file_handler_t *SharedFiles::add_new_handler( bool SharedFiles::open(const std::string& fileName, std::string *error) { msc_file_handler_t *a = find_handler(fileName); if (a == NULL) { - a = add_new_handler(fileName, error); + a = add_new_handler(fileName, error, 1); if (error->size() > 0) { return false; } diff --git a/src/utils/shared_files.h b/src/utils/shared_files.h index 7d71089ca..dfd9c79d9 100644 --- a/src/utils/shared_files.h +++ b/src/utils/shared_files.h @@ -33,6 +33,7 @@ namespace utils { typedef struct msc_file_handler { + char signature[8]; char *file_name; FILE *fp; int file_handler; @@ -59,7 +60,7 @@ class SharedFiles { protected: msc_file_handler_t *find_handler(const std::string &fileName); msc_file_handler_t *add_new_handler(const std::string &fileName, - std::string *error); + std::string *error, int pinc); private: SharedFiles() : m_first(NULL) { }