search

owasp-modsecurity / ModSecurity

ModSecurity is an open source, cross platform web application firewall (WAF) engine for Apache, IIS and Nginx. It has a robust event-based programming language which provides protection from a range of attacks against web applications and allows for HTTP traffic monitoring, logging and real-time analysis.
https://www.modsecurity.org
Apache License 2.0
8.11k stars 1.59k forks source link

SecRequestBodyAccess Off is still loading request body in memory #3022

Open cerebox opened 10 months ago

cerebox commented 10 months ago

Hello! I'm trying to setup ModSecurity but I'm dealing with issues when uploading large files.

At first I had issues uploading files so I set SecRequestBodyAccess to Off, which is working fine for files up to ~800MB but with larger files (1.3GB), nginx is still crashing.

Logs and dumps

Output of:

  1. Error logs 
    terminate called after throwing an instance of 'std::bad_alloc'
what():  std::bad_alloc
[alert] 18607#18607: worker process 18608 exited on signal 6 (core dumped)
    1. Backtrace of core dump 
      #0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007f7bd79ac535 in __GI_abort () at abort.c:79
#2  0x00007f7bd70dc983 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#3  0x00007f7bd70e28c6 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#4  0x00007f7bd70e2901 in std::terminate() () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#5  0x00007f7bd70e2b89 in __cxa_rethrow () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#6  0x00007f7bd7834d84 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<std::istreambuf_iterator<char, std::char_traits<char> > > (this=this@entry=0x7ffc4a54a280, __beg=..., __end=...) at /usr/include/c++/8/ext/new_allocator.h:116
#7  0x00007f7bd7840489 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct_aux<std::istreambuf_iterator<char, std::char_traits<char> > > (__end=..., __beg=..., this=0x7ffc4a54a280) at /usr/include/c++/8/bits/basic_string.h:252
#8  std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<std::istreambuf_iterator<char, std::char_traits<char> > > (
__end=..., __beg=..., this=0x7ffc4a54a280) at /usr/include/c++/8/bits/basic_string.h:255
#9  std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string<std::istreambuf_iterator<char, std::char_traits<char> >, void> (__a=..., __end=..., __beg=..., this=0x7ffc4a54a280) at /usr/include/c++/8/bits/basic_string.h:617
#10 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_replace_dispatch<std::istreambuf_iterator<char, std::char_traits<char> > > (__k2=..., __k1=..., __i2=..., __i1=0 '\000', this=0x7ffc4a54a1e0) at /usr/include/c++/8/bits/basic_string.tcc:384
#11 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::replace<std::istreambuf_iterator<char, std::char_traits<char> >, void> (
__k2=..., __k1=..., __i2=..., __i1=..., this=0x7ffc4a54a1e0) at /usr/include/c++/8/bits/basic_string.h:2091
#12 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::assign<std::istreambuf_iterator<char, std::char_traits<char> >, void> (
__last=..., __first=..., this=0x7ffc4a54a1e0) at /usr/include/c++/8/bits/basic_string.h:1471
#13 modsecurity::Transaction::requestBodyFromFile (this=0x5642022075d0, path=<optimized out>) at transaction.cc:1032
#14 0x00007f7bd7fdbe5c in ngx_http_modsecurity_pre_access_handler (r=0x564202279dc0)
at /usr/local/src/ModSecurity-nginx/src/ngx_http_modsecurity_pre_access.c:170
#15 0x00005641fe7d512f in ngx_http_core_generic_phase ()
#16 0x00005641fe7d0bdd in ngx_http_core_run_phases ()
#17 0x00007f7bd7fdbd66 in ngx_http_modsecurity_request_read (r=<optimized out>) at /usr/local/src/ModSecurity-nginx/src/ngx_http_modsecurity_pre_access.c:38
#18 0x00005641fe80ac33 in ?? ()
#19 0x00005641fe80d980 in ?? ()
#20 0x00005641fe7d9d1c in ?? ()
#21 0x00005641fe7b8810 in ngx_event_process_posted ()
#22 0x00005641fe7bff71 in ?? ()
#23 0x00005641fe7be84b in ngx_spawn_process ()
#24 0x00005641fe7c0374 in ?? ()
#25 0x00005641fe7c0bd7 in ngx_master_process_cycle ()
#26 0x00005641fe797588 in main ()

Expected behavior

I expected body not to be loaded in memory as SecRequestBodyAccess is set to Off and SecRequestBodyLimitAction is set to ProcessPartial.

Server :

Rule Set: OWASP coreruleset v3.3.5

Additional context

This issue is similar to https://github.com/SpiderLabs/ModSecurity/issues/1517

Would this patch be acceptable ? I have tested it with large files and nginx does not crash with it. Thanks.

diff --git a/src/transaction.cc b/src/transaction.cc
index 0c98b49c..fffeb70f 100644
--- a/src/transaction.cc
+++ b/src/transaction.cc
@@ -1023,13 +1023,13 @@ int Transaction::requestBodyFromFile(const char *path) {
     request_body.seekg(0, std::ios::end);
     try {
         str.reserve(request_body.tellg());
+        request_body.seekg(0, std::ios::beg);
+        str.assign((std::istreambuf_iterator<char>(request_body)),
+                   std::istreambuf_iterator<char>());
     } catch (...) {
         ms_dbg(3, "Failed to allocate memory to load request body.");
         return false;
     }
-    request_body.seekg(0, std::ios::beg);
-    str.assign((std::istreambuf_iterator<char>(request_body)),
-            std::istreambuf_iterator<char>());

     const char *buf = str.c_str();
     int len = request_body.tellg();
martinhsv commented 10 months ago

Hello @cerebox ,

What is the Content-Type for these requests? Is it 'multipart/form-data'? Or something else?

Are you using SecRequestBodyLimit? If so, what is its size compared to the size of the body in the request?

cerebox commented 9 months ago

Hello @martinhsv

The Content-Type of the request is multipart/form-data.

I tried and without and with using SecRequestBodyLimit and SecRequestBodyNoFilesLimit with these values

SecRequestBodyLimit 524288000
SecRequestBodyNoFilesLimit 524288000

I have a different behavior this time, the nginx worker is killed by the OOM reaper

martinhsv commented 8 months ago

Hello @cerebox ,

SecRequestBodyAccess Off has never truly shut off access to the request body in ModSecurity v3. The current behaviour of this setting is non-intuitive and problematic (see #2465 for some additional detail). Use with caution or not at all (the latter would be my suggestion).

ProcessPartial is also usually not the best choice. See my two comments from Feb. 17, 2022 beginning here: https://github.com/SpiderLabs/ModSecurity/issues/1471#issuecomment-1042962449

Regarding your proposed code change, I believe it is a good one. It looks like the original fix from 2017 did not take into account that the file might have increased in size after the call to reserve().