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.
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.
- 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.
|
@airween Do you guys have a release timeline for this feature? |
Yes: as soon as possible 😄. 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. |
|
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! |
- The shared files Windows implementation introduced in PR owasp-modsecurity#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 shared files Windows implementation introduced in PR owasp-modsecurity#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 shared files Windows implementation introduced in PR owasp-modsecurity#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 shared files Windows implementation introduced in PR owasp-modsecurity#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 shared files Windows implementation introduced in PR owasp-modsecurity#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)
- These were initially not included in these changes, as they were other PRs (owasp-modsecurity#3104 & owasp-modsecurity#3132) that address them.
- 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.
- The shared files Windows implementation introduced in PR owasp-modsecurity#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)
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.).