SecRequestBodyAccess Off is still loading request body in memory

[cerebox]

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)

2. 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 :

• ModSecurity version (and connector): ModSecurity v3.0.10 with nginx-connector v1.0.3
• WebServer: nginx-1.22.0
• OS (and distro): Debian 10 (buster)

Rule Set:
OWASP coreruleset v3.3.5

Additional context

This issue is similar to #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]

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]

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]

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: #1471 (comment)

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().