ModSecurity icon indicating copy to clipboard operation
ModSecurity copied to clipboard

Published 20 hours ago • verified publisher icon owasp-modsecurity

Add support to build libModSecurity v3 on Windows

Open eduar-hte opened this issue 1 year ago • 11 comments

This PR includes changes to support building libModSecurity v3 on Windows.

Work was initially done on a snapshot of v3.0.12 to work on the latest stable version of the library, and then changes were applied to v3/master up to the latest commit at the time the PR was created (commit 07e5a70).

The library builds with no warnings and was tested as a 64-bit DLL on Windows, running all unit & regression tests to validate the build. Additionally, the updated code was built on Linux, and unit & regression tests were executed on that version too. See below for more information on the results.

Building the library on Windows is done using MSVC C++ compiler, CMake and Conan package manager, so a new CMake build configuration was introduced for this platform. The following libModSecurity optional dependencies were integrated through available Conan package managers:

  • libxml2 2.12.6
  • libcurl 8.6.0
  • libmaxminddb 1.9.1
  • LUA 5.4.6
  • lmdb 0.9.31

These optional features can be enabled/disabled through flag variables in the CMake configuration file. The optional third-party dependencies GeoIP and ssdeep are not included in the port due to lack of Conan packages.

Additionally, a Windows Docker container configuration is included to simplify prerequisites setup and build of the library and associated binaries.

More information on building the library on Windows is available in build/win32/README.md.

Miscellaneous

  • In libModSecurity configuration files, paths on Windows still need to be specified using (forward) slashes. This avoids having to introduce changes to the parser.
    • For example, the path C:\TEMP\modsec.log needs to be configured as c:/temp/modsec.log (casing can be different because Windows filename are case-insensitive).
  • src/config.h is generated with CMake using similar rules as configure for non-Windows builds. This also sets up HAVE_xxx defines (such as HAVE_LUA), though these seem not to be used in the codebase (similar WITH_xxx compiler definitions are used instead, though these are not set in src/config.h).

Summary of changes

The following is a summary of the changes (more detail is available in each commit):

  • SharedFiles required updating file locking support to work on Windows. Additionally, a larger update was done on this class to remove unused shared memory code. See commit for detailed analysis of the rationale and changes.
  • SetENV::evaluate updated to use _putenv_s instead of setenv, not available in the MSVC C++ compiler.
  • MultipartPartTmpFile::Open updated usage of mkstemp (not available in the MSVC C++ compiler) with _mktemp_s and _open.
  • HttpsClient::download updated to set CURL option to use the native CA store for certificate validation on Windows.
  • expandEnv, createDir & cpu_seconds (from src/utils/system.cc) updated to work on Windows. expandEnv on Windows uses Glob from the POCO C++ libraries.
  • Env::evaluate updated to handle the case that on Windows environment variables are case-insensitive.
  • Fixed a use after free bug in ModSecurity::processContentOffset.

NOTE: The list does not include changes to files to update included header files to compile with the MSVC++ compiler on Windows and other minor changes.

Unit & regression test results

All changes to the latest version of the library on v3/master were built on Ubuntu 18.04 LTS and Ubuntu 22.04 LTS following the build instructions in compilation recipes, with equal results to the unmodified version of libModSecurity.

A 64-bit Windows build of the library obtains the same unit tests results as the Linux build. Regression test execution is also succesful, but 21 of the tests fail due to three different reasons:

  • Most of the failed tests reference paths not available on Windows (such as /tmp). These tests pass successfully if updated to reference a valid Windows path. Changing the path to the current directory (.) would allow tests to work on Windows and non-Windows platforms.
  • A few of the tests depend on the /bin/echo binary not available on Windows. These tests run successfully using a Windows port of echo (see UnxUtils) and updating the rules to reference the path of echo.exe
  • Two regression tests fail on the Windows build because the result does not match the expected one because the test depends on order not guaranteed by the underlying std C++ library used to store variables.

13 tests are skipped due to the library not including support for GeoIP and ssdeep at this time (variable-GEO.json depends on GeoIP, and operator-fuzzyhash.json depends on ssdeep).

The following is a list of the failed tests and analysis:

  • test-cases/regression/action-ctl_audit_engine.json
    • Test name: auditengine : Config=Off, ctl:auditEngine=on
      • The test configures SecAuditLog /tmp/modsec_test_ctl_auditengine_auditlog_1.log, which is not a valid Windows path. The test passes if the path is updated with a valid Windows path (for example, replace /tmp with the local path .).
  • test-cases/regression/action-exec.json
    • Test name: Testing action :: exec (1/3)
      • Rule SecRule REQUEST_HEADERS \"@contains PHPSESSID\" \"id:1,t:lowercase,t:none,exec:/bin/echo\" looks for a file (invalid LUA script) in /bin/echo which doesn't exist on Windows. The test passes if the path is updated in the rule and in the parser_error line to a valid Windows file (such as c:/windows/system32/kernel32.dll).
  • test-cases/regression/auditlog.json
    • Test name: auditlog : messages verification - nolog,auditlog
    • Test name: auditlog : multiMatch data, match after last transform
    • Test name: auditlog : multiMatch data, match only after intermediate transform
    • Test name: auditlog : rule chain, multiMatch data, match after last transform
    • Test name: auditlog : rule chain, multiMatch data, match only after intermediate transform
      • The tests configure SecAuditLog with a filename in the /tmp/test directory, which is not valid on Windows. The tests pass if the path is updated with a valid Windows path (for example, replace /tmp/test with the local path .).
  • test-cases/regression/issue-2000.json
    • Test name: Testing audit log part H should output when deny - issue-2000
      • The test configures SecAuditLog /tmp/test/modsec_audit.log, which is not a valid Windows path. The test passes if the path is updated with a valid Windows path (for example, replace /tmp/test with the local path .).
  • test-cases/regression/issue-2423-msg-in-chain.json
    • Test name: Test match variable (1/n)
    • Test name: Test match variable (2/n)
    • Test name: Test match variable (3/n)
      • Tests fails because order of matched variables is different in Windows build, due to iteration on underlying std::unordered_multimap (in void AnchoredSetVariable::resolve(std::vector<const VariableValue *> *l, variables::KeyExclusions &ke)) which doesn't guarantee ordered iteration.
  • test-cases/regression/issue-2427.json
    • Test name: Tmp file retained for @inspectFile (1/2)
    • Test name: Tmp file retained for @inspectFile (2/2)
      • The tests configure SecUploadDir & SecTmpDir with the path /tmp, which is not valid on Windows. The tests pass if the path is updated with a valid Windows path (for example, replace /tmp with the local path .).
  • test-cases/regression/operator-inpectFile.json
    • Test name: Testing Operator :: @inspectFile (1/3)
    • Test name: Testing Operator :: @inspectFile (2/3)
    • Test name: Testing Operator :: @inspectFile (3/3)
      • The rule SecRule ARGS:res \"@inspectFile /bin/echo\" \"id:1,phase:2,pass,t:trim\" tries to execute the /bin/echo binary which is not available on Windows. The tests run successfully using a Windows port of echo (see UnxUtils) and updating the rules to reference the path of echo.exe.
  • test-cases/regression/offset-variable.json
    • Test name: Variable offset - FILES_NAMES
      • Test fails because order of matched files is different in Windows build, due to iteration on underlying std::unordered_multimap (in void AnchoredSetVariable::resolve(std::vector<const VariableValue *> *l, variables::KeyExclusions &ke)) which doesn't guarantee ordered iteration.
    • Test name: Variable offset - FILES_TMP_CONTENT 1
    • Test name: Variable offset - FILES_TMP_CONTENT 2
    • Test name: Variable offset - MULTIPART_FILENAME
    • Test name: Variable offset - MULTIPART_NAME
      • The tests configure SecUploadDir with the path /tmp, which is not valid on Windows. The tests pass if the path is updated with a valid Windows path (for example, replace /tmp with the local path .).
  • test-cases/regression/request-body-parser-multipart.json
    • Test name: multipart parser (normal)
      • The test configure SecUploadDir with the path /tmp, which is not valid on Windows. The test passes if the path is updated with a valid Windows path (for example, replace /tmp with the local path .).

eduar-hte avatar Apr 25 '24 18:04 eduar-hte

Hi @eduar-hte,

first of all, let me congratulate you on this pull request: this is an awesome work.

Special thanks for detailed description you attached for the patch.

Although I'm sure the library has built in your environments (both on Linux and Windows), but as you can see there are some issues in our pipeline. Before we merge that, it's unavoidable to fix them.

The most critical issue is the make check-static result here. The fix is trivial, I think that would be enough to modify the test/cppcheck_suppressions.txt file, and change line number, because that line moved up by removing of these two lines.

Also you can take a look the SonarCloud issues, but I'm not sure those are really critical.

  • here is a Docker privileging problem; I'm not a Docker user, especially not on Windows, so please help me to decide is this a critical problem or not
  • the others are so called "reliability" issues, perhaps there are few (or all) false positives, but we also need to investigate them.

After merging this PR with the main codebase, we need to decide how to proceed. It seems like Nginx supported on Windows, but I don't know how do the modules operate on it. If the Nginx module architecture is also works on Windows, then we should make the ModSecurity-nginx connector.

What do you think?

We also welcome you on our Slack server in the channel #project-modsecurity, if you are able to join.

airween avatar Apr 26 '24 10:04 airween

The most critical issue is the make check-static result here. The fix is trivial, I think that would be enough to modify the test/cppcheck_suppressions.txt file, and change line number, because that line moved up by removing of these two lines.

I adjusted this in commit d19047a but I reviewed the rest of the files I updated in the PR to adjust other lines as well, see commit 3f9b38c. I think these builds need to be launched again to see if the latest update solves all the issues.

Also you can take a look the SonarCloud issues, but I'm not sure those are really critical.

  • here is a Docker privileging problem; I'm not a Docker user, especially not on Windows, so please help me to decide is this a critical problem or not

I don't think this needs to change, the recommendation is to run the container with low privileges (for example, if you're using the container to run a server). This container, however, is used just to build the library when the container image is built, so the container is not expected to run afterwards. Additionally, admin privileges are required during most of the container image generation, in order to install the prerequisites (MSVC C++ compiler, CMake, Conan package manager, GIT).

  • the others are so called "reliability" issues, perhaps there are few (or all) false positives, but we also need to investigate them.

I've updated some of these today in commits 14ed9d9, 96ff573, 9cd9024 & 459c0c0. These are my notes after reviewing the rest of the issues:

  • src/utils/msc_tree.cc: There are several reported issues in this file (about potential memory leak, null pointer dereference, removing unused variables, adding const to some function parameters, etc.) I only updated this file in commit 2207a92 to adjust the included header files to compile with MSVC, so I don't think these issues apply to this PR.
  • src/utils/string.cc: I just removed unused header files. I'd rather not introduce changes unrelated to this PR.
  • Recommends using fopen_s on src/utils/shared_files.cc & src/utils/system.cc, which is MSVC C++ compiler only so I think should be ignored.
  • MultipartPartTmpFile::Open() (in src/request_body_processor/multipart.cc):
    • There is a recommendation to replace a C-style array with std::string. I actually moved the declaration of this variable (tstr) and introduced default initialization to remove a call to memset. I could rollback my change, if necessary. I'd rather not change the call to strftime where the array is used.
    • Recommends merging an if statement with an enclosing one. This is how the unmodified code is written, and now this is part of an #ifdef block for non-Windows and Windows code, so actually having two statements prevents duplicating code.
  • There is a reported issue about nesting levels in expandEnv (in src/utils/system.cc). I introduced code in this function to implement Glob matching using the implementation from POCO C++ libraries, but I adhered to the existing code flow, which used these nesting levels (for the original implementation and a port to OpenBSD).
  • shared_files.h: There is a recommendation to "use the transparent equality "std::equal_to<>" and a custom transparent heterogeneous hasher with this associative string container". However, the interface of SharedFiles guarantees that all lookups and insertions on this map are done using std::string instances so adding this capability would be redundant.

After merging this PR with the main codebase, we need to decide how to proceed. It seems like Nginx supported on Windows, but I don't know how do the modules operate on it. If the Nginx module architecture is also works on Windows, then we should make the ModSecurity-nginx connector.

We're integrating libModSecurity on a custom web server on Windows. I've only looked at the Apache & Nginx ModSecurity connectors as a reference on how to use the library. At some point I saw that Nginx should compile on Windows as well, but I didn't try it.

eduar-hte avatar Apr 26 '24 17:04 eduar-hte

The most critical issue is the make check-static result here.

I adjusted this in commit d19047a but I reviewed the rest of the files I updated in the PR to adjust other lines as well, see commit 3f9b38c. I think these builds need to be launched again to see if the latest update solves all the issues.

Thanks!

Because this is your first issue, the CI workflow does not start automatically when you update the PR, so you weren't able to see that there is another check-static issue:

2024-04-27T07:09:25.0572337Z Checking src/modsecurity.cc ...
2024-04-27T07:09:25.0573014Z Defines:MS_CPPCHECK_DISABLED_FOR_PARSER=1
2024-04-27T07:09:25.0573902Z Undefines: MBEDTLS_MD5_ALT; MBEDTLS_SHA1_ALT; YYSTYPE; YY_USER_INIT
2024-04-27T07:09:25.0575313Z Includes: -Iheaders/ -I./ -Iothers/ -Isrc/ -Iothers/mbedtls/ -Isrc/parser/
2024-04-27T07:09:25.0576153Z Platform:Native
2024-04-27T07:09:25.0922287Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1...
2024-04-27T07:09:26.5722454Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;AIX...
2024-04-27T07:09:28.0162535Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;DRAGONFLY...
2024-04-27T07:09:29.4353636Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;FREEBSD...
2024-04-27T07:09:30.8117204Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;HPUX...
2024-04-27T07:09:32.2319299Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;LINUX...
2024-04-27T07:09:33.6444501Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;MACOSX...
2024-04-27T07:09:35.0453940Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;MSC_WITH_CURL...
2024-04-27T07:09:36.4378081Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;NETBSD...
2024-04-27T07:09:37.8554729Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;NO_LOGS...
2024-04-27T07:09:39.3005270Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;OPENBSD...
2024-04-27T07:09:40.7184693Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;SOLARIS...
2024-04-27T07:09:42.1278565Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;WIN32...
2024-04-27T07:09:43.5504218Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;WITH_GEOIP...
2024-04-27T07:09:44.9903719Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;WITH_LIBXML2...
2024-04-27T07:09:47.2117050Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;WITH_MAXMIND...
2024-04-27T07:09:48.6488191Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;WITH_PCRE2...
2024-04-27T07:09:50.1179638Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;WITH_YAJL...
2024-04-27T07:09:51.5599001Z warning: src/modsecurity.cc,265,performance,redundantCopyLocalConst,The const variable 'startingAt' is assigned a copy of the data. You can avoid the unnecessary data copying by converting 'startingAt' to const reference.
2024-04-27T07:09:51.5602109Z warning: src/modsecurity.cc,267,performance,redundantCopyLocalConst,The const variable 'size' is assigned a copy of the data. You can avoid the unnecessary data copying by converting 'size' to const reference.
2024-04-27T07:09:51.5604892Z warning: src/modsecurity.cc,349,performance,redundantCopyLocalConst,The const variable 'startingAt' is assigned a copy of the data. You can avoid the unnecessary data copying by converting 'startingAt' to const reference.
2024-04-27T07:09:51.5607899Z warning: src/modsecurity.cc,351,performance,redundantCopyLocalConst,The const variable 'size' is assigned a copy of the data. You can avoid the unnecessary data copying by converting 'size' to const reference.
2024-04-27T07:09:51.5798124Z 118/218 files checked 45% done

Seems like these came with the new modifications.

  • here is a Docker privileging problem; I'm not a Docker user, especially not on Windows, so please help me to decide is this a critical problem or not

I don't think this needs to change, the recommendation is to run the container with low privileges (for example, if you're using the container to run a server). This container, however, is used just to build the library when the container image is built, so the container is not expected to run afterwards. Additionally, admin privileges are required during most of the container image generation, in order to install the prerequisites (MSVC C++ compiler, CMake, Conan package manager, GIT).

Okay, thanks for clarification. We can mark that issue as false positive with this explanation.

  • the others are so called "reliability" issues, perhaps there are few (or all) false positives, but we also need to investigate them.

I've updated some of these today in commits 14ed9d9, 96ff573, 9cd9024 & 459c0c0. These are my notes after reviewing the rest of the issues:

* `src/utils/msc_tree.cc`: There are several reported issues in this file (about potential memory leak, null pointer dereference, removing unused variables, adding const to some function parameters, etc.) I only updated this file in commit [2207a92](https://github.com/owasp-modsecurity/ModSecurity/commit/2207a92da7605ac04347bb20e97c45c8441ce71a) to adjust the included header files to compile with MSVC, so I don't think these issues apply to this PR.

thanks - not just for this but the others too.

Unfortunately - as you wrote too - not all commits apply to this PR. I mean some modifications makes library to build on Windows, but some other are code cleaning - which is really appreciated and very useful.

* `src/utils/string.cc`: I just removed unused header files. I'd rather not introduce changes unrelated to this PR.

Same as above: if I'm not wrong, this is a code cleaning.

I think it's important to split up the two things: feature request and clean up. Both are really important and very welcome, but when someone will inspect the code later it's hard to decide why the author sent his feature request PR with unnecessary modifications.

Because you also mentioned this above, I really hope you understand this.

After merging this PR with the main codebase, we need to decide how to proceed. It seems like Nginx supported on Windows, but I don't know how do the modules operate on it. If the Nginx module architecture is also works on Windows, then we should make the ModSecurity-nginx connector.

We're integrating libModSecurity on a custom web server on Windows. I've only looked at the Apache & Nginx ModSecurity connectors as a reference on how to use the library. At some point I saw that Nginx should compile on Windows as well, but I didn't try it.

Thank you, it seems very interesting (the custom web server).

Thanks for all.

airween avatar Apr 27 '24 07:04 airween

Because this is your first issue, the CI workflow does not start automatically when you update the PR, so you weren't able to see that there is another check-static issue:

2024-04-27T07:09:50.1179638Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;WITH_YAJL...
2024-04-27T07:09:51.5599001Z warning: src/modsecurity.cc,265,performance,redundantCopyLocalConst,The const variable 'startingAt' is assigned a copy of the data. You can avoid the unnecessary data copying by converting 'startingAt' to const reference.
2024-04-27T07:09:51.5602109Z warning: src/modsecurity.cc,267,performance,redundantCopyLocalConst,The const variable 'size' is assigned a copy of the data. You can avoid the unnecessary data copying by converting 'size' to const reference.
2024-04-27T07:09:51.5604892Z warning: src/modsecurity.cc,349,performance,redundantCopyLocalConst,The const variable 'startingAt' is assigned a copy of the data. You can avoid the unnecessary data copying by converting 'startingAt' to const reference.
2024-04-27T07:09:51.5607899Z warning: src/modsecurity.cc,351,performance,redundantCopyLocalConst,The const variable 'size' is assigned a copy of the data. You can avoid the unnecessary data copying by converting 'size' to const reference.
2024-04-27T07:09:51.5798124Z 118/218 files checked 45% done

Seems like these came with the new modifications.

I think these need a suppression too, because the copy is needed to avoid the use after free that I fixed in commit 7cb67b0 (and which seems to have been introduced in ad28de4). The problem is that the current code is creating a reference to the last element in the list (lines 265 & 267) but then the last element is immediately removed (lines 266 & 268), which means that you have a dangling reference. Then the references are accessed in lines 273-274 & 278-279 which is incorrect. In this case, if you heed the warning, you're actually introducing a bug! :-)

(Lines 349 & 351 didn't have the problem but I marked the variables as const because they were not modified after assignment, and for consistency with the code in lines 265 & 267)

Can I add these suppressions in the code itself? If that works with your setup, I think it'd improve maintainability of the project, as you wouldn't need to update test/cppcheck_suppressions.txt when line numbers change because of unrelated updates.

eduar-hte avatar Apr 27 '24 15:04 eduar-hte

Can I add these suppressions in the code itself? If that works with your setup, I think it'd improve maintainability of the project, as you wouldn't need to update test/cppcheck_suppressions.txt when line numbers change because of unrelated updates.

I tried this in commit cc24a4f. Let's see if it works and what you think.

PS: I checked if by adding these lines I needed to update the line numbers of other suppressions on the same file and I think the ones about line 206 may not be necessary (I'll revert commit 30e1cd8 if I'm mistaken). Additionally, I don't think an r-value reference is necessary in the previous line, as the compiler will perform a return value optimization (RVO) to avoid a copy.

eduar-hte avatar Apr 27 '24 21:04 eduar-hte

I tried this in commit cc24a4f. Let's see if it works and what you think.

I enabled actions in my fork so I could try this. I found out that the current configuration used to run cppcheck needs the --inline-suppr argument for the inline suppressions to work. I've added that configuration in commit e9540ea. Once again, I hope you're ok with this.

PS: I checked if by adding these lines I needed to update the line numbers of other suppressions on the same file and I think the ones about line 206 may not be necessary (I'll revert commit 30e1cd8 if I'm mistaken). Additionally, I don't think an r-value reference is necessary in the previous line, as the compiler will perform a return value optimization (RVO) to avoid a copy.

No warnings/error were reported on line 206 either.

eduar-hte avatar Apr 28 '24 02:04 eduar-hte

I also tried out a workflow to build the Windows version of the library adding the following configuration in a branch in my fork:

  build-windows:
    runs-on: ${{ matrix.os }}
    strategy:
      matrix:
        os: [windows-2022]
        platform: [x86, x86_64]
    steps:
      - uses: actions/checkout@v4
        with:
          submodules: true
      - name: Install Conan
        run: |
          pip3 install conan --upgrade
          conan profile detect
      - uses: ammaraskar/msvc-problem-matcher@master
      - name: vcbuild ${{ matrix.platform }}
        run: vcbuild.bat Release ${{ matrix.platform }}
        shell: cmd

It's simpler than the Dockerfile I added as a reference!

The 64-bit build takes less than 5 minutes, which is similar to the build time of the Linux/macOS versions. However, the 32-bit version took around 33 minutes to build. This is because the Conan dependencies need to be built in this configuration (while the 64-bit build can use the compiled binaries available in the package).

eduar-hte avatar Apr 28 '24 03:04 eduar-hte

I enabled actions in my fork so I could try this. I found out that the current configuration used to run cppcheck needs the --inline-suppr argument for the inline suppressions to work. I've added that configuration in commit e9540ea. Once again, I hope you're ok with this.

I ended up looking at inlining cppcheck suppressions a bit more and created a new PR (#3134) to remove those that reference line numbers so it hopefully streamlines the process of submitting changes (by avoiding the build errors and the need for additional commits to update test/cppcheck_suppressions.txt).

I've seen this recently also in PR #3104, where guidance is being introduced to check if this file needs updating after making changes to the library (see commit 2c8684e).

eduar-hte avatar Apr 28 '24 20:04 eduar-hte

After merging this PR with the main codebase, we need to decide how to proceed. It seems like Nginx supported on Windows, but I don't know how do the modules operate on it. If the Nginx module architecture is also works on Windows, then we should make the ModSecurity-nginx connector.

Though this was unrelated to the work we're doing with libModSecurity v3, I ended up taking a stab at building the ModSecurity-nginx module and have just created ModSecurity-nginx PR #321 for that! :-)

eduar-hte avatar May 01 '24 00:05 eduar-hte

Okay, so now please pick up the changes from current v3/master, and let's see what happens.

airween avatar May 03 '24 12:05 airween

I started to review this PR, there are one relevant request change. I try to continue it in next days, I would like to review shared_files.cc and system.cc modifications. The other looks good to me.

airween avatar May 07 '24 20:05 airween

I started to review this PR, there are one relevant request change. I try to continue it in next days, I would like to review shared_files.cc and system.cc modifications. The other looks good to me.

system.c changes are simple, in WIN32 blocks and reference third-party implementations of glob and clock_gettime (which I initially didn't implement, as the macOS version doesn't currently implement this, but found out the public domain implementation from mingw-w64's winpthreads and decided to include it).

shared_files.* is more involved. I mean, the implementation in the PR is actually pretty simple and straightforward, but you need to go through all the history referenced in the commit message to analyze how the removed code was both dead-code and also had several minor issues introduced over time. I hope I was able to explain that well.

eduar-hte avatar May 08 '24 01:05 eduar-hte

system.c changes are simple, in WIN32 blocks and reference third-party implementations of glob and clock_gettime (which I initially didn't implement, as the macOS version doesn't currently implement this, but found out the public domain implementation from mingw-w64's winpthreads and decided to include it).

yepp, thanks, everything looks good.

shared_files.* is more involved. I mean, the implementation in the PR is actually pretty simple and straightforward, but you need to go through all the history referenced in the commit message to analyze how the removed code was both dead-code and also had several minor issues introduced over time. I hope I was able to explain that well.

yes, that's very impressive fix - thanks.

Let's finish the modsecurity.cc affected conversation above, and I'll merge this.

airween avatar May 08 '24 19:05 airween

I added a new commit to set up a test suite using CTest for the Windows build.

This only required a minor update to unit.cc & regression.cc to have them set their exit code to the number of tests that failed, so that CTest can use this to determine whether the test failed or not. This doesn't affect the Unix execution of the tests because those determine whether a test failed by analyzing their console output (processed through the shell scripts custom-test-driver & test-suite.sh used to run them).

There's a new /test/test-suite.in with the list of tests currently in Makefile.am because I wanted to share the definition of the tests between the Unix and Windows builds. I don't know much about autoconf/automake so I'm just doing a simple include of the file from Makefile.am. That means that each line of the file has a TESTS+= prefix (necessary for Makefile.am), and not just the test filename, and I have to ignore that prefix when processing the file on the Windows build to setup the CTest test suite. I couldn't figure out how to generate the TESTS variable from a file with just a list of filenames with autoconf/automake.

I've updated the GitHub workflow that builds libModSecurity on Windows to also run the same tests that make check do on the Unix builds. There are a couple of steps that handle the issues I found when running the current tests on Windows:

  • One step creates a tmp directory at the root of the drive where the build and tests are done, which makes those tests that configure SecAuditLog work.
  • This step also creates a bin directory at the root of the drive too, and then copies echo.exe from the usr/bin tools included with Git in the windows-2022 runner image to that directory, which allows tests that run /bin/echo to work.
    • echo.exe is also copied as \bin\echo for the test that checks if the file /bin/echo exist.
  • The other step (Disable tests that don't work on Windows) sets the enabled property to zero in the JSON tests that fail on Windows because the tests relies (incorrectly) on the order on which a std::unordered_map is iterated.
    • I had to update the offset-variable.json to remove a couple of comments in the JSON file, as this is not JSON compliant, and breaks the processing of the file to turn the test off.
build-windows:
    runs-on: ${{ matrix.os }}
    strategy:
      matrix:
        os: [windows-2022]
        platform: [x86_64]
        configuration: [Release]
    steps:
      - uses: actions/checkout@v4
        with:
          submodules: true
      - name: Install Conan
        run: |
          pip3 install conan --upgrade
          conan profile detect
      - uses: ammaraskar/msvc-problem-matcher@master
      - name: Build ${{ matrix.configuration }} ${{ matrix.platform }}
        shell: cmd
        run: vcbuild.bat ${{ matrix.configuration }} ${{ matrix.platform }}
      - name: Set up test environment
        working-directory: build\win32\build\${{ matrix.configuration }}
        env:
          BASE_DIR: ..\..\..\..
        shell: cmd
        run: |
          copy unit_tests.exe %BASE_DIR%\test
          copy regression_tests.exe %BASE_DIR%\test
          copy libModSecurity.dll %BASE_DIR%\test
          copy %BASE_DIR%\unicode.mapping %BASE_DIR%\test
          md \tmp
          md \bin
          copy "C:\Program Files\Git\usr\bin\echo.exe" \bin
          copy "C:\Program Files\Git\usr\bin\echo.exe" \bin\echo
      - name: Disable tests that don't work on Windows
        working-directory: test\test-cases\regression
        shell: cmd
        run: |
          jq "map(if .title == \"Test match variable (1/n)\" then .enabled = 0 else . end)" issue-2423-msg-in-chain.json > tmp.json && move /Y tmp.json issue-2423-msg-in-chain.json
          jq "map(if .title == \"Test match variable (2/n)\" then .enabled = 0 else . end)" issue-2423-msg-in-chain.json > tmp.json && move /Y tmp.json issue-2423-msg-in-chain.json
          jq "map(if .title == \"Test match variable (3/n)\" then .enabled = 0 else . end)" issue-2423-msg-in-chain.json > tmp.json && move /Y tmp.json issue-2423-msg-in-chain.json
          jq "map(if .title == \"Variable offset - FILES_NAMES\" then .enabled = 0 else . end)" offset-variable.json > tmp.json && move /Y tmp.json offset-variable.json
      - name: Run tests
        working-directory: build\win32\build
        run: |
          ctest -C ${{ matrix.configuration }} --output-on-failure

eduar-hte avatar May 12 '24 23:05 eduar-hte

I added a new commit to set up a test suite using CTest for the Windows build.

This only required a minor update to unit.cc & regression.cc to have them set their exit code to the number of tests that failed, so that CTest can use this to determine whether the test failed or not. This doesn't affect the Unix execution of the tests because those determine whether a test failed by analyzing their console output (processed through the shell scripts custom-test-driver & test-suite.sh used to run them).

There's a new /test/test-suite.in with the list of tests currently in Makefile.am because I wanted to share the definition of the tests between the Unix and Windows builds. I don't know much about autoconf/automake so I'm just doing a simple include of the file from Makefile.am. That means that each line of the file has a TESTS+= prefix (necessary for Makefile.am), and not just the test filename, and I have to ignore that prefix when processing the file on the Windows build to setup the CTest test suite. I couldn't figure out how to generate the TESTS variable from a file with just a list of filenames with autoconf/automake.

Thanks, let me check this in local - I'll try that soon.

I've updated the GitHub workflow that builds libModSecurity on Windows to also run the same tests that make check do on the Unix builds. There are a couple of steps that handle the issues I found when running the current tests on Windows:

Sorry, but I don't see any modification if .github directory at all - see the Files changed tab. Beside that I think if a new architecture appears in workflow, then we must see that in between tests results. But I also don't see even between your tests.

* One step creates a `tmp` directory at the root of the drive where the build and tests are done, which makes those tests that configure `SecAuditLog` work.

Right,

* This step also creates a `bin` directory at the root of the drive too, and then copies `echo.exe` from the `usr/bin` tools included with Git in the `windows-2022` runner image to that directory, which allows tests that run `/bin/echo` to work.

right,

  * `echo.exe` is also copied as `\bin\echo` for the test that checks if the file `/bin/echo` exist.

okay,

* The other step (`Disable tests that don't work on Windows`) sets the `enabled` property to zero in the JSON tests that fail on Windows because the tests relies (incorrectly) on the order on which a `std::unordered_map` is iterated.

right.

  * I had to update the `offset-variable.json` to remove a couple of comments in the JSON file, as this is not JSON compliant, and breaks the processing of the file to turn the test off.

This is okay.

Thanks - it would be very to see the Windows test in the Github workflow now.

airween avatar May 13 '24 19:05 airween

Thanks - it would be very to see the Windows test in the Github workflow now.

oh, I've never submitted committed the changes to the workflow in my PR because I thought it'd be restricted. I'll try that now and let you know.

eduar-hte avatar May 13 '24 20:05 eduar-hte

Thanks - it would be very to see the Windows test in the Github workflow now.

oh, I've never submitted committed the changes to the workflow in my PR because I thought it'd be restricted. I'll try that now and let you know.

No, there is no any restriction.

If you send the commit, GH will start the workflow at your branch separately. You can wait for the finish, if you want to check (but the workflow will start on main repository immediately, because the PR is active).

This is the goal (and a big advantage :smiley:).

airween avatar May 13 '24 20:05 airween

Quality Gate Failed Quality Gate failed

Failed conditions
E Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

sonarqubecloud[bot] avatar May 14 '24 21:05 sonarqubecloud[bot]

If you send the commit, GH will start the workflow at your branch separately. You can wait for the finish, if you want to check (but the workflow will start on main repository immediately, because the PR is active).

I added the Windows build yesterday.

I've made an update to also support building libModSecurity on Windows without some of the third party dependencies (LMDB, LUA, LibXML2, MaxMind & cURL), and have now configured builds without each of them.

NOTE: There is not a build in the GH workflow without LibXML2 (though the Windows build configuration now supports it) because the regression tests assume that XML support is always on. I guess that's the reason why this is not turned off either for Linux/macOS builds.

eduar-hte avatar May 14 '24 22:05 eduar-hte

I added the Windows build yesterday.

yes, I saw it - it ran successfully.

I've made an update to also support building libModSecurity on Windows without some of the third party dependencies (LMDB, LUA, LibXML2, MaxMind & cURL), and have now configured builds without each of them.

really nice, thank you.

NOTE: There is not a build in the GH workflow without LibXML2 (though the Windows build configuration now supports it) because the regression tests assume that XML support is always on. I guess that's the reason why this is not turned off either for Linux/macOS builds.

Yes, thank you for spotting that. That makes sense (because XML parser is a really important component of the library), but I don't understand why is there a test without YAJL. Never mind.

Thank you for this huge work, now everything is fine for me.

Please let me know if you finished the work or you still want to add something - I'm ready to merge this PR.

airween avatar May 15 '24 09:05 airween

Thank you for this huge work, now everything is fine for me.

Please let me know if you finished the work or you still want to add something - I'm ready to merge this PR.

That's great to hear. I'm not working on any further changes on the Windows port now, so I think you should just go ahead and integrate it (and then I can update the ModSecurity-nginx PR to sync with this -it currently references my repository in order to build libModSecurity on Windows).

eduar-hte avatar May 15 '24 12:05 eduar-hte

Merged, many thanks for this awesome work!

airween avatar May 15 '24 13:05 airween

NOTE: There is not a build in the GH workflow without LibXML2 (though the Windows build configuration now supports it) because the regression tests assume that XML support is always on. I guess that's the reason why this is not turned off either for Linux/macOS builds.

I'll create a quick PR to annotate the regression tests that depend on libxml and have them skipped if the build doesn't include that feature.

Then I'll add the Windows build configuration to build without libxml.

Yes, thank you for spotting that. That makes sense (because XML parser is a really important component of the library), but I don't understand why is there a test without YAJL. Never mind.

I'll try and take care of that too.

eduar-hte avatar May 15 '24 14:05 eduar-hte

@airween Do you guys have a release timeline for this feature?

varkey98 avatar Jul 10 '24 07:07 varkey98

Do you guys have a release timeline for this feature?

Yes: as soon as possible :smile:.

Seriously: the plan was that we make releases in last month, but there are few pending PR's which I would like to review and merge. Unfortunately I had no time to do this, but I hope I can continue the reviewing soon.

Note to everyone: I would like to improve the QA of the projects, eg. add more steps in CI/CD in case of mod_security2 and ModSecurity-nginx repositories. Last month I started to work on a special rule set. The aim is to cover all ModSecurity's target with the rules and make tests for them; tests can run with go-ftw.

I don't want to wait until we finish that rule set, but with help we can progress with it, so please anyone who knows how ModSecurity works and able to write rules, take a look to MRTS. You should take few finished data files like ARGS or ARGS_GET, and I'm sure that will be easy to write data files for other target's.

airween avatar Jul 10 '24 12:07 airween

Thanks for the reply @airween. I'd love to help out as well, but I'm a noob when it comes to Modsec rules, will try to learn more about it in the coming days :)

varkey98 avatar Jul 10 '24 12:07 varkey98

Thanks for the reply @airween. I'd love to help out as well, but I'm a noob when it comes to Modsec rules, will try to learn more about it in the coming days :)

That would be awesome! You can reach us on OWASP's Slack server, on #project-modsecurity channel - feel free to ask anything!

airween avatar Jul 10 '24 12:07 airween