Add support to build libModSecurity v3 on Windows #3132
Conversation
|
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 Also you can take a look the SonarCloud issues, but I'm not sure those are really critical.
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. |
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.
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).
I've updated some of these today in commits 14ed9d9, 96ff573, 9cd9024 & 459c0c0. These are my notes after reviewing the rest of the issues:
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. |
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 Seems like these came with the new modifications.
Okay, thanks for clarification. We can mark that issue as false positive with this explanation.
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.
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.
Thank you, it seems very interesting (the custom web server). Thanks for all. |
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. |
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. |
I enabled actions in my fork so I could try this. I found out that the current configuration used to run cppcheck needs the
No warnings/error were reported on line 206 either. |
|
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: cmdIt'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). |
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 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). |
- These were initially not included in these changes, as they were other PRs (owasp-modsecurity#3104 & owasp-modsecurity#3132) that address them.
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! :-) |
|
Okay, so now please pick up the changes from current v3/master, and let's see what happens. |
eduar-hte
force-pushed
the
windows-port
branch
2 times, most recently
from
b6ed127
to
81a86d2
Compare
- These were initially not included in these changes, as they were other PRs (owasp-modsecurity#3104 & owasp-modsecurity#3132) that address them.
…ual C++) - most of posix related functions and constants in unistd.h can be found in io.h in Visual C++ - introduced src/compat/msvc.h to adjust for compiler differences (and avoid updating code with #ifdef blocks for Windows support) - removed some included headers that are not needed (both on Unix and Windows builds)
…td::this_thread::sleep_for & std::chrono::microseconds. - disabled build error from warning C4716 because process_request does not return a value and Visual C++ doesn't support [[noreturn]]
…usive code block) - updated included headers to support compilation on Windows (using Visual C++)
…us _open. - Updated included headers to support compilation on Windows (using Visual C++) - Minor change to use C++ default (zero) initialization instead of calling memset.
- Removed unused m_first data member. - Explicitly delete copy constructor and assignment operator. - Removed unused included headers.
… verification of https requests. - Updated included headers to support compilation on Windows (using Visual C++)
- expandEnv on Windows uses POCO C++ Libraries implementation of Glob - Paths of matched files are adjusted to preserve UNIX path separators for consistency with the rest of the code. - Minor change to code shared with other platforms that removes allocation of std::ifstream on the heap to check whether the file can be opened, which can be done with local stack variable that closes the file when out of scope. - createDir uses _mkdir on Windows, which doesn't support configuring the new directory's mode. - added public domain implementation of clock_gettime for clock_id CLOCK_PROCESS_CPUTIME_ID from mingw-w64's winpthreads to support cpu_seconds on Windows. - Updated included headers to support compilation on Windows (using Visual C++)
- 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.
- For OWASP v2 rules, switch to a v2 tag for the paths referenced in the rest of the script to apply.
…ce reference counting and remove unused code.
- 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.
…(MSVC compiler & CMake) and Conan package manager - Included Dockerfile to automate the setup process of prerequisites and build of libModSecurity binaries.
- 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.
- Addresses SonarCloud cpp:S3490 issue (Special member function should not be defined unless a non standard behavior is required)
- 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.
|
I added a new commit to set up a test suite using CTest for the Windows build. This only required a minor update to There's a new I've updated the GitHub workflow that builds libModSecurity on Windows to also run the same tests that
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 |
- Added new test/test_suite.in with list of regression and unit tests
previously in Makefile.am, to be shared between Unix and Windows
builds.
- Updated regression.cc & unit.cc to return the number of failed tests
to indicate to CTest that the test failed. Similarly, a crash or
unhandled exception terminates the process with a non-zero exit code.
- This change doesn't affect running the tests with autotest in Unix
builds because this processes test output from custom-test-driver &
test-suite.sh, and ignores the exit code of the test runner.
- Removed comment in test/test-cases/regression-offset-variable.json as
this is not supported by JSON and prevents strict parsers to read and
process the file.
- Minor change in regression.cc's clearAuditLog to replace std::ifstream
with std::ofstream as the mode to open the flag applies to an output
stream.
- Minor change in unit.cc to simplify code that deletes tests.
- Minor changes to test/custom-test-driver to correct usage information.
Thanks, let me check this in local - I'll try that soon.
Sorry, but I don't see any modification if
Right,
right,
okay,
right.
This is okay. 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 😃). |
- By default, all the 3rd party dependencies are enabled. - A dependency can be turned off by adding the "-DWITHOUT_xxx=ON" to the call of vcbuild.bat - List of 3rd party dependencies and associated option to turn them off: - LMDB: WITHOUT_LMDB - LUA: WITHOUT_LUA - LibXML2: WITHOUT_LIBXML2 - MaxMind: WITHOUT_MAXMIND - cURL: WITHOUT_CURL
|
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. |
yes, I saw it - it ran successfully.
really nice, thank you.
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. |
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). |
|
Merged, many thanks for this awesome work! |
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.
I'll try and take care of that too. |
- 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 owasp-modsecurity#3132 would probably be overwritten when the parser is updated.
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:
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
C:\TEMP\modsec.logneeds to be configured asc:/temp/modsec.log(casing can be different because Windows filename are case-insensitive).src/config.his generated with CMake using similar rules asconfigurefor non-Windows builds. This also sets upHAVE_xxxdefines (such asHAVE_LUA), though these seem not to be used in the codebase (similarWITH_xxxcompiler definitions are used instead, though these are not set insrc/config.h).Summary of changes
The following is a summary of the changes (more detail is available in each commit):
SharedFilesrequired 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::evaluateupdated to use_putenv_sinstead ofsetenv, not available in the MSVC C++ compiler.MultipartPartTmpFile::Openupdated usage ofmkstemp(not available in the MSVC C++ compiler) with_mktemp_sand_open.HttpsClient::downloadupdated to set CURL option to use the native CA store for certificate validation on Windows.expandEnv,createDir&cpu_seconds(fromsrc/utils/system.cc) updated to work on Windows.expandEnvon Windows uses Glob from the POCO C++ libraries.Env::evaluateupdated to handle the case that on Windows environment variables are case-insensitive.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:
.) would allow tests to work on Windows and non-Windows platforms./bin/echobinary not available on Windows. These tests run successfully using a Windows port of echo (see UnxUtils) and updating the rules to reference the path ofecho.exe13 tests are skipped due to the library not including support for GeoIP and ssdeep at this time (
variable-GEO.jsondepends on GeoIP, andoperator-fuzzyhash.jsondepends on ssdeep).The following is a list of the failed tests and analysis:
test-cases/regression/action-ctl_audit_engine.jsonauditengine : Config=Off, ctl:auditEngine=onSecAuditLog /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/tmpwith the local path.).test-cases/regression/action-exec.jsonTesting action :: exec (1/3)SecRule REQUEST_HEADERS \"@contains PHPSESSID\" \"id:1,t:lowercase,t:none,exec:/bin/echo\"looks for a file (invalid LUA script) in/bin/echowhich doesn't exist on Windows. The test passes if the path is updated in the rule and in theparser_errorline to a valid Windows file (such asc:/windows/system32/kernel32.dll).test-cases/regression/auditlog.jsonauditlog : messages verification - nolog,auditlogauditlog : multiMatch data, match after last transformauditlog : multiMatch data, match only after intermediate transformauditlog : rule chain, multiMatch data, match after last transformauditlog : rule chain, multiMatch data, match only after intermediate transformSecAuditLogwith a filename in the/tmp/testdirectory, which is not valid on Windows. The tests pass if the path is updated with a valid Windows path (for example, replace/tmp/testwith the local path.).test-cases/regression/issue-2000.jsonTesting audit log part H should output when deny - issue-2000SecAuditLog /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/testwith the local path.).test-cases/regression/issue-2423-msg-in-chain.jsonTest match variable (1/n)Test match variable (2/n)Test match variable (3/n)std::unordered_multimap(invoid AnchoredSetVariable::resolve(std::vector<const VariableValue *> *l, variables::KeyExclusions &ke)) which doesn't guarantee ordered iteration.test-cases/regression/issue-2427.jsonTmp file retained for @inspectFile (1/2)Tmp file retained for @inspectFile (2/2)SecUploadDir&SecTmpDirwith 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/tmpwith the local path.).test-cases/regression/operator-inpectFile.jsonTesting Operator :: @inspectFile (1/3)Testing Operator :: @inspectFile (2/3)Testing Operator :: @inspectFile (3/3)SecRule ARGS:res \"@inspectFile /bin/echo\" \"id:1,phase:2,pass,t:trim\"tries to execute the/bin/echobinary 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 ofecho.exe.test-cases/regression/offset-variable.jsonVariable offset - FILES_NAMESstd::unordered_multimap(invoid AnchoredSetVariable::resolve(std::vector<const VariableValue *> *l, variables::KeyExclusions &ke)) which doesn't guarantee ordered iteration.Variable offset - FILES_TMP_CONTENT 1Variable offset - FILES_TMP_CONTENT 2Variable offset - MULTIPART_FILENAMEVariable offset - MULTIPART_NAMESecUploadDirwith 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/tmpwith the local path.).test-cases/regression/request-body-parser-multipart.jsonmultipart parser (normal)SecUploadDirwith 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/tmpwith the local path.).