From 7ef0e4a0bd601426d9ff8775ed6cb548b8c91cae Mon Sep 17 00:00:00 2001 From: gberkes Date: Fri, 1 Mar 2024 08:49:36 +0100 Subject: [PATCH 01/14] Refactor: Ensure safe error handling by removing isolated `throw;` statements. - 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. --- headers/modsecurity/transaction.h | 10 ++++------ src/rule_with_actions.cc | 5 ++--- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/headers/modsecurity/transaction.h b/headers/modsecurity/transaction.h index b0dc4b7145..c6aeed6a9d 100644 --- a/headers/modsecurity/transaction.h +++ b/headers/modsecurity/transaction.h @@ -14,6 +14,7 @@ */ #ifdef __cplusplus +#include #include #include #include @@ -307,11 +308,8 @@ class TransactionSecMarkerManagement { } std::shared_ptr getCurrentMarker() const { - if (m_marker) { - return m_marker; - } else { - throw; - } + assert(m_marker != nullptr); + return m_marker; } void removeMarker() { @@ -323,7 +321,7 @@ class TransactionSecMarkerManagement { } private: - std::shared_ptr m_marker; + std::shared_ptr m_marker = nullptr; }; /** @ingroup ModSecurity_CPP_API */ diff --git a/src/rule_with_actions.cc b/src/rule_with_actions.cc index 04ee219c4b..4b6c8fef6d 100644 --- a/src/rule_with_actions.cc +++ b/src/rule_with_actions.cc @@ -17,6 +17,7 @@ #include +#include #include #include #include @@ -121,10 +122,8 @@ RuleWithActions::RuleWithActions( m_actionsRuntimePos.push_back(a); } } else { + assert(false); // Not implemented delete a; - std::cout << "General failure, action: " << a->m_name; - std::cout << " has an unknown type." << std::endl; - throw; } } delete actions; From c7896845831fb715aa1bfeea490998caf3ac8367 Mon Sep 17 00:00:00 2001 From: gberkes Date: Fri, 1 Mar 2024 14:19:57 +0100 Subject: [PATCH 02/14] Build System: Introduce Configurable Assertion Handling Implemented a new configuration option --enable-assertions=[yes|no] within config.ac, enabling controlled inclusion of -DNDEBUG in CPPFLAGS. The default setting suppresses assertions (by adding -DNDEBUG to CPPFLAGS), preserving the original behavior. This enhancement allows for the optional enabling of assertions during development or debugging by setting --enable-assertions=yes, thereby excluding -DNDEBUG from CPPFLAGS. --- configure.ac | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/configure.ac b/configure.ac index d8636036b9..ca92ef2e30 100644 --- a/configure.ac +++ b/configure.ac @@ -227,6 +227,19 @@ MSC_GIT_VERSION=msc_version_git AC_SUBST([MSC_GIT_VERSION]) + +AC_ARG_ENABLE(assertions, + [AS_HELP_STRING([--enable-assertions],[Turn on assertions feature: undefine NDEBUG])], + + [case "${enableval}" in + yes) assertions=true ;; + no) assertions=false ;; + *) AC_MSG_ERROR(bad value ${enableval} for --enable-asserions) ;; + esac], + + [assertions=false] + ) + AC_ARG_ENABLE(debug-logs, [AS_HELP_STRING([--disable-debug-logs],[Turn off the SecDebugLog feature])], @@ -355,6 +368,12 @@ if test "$aflFuzzer" == "true"; then GLOBAL_CPPFLAGS="$GLOBAL_CPPFLAGS $FUZZ_CPPCFLAGS" $buildExamples = false fi + +if test "$assertions" == "false"; then + ASSERTIONS_CPPCFLAGS="-DNDEBUG" + GLOBAL_CPPFLAGS="$GLOBAL_CPPFLAGS $ASSERTIONS_CPPCFLAGS" +fi + AC_SUBST(GLOBAL_LDADD) AC_SUBST(GLOBAL_CPPFLAGS) @@ -589,6 +608,14 @@ if test $buildTestUtilities = true; then else echo " + Test Utilities ....disabled" fi + + +if test $assertions = true; then + echo " + Assertions ....enabled" +else + echo " + Assertions ....disabled" +fi + if test $debugLogs = true; then echo " + SecDebugLog ....enabled" else From 285a687afba52b2eb8b3a4fcdbe09914a3daf0de Mon Sep 17 00:00:00 2001 From: gberkes Date: Fri, 1 Mar 2024 19:00:21 +0100 Subject: [PATCH 03/14] Making your assert statements more descriptive Source: https://www.learncpp.com/cpp-tutorial/assert-and-static_assert/ --- headers/modsecurity/transaction.h | 2 +- src/rule_with_actions.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/headers/modsecurity/transaction.h b/headers/modsecurity/transaction.h index c6aeed6a9d..831cf98f23 100644 --- a/headers/modsecurity/transaction.h +++ b/headers/modsecurity/transaction.h @@ -308,7 +308,7 @@ class TransactionSecMarkerManagement { } std::shared_ptr getCurrentMarker() const { - assert(m_marker != nullptr); + assert((m_marker != nullptr) && "You might have forgotten to call and evaluate isInsideAMarker() before calling getCurrentMarker()."); return m_marker; } diff --git a/src/rule_with_actions.cc b/src/rule_with_actions.cc index 4b6c8fef6d..4eac5f860d 100644 --- a/src/rule_with_actions.cc +++ b/src/rule_with_actions.cc @@ -122,7 +122,7 @@ RuleWithActions::RuleWithActions( m_actionsRuntimePos.push_back(a); } } else { - assert(false); // Not implemented + assert(false && "The handling of RunTimeBeforeMatchAttemptKind has not been implemented yet."); delete a; } } From b74cec0c813afc9a3287038104e6fb23c8aaefbb Mon Sep 17 00:00:00 2001 From: gberkes Date: Wed, 6 Mar 2024 11:13:25 +0100 Subject: [PATCH 04/14] Update README.md to recommend --enable-assertions=yes for debugging sessions. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 3be0881080..06afa4849d 100644 --- a/README.md +++ b/README.md @@ -231,7 +231,7 @@ CFLAGS to disable the compilation optimization parameters: ```shell $ export CFLAGS="-g -O0" $ ./build.sh -$ ./configure +$ ./configure --enable-assertions=yes $ make $ sudo make install ``` From e28ecb141463fe932a9d0ce4112956eee0fe7860 Mon Sep 17 00:00:00 2001 From: gberkes Date: Wed, 27 Mar 2024 14:42:00 +0100 Subject: [PATCH 05/14] Fix null pointer dereference error in 'src/rule_with_actions.cc' Resolved a null pointer dereference error identified by `make check-static` due to an outdated suppression in 'test/cppcheck_suppressions.txt'. The suppression at line 57 was incorrectly referencing 'src/rule_with_actions.cc:244'. Updated this to 'src/rule_with_actions.cc:243' to align with the current codebase. Additionally, removed suppressions for 'rethrowNoCurrentException' as identified by SonarCloud. This was possible due to the removal of standalone 'throw;' statements in the code. --- test/cppcheck_suppressions.txt | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/cppcheck_suppressions.txt b/test/cppcheck_suppressions.txt index ebbc665ee0..2cee41dbbd 100644 --- a/test/cppcheck_suppressions.txt +++ b/test/cppcheck_suppressions.txt @@ -52,9 +52,7 @@ noConstructor:src/variables/variable.h:152 danglingTempReference:src/modsecurity.cc:206 knownConditionTrueFalse:src/operators/validate_url_encoding.cc:77 knownConditionTrueFalse:src/operators/verify_svnr.cc:87 -rethrowNoCurrentException:headers/modsecurity/transaction.h:313 -rethrowNoCurrentException:src/rule_with_actions.cc:127 -ctunullpointer:src/rule_with_actions.cc:244 +ctunullpointer:src/rule_with_actions.cc:243 ctunullpointer:src/rule_with_operator.cc:135 ctunullpointer:src/rule_with_operator.cc:95 passedByValue:test/common/modsecurity_test.cc:49 From a35e2018c26e9cda0f9476e0fc846c1ceb6f1eb5 Mon Sep 17 00:00:00 2001 From: gberkes Date: Wed, 27 Mar 2024 15:36:46 +0100 Subject: [PATCH 06/14] Enable assertions in QA testing environment to improve error detection. --- .github/workflows/ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3350743b16..27e0828c82 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,7 +32,7 @@ jobs: - name: build.sh run: ./build.sh - name: configure ${{ matrix.configure.label }} - run: ./configure ${{ matrix.configure.opt }} + run: ./configure ${{ matrix.configure.opt }} --enable-assertions=yes - uses: ammaraskar/gcc-problem-matcher@master - name: make run: make -j `nproc` @@ -66,7 +66,7 @@ jobs: - name: build.sh run: ./build.sh - name: configure ${{ matrix.configure.label }} - run: ./configure ${{ matrix.configure.opt }} + run: ./configure ${{ matrix.configure.opt }} --enable-assertions=yes - uses: ammaraskar/gcc-problem-matcher@master - name: make run: make -j `sysctl -n hw.logicalcpu` From 0aa711ad288d1bd66a8f4c0c44c78d1384908668 Mon Sep 17 00:00:00 2001 From: gberkes Date: Thu, 25 Apr 2024 11:56:47 +0200 Subject: [PATCH 07/14] Explicitly use -UNDEBUG when --enable-assertions=yes is specified. --- configure.ac | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/configure.ac b/configure.ac index ca92ef2e30..06fd5e2dab 100644 --- a/configure.ac +++ b/configure.ac @@ -234,7 +234,7 @@ AC_ARG_ENABLE(assertions, [case "${enableval}" in yes) assertions=true ;; no) assertions=false ;; - *) AC_MSG_ERROR(bad value ${enableval} for --enable-asserions) ;; + *) AC_MSG_ERROR(bad value ${enableval} for --enable-assertions) ;; esac], [assertions=false] @@ -369,10 +369,12 @@ if test "$aflFuzzer" == "true"; then $buildExamples = false fi -if test "$assertions" == "false"; then - ASSERTIONS_CPPCFLAGS="-DNDEBUG" - GLOBAL_CPPFLAGS="$GLOBAL_CPPFLAGS $ASSERTIONS_CPPCFLAGS" -fi +case $assertions in + false) ASSERTIONS_CPPCFLAGS="-DNDEBUG" ;; + true) ASSERTIONS_CPPCFLAGS="-UNDEBUG" ;; + *) AC_MSG_ERROR(bad value ${assertions} for assertions) ;; +esac +GLOBAL_CPPFLAGS="$GLOBAL_CPPFLAGS $ASSERTIONS_CPPCFLAGS" AC_SUBST(GLOBAL_LDADD) AC_SUBST(GLOBAL_CPPFLAGS) From 2c8684eeb0e99b25a4d488d802910cba927153bb Mon Sep 17 00:00:00 2001 From: gberkes Date: Thu, 25 Apr 2024 12:43:44 +0200 Subject: [PATCH 08/14] Document importance of reviewing cppcheck_suppressions.txt during development and usage of assertions. --- README.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/README.md b/README.md index 06afa4849d..7315db844e 100644 --- a/README.md +++ b/README.md @@ -213,6 +213,12 @@ $ cd test $ ./regression-tests $ ./unit-tests ``` +Please take care that the '/test/cppcheck_suppressions.txt' +file contains hardcoded 'filename:line number' suppressions. If you modify a +file with explicit suppressions, you must update the 'cppcheck_suppressions.txt' +accordingly to ensure the suppression references remain aligned with the +intended lines of code. Failing to do so might trigger 'make check-static' to +break the build with errors that are logically unrelated to your modifications. ### Debugging @@ -235,7 +241,12 @@ $ ./configure --enable-assertions=yes $ make $ sudo make install ``` +"Assertions allow us to document assumptions and to spot violations early in the +development process. What is more, assertions allow us to spot violations with a +minimum of effort." https://dl.acm.org/doi/pdf/10.1145/240964.240969 +It is recommended to use assertions where applicable, and to enable them with +'--enable-assertions=yes' during the testing and debugging workflow. ## Reporting Issues From cee3965f689ea52004d0e8b160054e227e6c5593 Mon Sep 17 00:00:00 2001 From: gberkes Date: Thu, 25 Apr 2024 13:57:38 +0200 Subject: [PATCH 09/14] Remove unnecessary nullptr initialization from m_marker. --- headers/modsecurity/transaction.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/headers/modsecurity/transaction.h b/headers/modsecurity/transaction.h index 831cf98f23..a923ad6ac2 100644 --- a/headers/modsecurity/transaction.h +++ b/headers/modsecurity/transaction.h @@ -321,7 +321,7 @@ class TransactionSecMarkerManagement { } private: - std::shared_ptr m_marker = nullptr; + std::shared_ptr m_marker; }; /** @ingroup ModSecurity_CPP_API */ From d12376fc3d03cfb1b6c2a62c7eafe46702682344 Mon Sep 17 00:00:00 2001 From: gberkes Date: Thu, 25 Apr 2024 16:13:28 +0200 Subject: [PATCH 10/14] Refactor action_kind processing to use switch() instead of if-else chains; add assertion in default case. --- src/rule_with_actions.cc | 79 ++++++++++++++++++---------------- test/cppcheck_suppressions.txt | 2 +- 2 files changed, 44 insertions(+), 37 deletions(-) diff --git a/src/rule_with_actions.cc b/src/rule_with_actions.cc index 4eac5f860d..2e33156fdb 100644 --- a/src/rule_with_actions.cc +++ b/src/rule_with_actions.cc @@ -87,43 +87,50 @@ RuleWithActions::RuleWithActions( if (actions) { for (Action *a : *actions) { - if (a->action_kind == Action::ConfigurationKind) { - a->evaluate(this, NULL); - delete a; - - } else if (a->action_kind == Action::RunTimeOnlyIfMatchKind) { - if (dynamic_cast(a)) { - m_containsCaptureAction = true; - delete a; - } else if (dynamic_cast(a)) { - m_containsMultiMatchAction = true; + switch (a->action_kind) { + case Action::ConfigurationKind: + a->evaluate(this, NULL); delete a; - } else if (dynamic_cast(a)) { - m_severity = dynamic_cast(a); - } else if (dynamic_cast(a)) { - m_logData = dynamic_cast(a); - } else if (dynamic_cast(a)) { - m_msg = dynamic_cast(a); - } else if (dynamic_cast(a)) { - m_actionsSetVar.push_back( - dynamic_cast(a)); - } else if (dynamic_cast(a)) { - m_actionsTag.push_back(dynamic_cast(a)); - } else if (dynamic_cast(a)) { - m_actionsRuntimePos.push_back(a); - m_containsStaticBlockAction = true; - } else if (a->isDisruptive() == true) { - if (m_disruptiveAction != nullptr) { - delete m_disruptiveAction; - m_disruptiveAction = nullptr; + break; + case Action::RunTimeOnlyIfMatchKind: + if (dynamic_cast(a)) { + m_containsCaptureAction = true; + delete a; + } else if (dynamic_cast(a)) { + m_containsMultiMatchAction = true; + delete a; + } else if (dynamic_cast(a)) { + m_severity = dynamic_cast(a); + } else if (dynamic_cast(a)) { + m_logData = dynamic_cast(a); + } else if (dynamic_cast(a)) { + m_msg = dynamic_cast(a); + } else if (dynamic_cast(a)) { + m_actionsSetVar.push_back( + dynamic_cast(a)); + } else if (dynamic_cast(a)) { + m_actionsTag.push_back(dynamic_cast(a)); + } else if (dynamic_cast(a)) { + m_actionsRuntimePos.push_back(a); + m_containsStaticBlockAction = true; + } else if (a->isDisruptive() == true) { + if (m_disruptiveAction != nullptr) { + delete m_disruptiveAction; + m_disruptiveAction = nullptr; + } + m_disruptiveAction = a; + } else { + m_actionsRuntimePos.push_back(a); } - m_disruptiveAction = a; - } else { - m_actionsRuntimePos.push_back(a); - } - } else { - assert(false && "The handling of RunTimeBeforeMatchAttemptKind has not been implemented yet."); - delete a; + break; + default: + std::cout << "General failure, action: " << a->m_name; + std::cout << " has an unknown type." << std::endl; + delete a; + #ifdef NDEBUG + break; + #endif + assert(false); } } delete actions; @@ -240,7 +247,7 @@ void RuleWithActions::executeActionsAfterFullMatch(Transaction *trans, bool containsBlock, std::shared_ptr ruleMessage) { bool disruptiveAlreadyExecuted = false; - for (auto &a : trans->m_rules->m_defaultActions[getPhase()]) { + for (auto &a : trans->m_rules->m_defaultActions[getPhase()]) { // cppcheck_suppressions.txt:55 if (a.get()->action_kind != actions::Action::RunTimeOnlyIfMatchKind) { continue; } diff --git a/test/cppcheck_suppressions.txt b/test/cppcheck_suppressions.txt index 2cee41dbbd..cb0c213afc 100644 --- a/test/cppcheck_suppressions.txt +++ b/test/cppcheck_suppressions.txt @@ -52,7 +52,7 @@ noConstructor:src/variables/variable.h:152 danglingTempReference:src/modsecurity.cc:206 knownConditionTrueFalse:src/operators/validate_url_encoding.cc:77 knownConditionTrueFalse:src/operators/verify_svnr.cc:87 -ctunullpointer:src/rule_with_actions.cc:243 +ctunullpointer:src/rule_with_actions.cc:250 ctunullpointer:src/rule_with_operator.cc:135 ctunullpointer:src/rule_with_operator.cc:95 passedByValue:test/common/modsecurity_test.cc:49 From 3e7e5aa718cd77567dd28883540c8331f9d75d1a Mon Sep 17 00:00:00 2001 From: gberkes Date: Fri, 26 Apr 2024 17:04:01 +0200 Subject: [PATCH 11/14] Delete code breaking cppcheck. --- src/rule_with_actions.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/rule_with_actions.cc b/src/rule_with_actions.cc index 2e33156fdb..72002d3743 100644 --- a/src/rule_with_actions.cc +++ b/src/rule_with_actions.cc @@ -127,9 +127,6 @@ RuleWithActions::RuleWithActions( std::cout << "General failure, action: " << a->m_name; std::cout << " has an unknown type." << std::endl; delete a; - #ifdef NDEBUG - break; - #endif assert(false); } } From 6d90df2a5093ad2f809a8acb4010d4f2af58a55d Mon Sep 17 00:00:00 2001 From: gberkes Date: Fri, 26 Apr 2024 17:22:42 +0200 Subject: [PATCH 12/14] Align suppression line number. --- test/cppcheck_suppressions.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cppcheck_suppressions.txt b/test/cppcheck_suppressions.txt index cb0c213afc..42499d8a21 100644 --- a/test/cppcheck_suppressions.txt +++ b/test/cppcheck_suppressions.txt @@ -52,7 +52,7 @@ noConstructor:src/variables/variable.h:152 danglingTempReference:src/modsecurity.cc:206 knownConditionTrueFalse:src/operators/validate_url_encoding.cc:77 knownConditionTrueFalse:src/operators/verify_svnr.cc:87 -ctunullpointer:src/rule_with_actions.cc:250 +ctunullpointer:src/rule_with_actions.cc:247 ctunullpointer:src/rule_with_operator.cc:135 ctunullpointer:src/rule_with_operator.cc:95 passedByValue:test/common/modsecurity_test.cc:49 From 1a2bc0ffbe2e54b167e7508a87f94536b1f47a8a Mon Sep 17 00:00:00 2001 From: gberkes Date: Fri, 26 Apr 2024 18:11:43 +0200 Subject: [PATCH 13/14] Refactor to use cppcheck-friendly conditional inclusion of break/assert() statements. --- src/rule_with_actions.cc | 4 ++++ test/cppcheck_suppressions.txt | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/rule_with_actions.cc b/src/rule_with_actions.cc index 72002d3743..039f8519a1 100644 --- a/src/rule_with_actions.cc +++ b/src/rule_with_actions.cc @@ -127,7 +127,11 @@ RuleWithActions::RuleWithActions( std::cout << "General failure, action: " << a->m_name; std::cout << " has an unknown type." << std::endl; delete a; + #ifdef NDEBUG + break; + #else assert(false); + #endif } } delete actions; diff --git a/test/cppcheck_suppressions.txt b/test/cppcheck_suppressions.txt index 42499d8a21..51f4c9ef07 100644 --- a/test/cppcheck_suppressions.txt +++ b/test/cppcheck_suppressions.txt @@ -52,7 +52,7 @@ noConstructor:src/variables/variable.h:152 danglingTempReference:src/modsecurity.cc:206 knownConditionTrueFalse:src/operators/validate_url_encoding.cc:77 knownConditionTrueFalse:src/operators/verify_svnr.cc:87 -ctunullpointer:src/rule_with_actions.cc:247 +ctunullpointer:src/rule_with_actions.cc:251 ctunullpointer:src/rule_with_operator.cc:135 ctunullpointer:src/rule_with_operator.cc:95 passedByValue:test/common/modsecurity_test.cc:49 From c50e9844c0295d8f90da4ef030e3d61114baff96 Mon Sep 17 00:00:00 2001 From: gberkes Date: Fri, 26 Apr 2024 18:19:07 +0200 Subject: [PATCH 14/14] Fix SonarCloud issue: Make this variable a const reference. https://sonarcloud.io/project/issues?resolved=false&pullRequest=3104&id=owasp-modsecurity_ModSecurity&open=AY8Vpgy4f6U6E7VKL4Cn --- src/rule_with_actions.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rule_with_actions.cc b/src/rule_with_actions.cc index 039f8519a1..5084fa2361 100644 --- a/src/rule_with_actions.cc +++ b/src/rule_with_actions.cc @@ -248,7 +248,7 @@ void RuleWithActions::executeActionsAfterFullMatch(Transaction *trans, bool containsBlock, std::shared_ptr ruleMessage) { bool disruptiveAlreadyExecuted = false; - for (auto &a : trans->m_rules->m_defaultActions[getPhase()]) { // cppcheck_suppressions.txt:55 + for (const auto &a : trans->m_rules->m_defaultActions[getPhase()]) { // cppcheck_suppressions.txt:55 if (a.get()->action_kind != actions::Action::RunTimeOnlyIfMatchKind) { continue; }