- Renamed Transformation::evaluate to Transformation::transform to avoid
confusion with Action's overload methods.
- Updated Transformation::transform signature to receive the value by
reference and perform the transformation inline, if possible.
- Some transformations still need to use a temporary std::string to
perform their work, and then copy the result back.
- Made Transformation::transform methods const and updated Transaction
parameter to be const.
- Transaction parameter could not be removed because it's used by just
a single transformation, UrlDecodeUni.
- Removed std::string Action::evaluate(const std::string &exp,
Transaction *transaction); which was only implemented by
Transformation but was not used from the base class, but only after
downcasting to Transformation, so it can just be declared there (and
not pollute other actions with a default member implementation -that
does nothing- which is never called).
- Leverage std::size to determine buffer size at compile time.
- Simplified 'TimeMon::evaluate' implementation as it was using strftime
to get the month, convert the string to int, and then decrement it by
one to make it zero based. This same value is already available in
the 'struct tm' previously generated with the call to localtime_r (and
where the month is already zero-based)
- std::ctime returns a pointer to a string that "may be shared between
std::asctime and std::ctime, and may be overwritten on each invocation
of any of those functions.".
- https://en.cppreference.com/w/cpp/chrono/c/ctime
- Replaced with call to strftime to generate the same string
representation (using the format string: %c)
- Leveraged localtime_r (which is thread-safe) to convert time_t to
struct tm, as required by strftime.
- This was introduced in commit 119a6fc & 7d786b3 because of a potential
issue reported in #1573.
- The ACMP tree structure is initialized when the operator is
initialized.
- During transaction execution the ACMP tree structure is only 'read'
while traversing the tree (in acmp_process_quick) so this is safe for
use in a multi-threaded environment.
- Replaced pthread_mutex_t in modsecurity::operators::Pm with std::mutex
- Replaced pthread's thread usage in reading_logs_via_rule_message
example with std::thread.
- Simplified and modernized C++ code.
- Removed unnecessary includes of pthread.h
- As reported in #3054, the resolve methods in InMemoryPerProcess are
not acquiring a lock/mutex to prevent concurrent access to the data
structures that may be modified at the same time from other threads,
and thus triggering undefined behaviour.
- Replace inheritance of std::unordered_multimap data structure with
data member to prevent potential clients to use it without acquiring
the mutex to protect concurrent access.
- Replace pthreads lock with std C++11 std::shared_mutex
- Provides exclusive/shared lock access so that multiple readers can
access the data at the same time, but only one writer. this is used
to favor query performance by allowing more concurrent access to the
data until an update needs to be performed.
- Simplifies acquisition and disposal of lock/mutex with
std::lock_guard, which has RAII semantics.
- NOTE: Because std::shared_mutex is not recursive, calls to another
function that tries to acquire the lock will fail. Introduced
__store & __updateFirst helper methods to workaround this.
- Updates to InMemoryPerProcess::resolveFirst
- Updated the code to store the expired var in 'expiredVars' to delete
them after iterating over the range (and releasing the read lock, as
'delIfExpired' needs to acquire it for exclusive access), as the
current call to 'delIfExpired' would invalidate the range triggering
undefined behaviour on the following iteration.
- Noticed that in commit 118e1b3 the call to 'delIfExpired' in this
function is done using 'it->second.getValue()'' instead of
'it->first', which seems incorrect (based on similar code in other
resolveXXX functions).
- Updated InMemoryPerProcess::delIfExpired to use 'std::find_if' (with a
lambda that matches both the key and the 'isExpired' condition)
because the data structure is a multimap. The version introduced in
commit 118e1b3 could find an entry (not necessarily the first, because
the map is unordered) where 'isExpired' is 'false' and exit, while
another entry could be expired.
- cppreference mentions this about the constructor that receives a
const char *:
- Constructs the string with the contents initialized with a copy of
the null-terminated character string pointed to by s. The length of
the string is determined by the first null character. The behavior
is undefined if [s, s + Traits::length(s)) is not a valid range
(for example, if s is a null pointer).
- C++23 introduces a deleted constructor to prevent this in static
scenarios, which is how this issue was detected.
C++17 introduced a construct to create and initialize a variable within the
condition of if and switch statements, and C++20 added this construct to
range-based for loops. Using this new feature simplifies common code patterns
and helps in giving variables the right scope.
Reference: https://sonarcloud.io/project/issues?open=AZDCieK2zGtqRpL2rnl-&id=owasp-modsecurity_ModSecurity
- Initialize `log` temporary value on construction instead of doing
default initialization and then calling `append`.
- Leverage `std::string_view` to replace `const std::string&` parameters
in `utils::string::replaceAll` to avoid creating a `std::string`
object (and associated allocation and copy) for the string literal`%d`
- Keep m_it->disruptive value and use it as return value to guarantee
that the value is correct.
- If m_it->disruptive is false and the 'it' argument has not been
initialized/cleaned, the function may incorrectly return a non-zero
value.
- When a disruptive intervention is being reported by the function,
defensively initialize log & url to NULL if there's no such data to
provide to the caller.
- If the caller has not initialized/cleaned those fields in the 'it'
argument, after returning from transaction::intervention, the user
can safely read the log & url fields and in all scenarios they'll
have valid values.
- The shared files Windows implementation introduced in PR #3132 works
in multi-process single-threaded contexts but it doesn't work
correctly in single-process multi-threaded contexts.
- The issue is that the LockFileEx Win32 function works on a per-handle
basis.
- In a multi-process context, each process will have called
SharedFiles::add_new_handler when initializing the SharedFile and
obtained a handle, and thus locking will work.
- When running ModSecurity in a single process using multiple threads,
the initialization of the SharedFile will happen once and the handle
will be shared by all threads. Then, if two threads try to write to
the same shared file concurrently, they may deadlock as one of them
will lock the file (by calling LockFileEx) and then proceed to write
to the file. If before writing to the file and unlocking it, another
thread calls LockFileEx on the same handle, the attempt to write to
the file will lock generating a deadlock.
- The new implementation replaces usage of LockFileEx/UnlockFileEx with
a named mutex to lock access to the shared file.
- A named mutex is used to support multi-process scenarios.
- The mutex name is generated using the filename to support multiple
shared files (such as that for the debug and audit logs).
- This assumes that both process will initialize the SharedFile
instance using the same filename (which is expected as they'd be
using the same configuration file)
- The following methods are introduced to allow clients of
libModSecurity that are not able to link and call the C/C++ standard
library to be able to free the buffers allocated by libModSecurity.
- msc_intervention_cleanup: Frees the buffers in a
ModSecurityIntervention structure that have been allocated by calls to
msc_intervention.
- msc_rules_error_cleanup: Frees an error message buffer allocated by
the msc_rules_xxx functions to detail the condition that triggered
the error.
- SonarCloud analysis identified standalone `throw;` calls without accompanying `try-catch` blocks, used inconsistently as placeholders or for premature termination under specific conditions.
- Removed these `throw;` instances to prevent potential runtime issues in future development phases, where such configurations might inadvertently be created.
- Introduced `assert` statements as a more appropriate mechanism for asserting preconditions in the affected class member functions, ensuring clearer intent and safer code behavior during development.
- Refactor action_kind processing to use switch() instead of if-else chains; add assertion in default case.
- Fix SonarCloud issue: Make this variable a const reference.
https://sonarcloud.io/project/issues?resolved=false&pullRequest=3104&id=owasp-modsecurity_ModSecurity&open=AY8Vpgy4f6U6E7VKL4Cn
- The previous approach would create a std::unique_ptr and store it in
a std::list in VariableValue (Origins)
- The new approach now stores Origins in a std::vector and constructs
VariableOrigin elements in-place on insertion.
- Instead of having two heap-allocations for every added VariableOrigin
instance, this performs only one.
- If multiple origins are added, std::vector's growth strategy may even
prevent a heap-allocation. There's a cost on growing the size of the
vector, because a copy of current elements will be necessary.
- Introduced reserveOrigin method to notify that multiple insertions
will be made, so that we can use std::vector's reserve and do a
single allocation (and copy of previous elements), and then just
initialize the new elements in-place.
- The parser is not used interactively so we can avoid including
unistd.h, which is not available on Windows MSVC C++ compiler.
- The #ifdef WIN32 introduced in PR #3132 would probably be overwritten
when the parser is updated.
- Addresses SonarCloud issue cpp:S5025 (Memory should not be managed manually)
- This function was not changed for the Windows port, but a similar
change to the one suggested was done in expandEnv in the same file.
- The first stream is not destructed at the exact same point it was in
the previous code (but rather when the second stream replaces it on
assignment to the same variable). An arbitrary scope could have been
introduced to destruct the object at the same place, but it doesn't
seem to be necessary and would make the code a bit strange.
- Address SonarCloud cpp:S3806 issues ("#include" paths should be portable)
- This is not an actual issue in this case, because WinSock2.h and
WS2tcpip.h are Windows only.
- In Windows build, replaced usage of fcntl with cmd F_SETLKW with
Win32 APIs to do file locking (LockFileEx & UnlockFileEx).
- Reintroduced the reference counting initially present in the class
which is necessary to correctly handle merging of rules. This allows
for correctly closing the file and removing the associated entry from
m_handlers when the file is no longer used.
- The need for reference counting can be seen in the example
simple_example_using_c, where rules are initially loaded locally and
then further rules are loaded remotely. This will initially open a
shared file for a log, then in order to merge rules, the shared file
is opened again for the new configuration. Then, the previous
configuration closes the shared file on destruction. That is, two
consecutive opens are done on a shared file, which is followed by
a close. If the shared file is not reference counted, the shared file
will be closed while there is still a reference active. The current
version works because closing of the file has been disabled after
reference counting was removed.
- Replaced `std::vector` data structure with `std::unordered_map` to
improve lookup/update times, and simplify code.
- Removed unused code
- Shared memory to store msc_file_handler structure
- Initially SharedFiles used shared memory to store information
about each shared file, including its file pointer and a mutex to
synchronize access to the file on write. See code at commit 01c13da,
in particular, usage of lock & fp fields in the msc_file_handler_t
structure.
- At that time, msc_file_handler_t included reference counting too
with the using_it field, which was incremented when a file was
opened and decremented on close. If the reference count reached
zero, the shared file would be closed, the lock destroyed and the
file handler entry removed from m_handlers.
- Reference counting was removed in commit 7f9cd76, which
introduced the following issues in SharedFiles::close:
- No longer closes the file pointer.
- The file pointer appears to be reset when a.second = 0, but
this is a local copy of the data pair obtained from m_handlers,
so this is essentially a nop (updating a local variable that is
not referenced later in the function).
- NOTE: The file pointer was moved out of the shared memory in
this commit too, and stored alongside the msc_file_handler_t
instance in the m_handlers entry associated to the shared file.
- The lock is no longer destroyed.
- The shared memory is marked to be destroyed in the call to:
shmctl(a.first->shm_id_structure, IPC_RMID, NULL);
- The shared file entry is not removed from m_handlers, so:
- the file pointer is still valid, which is how writing to the
file continues to work,
- the reference to the shared memory is also present and will
be marked to be destroyed whenever close is called again on the
shared file.
- File locking using the mutex in msc_file_handler_t was replaced in
commit 3d20304 with usage of fcntl with cmd F_SETLKW.
- At this time, it appears that the shared memory is no longer used,
as the file pointer and locking no longer depend on it.
- MODSEC_USE_GENERAL_LOCK
- This code is introduced commit 7f9cd76 and is enabled if
MODSEC_USE_GENERAL_LOCK` is defined.
- The define is commented out in the source code since the original
commit and is not present in the build configuration either.
- In commit ff9152e, in the SharedFiles constructor, the
initialization of the local variable toBeCreated is removed. This
means that in this version, if MODSEC_USE_GENERAL_LOCK is enabled,
execution of the code that checks on toBeCreated is undefined.
- Then, in commit 9b40a04, the variable toBeCreated is initialized
again, but is now set to false, which means that if
MODSEC_USE_GENERAL_LOCK is enabled, the shared memory and lock it
uses will *not* be initialized and thus doesn't work (execution of
the current version will result in trying to acquire a lock that
will be null).
- I conclude that the feature is not used and can be removed.
- Additionally, if it were working, I think the lock should be
used in SharedFiles::write as well, which is a reader of the
underlying data structures protected by this lock when they're
modified in SharedFiles::open & SharedFiles::close.
- Env::evaluate
- Environment variable names in Windows are case-insensitive, so in
the Windows build we use strcasecmp to ignore case when matching
variables in transaction->m_variableEnvs.
- If the variable is found, we use the expected variable name to
create the VariableValue instance, as further rule processing will
look for the variable using case-sensitive comparisons.
- This code is not limited to Windows to avoid another #ifdef block
because for other platforms, because the env variable names are
case-sensitive the value from either x.first and m_name will be the
same.
- In Windows build, avoid redefining environ, already defined by
including stdlib.h.
- Use after free issue detected with Address Sanitizer while running
the reading_logs_with_offset example.
- Keeps reference to last element in vars vector with vars.back(). Then
it removes the element from vars calling vars.pop_back() which
invalidates the reference, but it's accessed later in the function.
- RuleWithActions::evaluate(Transaction *transaction)
- Removed temporary rm local variable used to immediately create
std::shared_ptr<RuleMessage>.
- Leverage std::make_shared & auto to simplify code.
