Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

file uploads over 8k fail when using ModSecurity 2.7.5 and Nginx 1.4.2 #142

Closed
wellumies opened this issue Sep 12, 2013 · 37 comments
Closed
Assignees

Comments

@wellumies
Copy link

Linux debian 3.2.0-4-amd64 #1 SMP Debian 3.2.46-1+deb7u1 x86_64 GNU/Linux

ModSec 2.7.5 and Nginx 1.4.2

I have an Apache backend and it receives my file uploads and requests if the file is below 8k. Only got the basic modsecurity.conf loaded without any rules. If I set the SecRequestBodyAccess = Off even those pass through. Succesful upload:


[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Initialising transaction (txid AcAcAGl3AcAcAcAcAcAcAcAc).
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Transaction context created (dcfg 7f35a9f41980).
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Starting phase REQUEST_HEADERS.
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Second phase starting (dcfg 7f35a9f41980).
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Input filter: Reading request body.
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Multipart: Created temporary file 1 (mode 0600): /var/log/modsecurity_workdir/20130912-151049-AcAcAGl3AcAcAcAcAcAcAcAc-file-vIn5DC
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][5] Adding request argument (BODY): name "submit", value "Submit"
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Request body no files length: 150
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Input filter: Completed receiving request body (length 4719).
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Starting phase REQUEST_BODY.
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Hook insert_filter: Adding input forwarding filter (r 7f35a9d950a0).
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Hook insert_filter: Adding output filter (r 7f35a9d950a0).
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Input filter: Forwarding input: mode=0, block=0, nbytes=-1 (f 7f35a9d962b0, r 7f35a9d950a0).
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Input filter: Forwarded 4719 bytes.
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Input filter: Sent EOS.
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Input filter: Input forwarding complete.
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Starting phase RESPONSE_HEADERS.
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Output filter: Not buffering response body for unconfigured MIME type "text/html".
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Output filter: Completed receiving response body (non-buffering).
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Starting phase RESPONSE_BODY.
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Output filter: Output forwarding complete.
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Initialising logging.
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Starting phase LOGGING.
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Recording persistent data took 0 microseconds.
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Audit log: Ignoring a non-relevant request.
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Multipart: Cleanup started (remove files 1).
[12/Sep/2013:15:10:49 +0300] [/sid#7f35a9f410a0][rid#7f35a9d950a0][/upload_file.php][4] Multipart: Deleted file (part) "/var/log/modsecurity_workdir/20130912-151049-AcAcAGl3AcAcAcAcAcAcAcAc-file-vIn5DC"


failed upload:


[12/Sep/2013:15:12:48 +0300] [/sid#7f35a9f410a0][rid#7f35a9d8d0a0][/upload_file.php][4] Initialising transaction (txid AcAcATAcccAcAcRcvYAIpcAc).
[12/Sep/2013:15:12:48 +0300] [/sid#7f35a9f410a0][rid#7f35a9d8d0a0][/upload_file.php][4] Transaction context created (dcfg 7f35a9f41980).
[12/Sep/2013:15:12:48 +0300] [/sid#7f35a9f410a0][rid#7f35a9d8d0a0][/upload_file.php][4] Starting phase REQUEST_HEADERS.
[12/Sep/2013:15:12:48 +0300] [/sid#7f35a9f410a0][rid#7f35a9d8d0a0][/upload_file.php][4] Second phase starting (dcfg 7f35a9f41980).
[12/Sep/2013:15:12:48 +0300] [/sid#7f35a9f410a0][rid#7f35a9d8d0a0][/upload_file.php][4] Input filter: Reading request body.
[12/Sep/2013:15:12:48 +0300] [/sid#7f35a9f410a0][rid#7f35a9d8d0a0][/upload_file.php][4] Multipart: Created temporary file 1 (mode 0600): /var/log/modsecurity_workdir/20130912-151248-AcAcATAcccAcAcRcvYAIpcAc-file-qmZcxo
[12/Sep/2013:15:12:48 +0300] [/sid#7f35a9f410a0][rid#7f35a9d8d0a0][/upload_file.php][5] Adding request argument (BODY): name "submit", value "Submit"
[12/Sep/2013:15:12:48 +0300] [/sid#7f35a9f410a0][rid#7f35a9d8d0a0][/upload_file.php][4] Request body no files length: 149
[12/Sep/2013:15:12:48 +0300] [/sid#7f35a9f410a0][rid#7f35a9d8d0a0][/upload_file.php][4] Input filter: Completed receiving request body (length 8893).
[12/Sep/2013:15:12:48 +0300] [/sid#7f35a9f410a0][rid#7f35a9d8d0a0][/upload_file.php][4] Starting phase REQUEST_BODY.
[12/Sep/2013:15:12:48 +0300] [/sid#7f35a9f410a0][rid#7f35a9d8d0a0][/upload_file.php][4] Hook insert_filter: Adding input forwarding filter (r 7f35a9d8d0a0).
[12/Sep/2013:15:12:48 +0300] [/sid#7f35a9f410a0][rid#7f35a9d8d0a0][/upload_file.php][4] Hook insert_filter: Adding output filter (r 7f35a9d8d0a0).
[12/Sep/2013:15:12:48 +0300] [/sid#7f35a9f410a0][rid#7f35a9d8d0a0][/upload_file.php][4] Input filter: Forwarding input: mode=0, block=0, nbytes=-1 (f 7f35a9d8e2b0, r 7f35a9d8d0a0).
[12/Sep/2013:15:12:48 +0300] [/sid#7f35a9f410a0][rid#7f35a9d8d0a0][/upload_file.php][4] Input filter: Forwarded 8192 bytes.
[12/Sep/2013:15:13:48 +0300] [/sid#7f35a9f410a0][rid#7f35a9d8d0a0][/upload_file.php][4] Initialising logging.
[12/Sep/2013:15:13:48 +0300] [/sid#7f35a9f410a0][rid#7f35a9d8d0a0][/upload_file.php][4] Starting phase LOGGING.
[12/Sep/2013:15:13:48 +0300] [/sid#7f35a9f410a0][rid#7f35a9d8d0a0][/upload_file.php][4] Recording persistent data took 0 microseconds.
[12/Sep/2013:15:13:48 +0300] [/sid#7f35a9f410a0][rid#7f35a9d8d0a0][/upload_file.php][4] Audit log: Ignoring a non-relevant request.
[12/Sep/2013:15:13:48 +0300] [/sid#7f35a9f410a0][rid#7f35a9d8d0a0][/upload_file.php][4] Multipart: Cleanup started (remove files 1).
[12/Sep/2013:15:13:48 +0300] [/sid#7f35a9f410a0][rid#7f35a9d8d0a0][/upload_file.php][4] Multipart: Deleted file (part) "/var/log/modsecurity_workdir/20130912-151248-AcAcATAcccAcAcRcvYAIpcAc-file-qmZcxo"

@wellumies
Copy link
Author

After the 4k upload ModSec adds a double charset-header with random value after UTF-8. Random value changes if I change request parameters

Content-Type=text/html; charset=UTF-8; charset=UTF-8Q

@wellumies
Copy link
Author

There seems to be no workaround for the double charset. ModSec will add it even if nginx is configured with "charset off" or "override charset off". When ModSec sees Content-Type it will add a charset header. The memory corruption after UTF-8 is a result of unterminated string being used.

We have made modifications to the source code so at least 64k uploads now pass ModSec, but we are still testing

@wellumies
Copy link
Author

BUG: Garbage at the end of content_type header.
REASON: Ngx_snprintf does not terminate the string. Neither does
modsecurity.
PROPOSED FIX:
diff --git a/nginx/modsecurity/ngx_http_modsecurity.c
b/nginx/modsecurity/ngx_http_modsecurity.c
index 26112a6..c1350c0 100644
--- a/nginx/modsecurity/ngx_http_modsecurity.c
+++ b/nginx/modsecurity/ngx_http_modsecurity.c
@@ -618,6 +618,7 @@
ngx_http_modsecurity_load_headers_out(ngx_http_request_t *r)
"%V; charset=%V",
&r->headers_out.content_type,
&r->headers_out.charset);

  •    content_type[content_type_len-1] = 0; // ngx_snprintf does not
    

    terminate the string

      r->headers_out.content_type.data = content_type;
      r->headers_out.content_type.len = content_type_len;
    

BUG: Requests in excess of 8k are not forwarded.
REASON: It seems only the first chunk is forwarded--there's no logic to
forward the rest of the chunks.
PROPOSED FIX:
diff --git a/apache2/apache2_io.c b/apache2/apache2_io.c
index 88f1903..6629c4a 100644
--- a/apache2/apache2_io.c
+++ b/apache2/apache2_io.c
@@ -66,6 +66,7 @@ apr_status_t input_filter(ap_filter_t *f,
apr_bucket_brigade *bb_out,
" (f %pp, r %pp).", mode, block, nbytes, f, f->r);
}

+again:
if (msr->if_started_forwarding == 0) {
msr->if_started_forwarding = 1;
rc = modsecurity_request_body_retrieve_start(msr, &my_error_msg);
@@ -154,6 +155,8 @@ apr_status_t input_filter(ap_filter_t *f,
apr_bucket_brigade *bb_out,
if (msr->txcfg->debuglog_level >= 4) {
msr_log(msr, 4, "Input filter: Input forwarding complete.");
}

  • } else {

  •    goto again;
    

    }

    return APR_SUCCESS;

@zimmerle
Copy link
Contributor

zimmerle commented Oct 1, 2013

Hi @wellumies,

Indeed, the ngx_snprintf does not place the string terminator. Note that the size was allocated considering the terminator but it was not set, so It could be fixed by placing a '\0' in the end of the string. As I did here: zimmerle@ff19dcd

@wellumies
Copy link
Author

Any comment on the second bug where file chunks are not passed to backend?

@zimmerle
Copy link
Contributor

Hi @wellumies,

It seems to me that the second bug was a consequence of the first one. Can you check it again, and, if still happens, can you give more information on how to reproduce?

Thanks,
F.

@wellumies
Copy link
Author

Can't imagine how the second bug would be a consequence of a string terminator missing, since the second bug is happening because there is no logic to pass more than 1 chunk to the backend...

Can you give me the exact git clone command to get the fixed version? I'm not familiar with Git usage

@zimmerle
Copy link
Contributor

You have to clone the repository and then switch to the "trunk" branch, using:

git clone https://github.com/SpiderLabs/ModSecurity.git
git checkout origin/remotes/trunk -b trunk

@taleodor
Copy link

I have same problem with the latest trunk source code on Nginx. I'm getting error for longer (approximately, more than 64 KB) JSON POST payloads. Short payloads work fine. It also works with SecRequestBodyAccess and SecResponseBodyAccess set to Off.

zimmerle pushed a commit to zimmerle/ModSecurity that referenced this issue Oct 24, 2013
The headers are represented in the format of an apr_table, which
is able to handle elements with the same key, however the function
apr_table_setn checks if the key exists before add the element, if so
it replaces the old value with the new one. This was making our
implementation to just keep the last added Cookie. The apr_table_addn
function, which is now used, just add a new item without check for
olders one.
@wellumies
Copy link
Author

Haven't had time to test the header fix, but I would like to know if the file upload has been fixed? I think I'm going to lose a customer :( They have been waiting for a month now for ModSec for Nginx which is totally unusable atm, since no files can be passed to the backend.

@monasor28
Copy link

Has this issue been addressed, i still cant get over 5K+ uploads with modsec on nginx !

@wellumies: have you tried older version's ?

@wellumies
Copy link
Author

Well it has been addressed by me and even supplied a fix in one of my comments.

I haven't tried any older versions. I'm quessing nginx version uses some different code paths in apache2_io.c, since I have no problems using this version with Apache

@zimmerle
Copy link
Contributor

Hi @wllumies,

in fact the bug exists. As you said it is not happening in Apache, just nginx (not tested in IIS yet). So far i can tell this very same code is used in our tree ports. I believe that the patch that you have provided will not fit for all platforms that we support, as you said Apache is working well. It is important to investigate the nginx port to understand what it is providing to that function and what is expected, i think that is the way to go. Maybe there is a fragmentation or something that is messing the data.

So far, nginx users can use you patch. I am not merging due the fact that it may be a problem with users from others platforms.

@wellumies if you are interested to still work in this bug (or others) it will be a pleasure to help.

Just to document a systematic way to reproduce the problem:

  1. Go to http://www.json-generator.com/, generate a JSON with: repeat(5, 800).
  2. Make sure that SecRequestBodyAccess is set to "On" in our modsecurity configuration:
    SecRequestBodyAccess On
  3. Use Curl to POST the recent created JSON to any script that is able to handle it (via your nginx reverse proxy):
    curl -H "Content-Type: application/json" -X POST -d @/path/to/the/json http://localhost/cgi-bin/printenv
  4. The connection will time out, the nginx worker process will be dead.

@zimmerle
Copy link
Contributor

Just a quick update on this issue.

Nginx handles big amount of data from POSTs into chunks. Since the idea behind nginx is to avoid the use of loop it does return NGX_AGAIN whenever a second (third, ..., nth) call will be placed in order to fill up a buffer while using the "ngx_http_read_client_request_body".

Currently ModSecurity got lost if NGX_AGAIN is returned. Apparently the data produced by the work into chunks are not being consumed correctly by ModSecurity. One probably cause is the gap of the information to mount a buffer from the chunks. Looking at other modules they do use something called "upstream" in a way that it helps connecting the chunks. Further study needs to be done in order to get it working correctly.

If someone else in interested to help, here goes a good reference:

By editing "ngx_http_modsecurity.c" there is a call to "ngx_http_read_client_request_body"
using "ngx_http_modsecurity_body_handler" as an argument. Comment this line the bug gone away
unfortunately with the body parser :)

Strace and/or gdb can be used, for that, reduce the nginx workers to one, and plug the
gdb/strace using -p . To reduce the workers to one, place in the nginx.conf:

worker_processes 1;

Remember to configure/build nginx with --with-debug support.

@wellumies
Copy link
Author

Hi Felipe!

I glad you are making progress. Is the workaround that when we reduce the
amount of workers to one ModSec will find all the chunks? I think we will
wait for a proper fix. We really need the body parser to work.

Regards,

Veli Pekka Jutila

2013/11/26 Felipe Zimmerle notifications@github.com

Just a quick update on this issue.

Nginx handles big amount of data from POSTs into chunks. Since the idea
behind nginx is to avoid the use of loop it does return NGX_AGAIN whenever
a second (third, ..., nth) call will be placed in order to fill up a buffer
while using the "ngx_http_read_client_request_body".

Currently ModSecurity got lost if NGX_AGAIN is returned. Apparently the
data produced by the work into chunks are not being consumed correctly by
ModSecurity. One probably cause is the gap of the information to mount a
buffer from the chunks. Looking at other modules they do use something
called "upstream" in a way that it helps connecting the chunks. Further
study needs to be done in order to get it working correctly.

If someone else in interested to help, here goes a good reference:

By editing "ngx_http_modsecurity.c" there is a call to
"ngx_http_read_client_request_body"
using "ngx_http_modsecurity_body_handler" as an argument. Comment this
line the bug gone away
unfortunately with the body parser :)

Strace and/or gdb can be used, for that, reduce the nginx workers to one,
and plug the
gdb/strace using -p . To reduce the workers to one, place in the
nginx.conf:

worker_processes 1;

Remember to configure/build nginx with --with-debug support.


Reply to this email directly or view it on GitHubhttps://github.com//issues/142#issuecomment-29292828
.

zimmerle added a commit that referenced this issue Dec 16, 2013
The charset in headers is mounted using ngx_snprintf which
does not place the string terminator. This patch adds the
terminator at the end of the string. The size was correctly
allocated, just missing the terminator.

This bug was report at:
- https://www.modsecurity.org/tracker/browse/MODSEC-420
- #142

Both reports cames with patch, first by Veli Pekka Jutila and
second by wellumies.
@dimyse
Copy link

dimyse commented Dec 22, 2013

Tried on version "master" the same result. Large POST requests getting 408 (Request Timeout) from back-end

debug.log http://www.sendspace.com/file/xnxieo

access.log http://www.sendspace.com/file/iwu97g

@tattipravin
Copy link

I think the problem is in function move_brigade_to_chain() while allocating chain links. Its application module responsibility to initialize the cl->buf and cl->next pointers NGINX will only allocate the memory and returns without initializing to NULL. Hence it may contain some uninitialized value. Try with below code changes and check whether it fixes the issue. To fix this issue in move_brigade_to_chain() function initialize cl->next = NULL after ngx_alloc_chain_link().

diff --git a/nginx/modsecurity/apr_bucket_nginx.c b/nginx/modsecurity/apr_bucket_nginx.c
index 62eb59a..5825534 100644
--- a/nginx/modsecurity/apr_bucket_nginx.c
+++ b/nginx/modsecurity/apr_bucket_nginx.c
@@ -214,6 +214,7 @@ move_brigade_to_chain(apr_bucket_brigade _bb, ngx_chain_t *_ll, ngx_pool_t *pool
break;
}

  •            cl->next = NULL;
             cl->buf->last_buf = 1;
             *ll = cl;
         } else {
    

@dimyse
Copy link

dimyse commented Dec 24, 2013

cl->next = NULL; cl->buf->last_buf = 1; *ll = cl; } else {

Tested, the result is negative the problem remained.

@wellumies
Copy link
Author

Any updates on this issue? This bug kinda makes ModSec for Nginx unusable. If you are using Nginx as a loadbalancer with an Apache backend you can't post any files or large forms though ModSec

@wellumies
Copy link
Author

2.8.0 did not fix this?

@defanator
Copy link
Contributor

Hi @zimmerle,

I had a chance to do some testing with nginx and mod_security/2.8.0 (both master and nginx_refactoring branches). I can say that refactored code fixes a bunch of segfaults, mostly in ngx_http_write_filter() function. We have our own test suite that I used for testing:

http://hg.nginx.org/nginx-tests/

It requires a bit patching in order to turn on mod_security processing - something like this:

diff -r 646e2ea2c4b5 lib/Test/Nginx.pm
--- a/lib/Test/Nginx.pm Tue May 13 15:40:09 2014 +0400
+++ b/lib/Test/Nginx.pm Wed May 14 17:49:42 2014 +0400
@@ -25,6 +25,7 @@
 use POSIX qw/ waitpid WNOHANG /;
 use Socket qw/ CRLF /;
 use Test::More qw//;
+use File::Copy qw(copy);

 ###############################################################################

@@ -46,6 +47,13 @@
        mkdir "$self->{_testdir}/logs"
                or die "Can't create logs directory: $!\n";

+       open F, '>' . $self->{_testdir} . '/modsec.conf'
+               or die "Can't open modsec.conf: $!";
+       close F;
+
+       copy $0, $self->{_testdir} . '/';
+       chdir $self->{_testdir};
+
        return $self;
 }

It creates empty modsec.conf file in temporary test directory which is used as ModSecurityConfig directive parameter. In my understanding, using empty config should turn on mod_security processing (header/body filters) without any actual filtering - correct me if I'm wrong here.
I used the following environment vars to run prove:

TEST_NGINX_BINARY=/usr/sbin/nginx.debug
TEST_NGINX_LEAVE=yes
TEST_NGINX_GLOBALS_HTTP="ModSecurityEnabled on; ModSecurityConfig modsec.conf;"

The only segfaulted tests I've got under nginx_refactoring branch were proxy_redirect.t and proxy_cookie.t, and all their backtraces look like this:

#0  ngx_atoi (line=0x0, n=11267672) at src/core/ngx_string.c:907
907         if (*line < '0' || *line > '9') {
(gdb) bt
#0  ngx_atoi (line=0x0, n=11267672) at src/core/ngx_string.c:907
#1  0x00000000005634b7 in ngx_http_modsecurity_load_request (r=0x9e8860) at     extras/modsecurity-2.8.0/nginx/modsecurity/ngx_http_modsecurity.c:329
#2  ngx_http_modsecurity_process_request (r=0x9e8860) at extras/modsecurity-2.8.0/nginx/modsecurity/ngx_http_modsecurity.c:872
#3  0x0000000000563b93 in ngx_http_modsecurity_handler (r=0x9e8860) at extras/modsecurity-2.8.0/nginx/modsecurity/ngx_http_modsecurity.c:1185
#4  0x00000000004a7962 in ngx_http_core_generic_phase (r=0x9e8860, ph=0xab04e8) at src/http/ngx_http_core_module.c:910
#5  0x00000000004a46ad in ngx_http_core_run_phases (r=0x9e8860) at src/http/ngx_http_core_module.c:888
#6  0x00000000004b04d1 in ngx_http_process_request (r=0x9e8860) at src/http/ngx_http_request.c:1927
#7  0x00000000004b13cc in ngx_http_process_request_line (rev=0xadd1f8) at src/http/ngx_http_request.c:1022
#8  0x0000000000499f91 in ngx_epoll_process_events (cycle=0x9da010, timer=<value optimized out>, flags=<value optimized out>)
at src/event/modules/ngx_epoll_module.c:691
#9  0x000000000048fd3e in ngx_process_events_and_timers (cycle=0x9da010) at src/event/ngx_event.c:248
#10 0x00000000004979a5 in ngx_worker_process_cycle (cycle=0x9da010, data=<value optimized out>) at src/os/unix/ngx_process_cycle.c:820
#11 0x0000000000495e14 in ngx_spawn_process (cycle=0x9da010, proc=0x4978d0 <ngx_worker_process_cycle>, data=0x0, name=0x64ca7e "worker process", respawn=-3)
at src/os/unix/ngx_process.c:198
#12 0x0000000000497b3c in ngx_start_worker_processes (cycle=0x9da010, n=1, type=-3) at src/os/unix/ngx_process_cycle.c:365
#13 0x0000000000498364 in ngx_master_process_cycle (cycle=0x9da010) at src/os/unix/ngx_process_cycle.c:137
#14 0x0000000000477186 in main (argc=<value optimized out>, argv=<value optimized out>) at src/core/nginx.c:407

However, a large bunch of tests have failed (without segfaults) due to some breakage introduced by mod_security ("ModSecurityEnabled on" with an empty ruleset).

I've already submitted a couple of pull requests for the obvious fixes, and I'm going to continue my investigations for some time.

@zimmerle
Copy link
Contributor

Hi @defanator, thank you very much. We appreciate the contributions. I have saw your patches. They are under the queue. I will apply as soon as possible.

That is a nice way to have it tested, I just have used the ModSecurity regression test, which seems to pass in all the tests that we have there. I will try to find some time to have this nginx tests on our BuildBots (http://www.modsecurity.org/developers/buildbot/waterfall).

One huge difficult that I had while refactoring the code was to place those references count (https://github.com/SpiderLabs/ModSecurity/blob/nginx_refactoring/nginx/modsecurity/ngx_http_modsecurity.c#L1213-L1217) on the right place/time. I did by looking at other modules code (such as lua) and trusting in the debug messages.

For the configuration, maybe it will be good to have the ModSecurity recommend configuration (https://github.com/SpiderLabs/ModSecurity/blob/nginx_refactoring/modsecurity.conf-recommended) this configuration set the SecRuleEngine to "On" and others such as "SecRequestBodyAccess".

Keep investigation, contributions are very welcomed ;)

Thanks,
F.

@defanator
Copy link
Contributor

The following change fixes segfault in ngx_atoi() mentioned earlier:
defanator@1baeaef

I'm not sure that my approach is entirely correct: I use connection socket to determine port, while one may want to parse URI line for ":port" part. Let me know your thoughts here.

@autrejacoupa
Copy link

Running into 8192 byte file upload limit with Nginx 1.6.0 and Mod Security 2.8.0 from https://github.com/SpiderLabs/ModSecurity/tree/nginx_refactoring. Works fine when SecRequestBodyAccess Off and when turned On upload fails. The issue is still open with nginx_refactoring branch? any workarounds?

@zakarth
Copy link

zakarth commented Jan 29, 2015

I can tentatively confirm that using nginx_refactoring I was able to fix this particular problem.

@applematt
Copy link

I, too, can confirm I'm running into this issue with nginx 1.6.2 where uploads over 8k with SecRequestBodyAccess On.

2015/02/02 11:11:00 [notice] 24627#0: ModSecurity for nginx (STABLE)/2.8.0 (http://www.modsecurity.org/) configured.
2015/02/02 11:11:00 [notice] 24627#0: ModSecurity: APR compiled version="1.3.9"; loaded version="1.3.9"
2015/02/02 11:11:00 [notice] 24627#0: ModSecurity: PCRE compiled version="7.8 "; loaded version="7.8 2008-09-05"
2015/02/02 11:11:00 [notice] 24627#0: ModSecurity: LIBXML compiled version="2.7.6"

I will have to test using the nginx_refactoring branch when I have time. For now, I have SecRequestBodyAccess set to Off as file uploads are necessary.

@zimmerle
Copy link
Contributor

Hi guys, few minutes ago I've merge #904 into nginx_refactoring branch. It should fix this issue, please confirm that the issue is fixed.

@ghost
Copy link

ghost commented Jul 21, 2015

I just built the standalone ModSecurity module from the nginx_refactoring branch and compiled nginx 1.9.1 with it:

nginx version: nginx/1.9.1
built by gcc 4.8.2 20140120 (Red Hat 4.8.2-16) (GCC) 
built with OpenSSL 1.0.1k-fips 8 Jan 2015
TLS SNI support enabled
configure arguments: --add-module=../ModSecurity/nginx/modsecurity/ --prefix=/etc/nginx --sbin-path=/usr/sbin/nginx --conf-path=/etc/nginx/nginx.conf --error-log-path=/var/log/nginx/error.log --http-log-path=/var/log/nginx/access.log --pid-path=/var/run/nginx.pid --lock-path=/var/run/nginx.lock --http-client-body-temp-path=/var/cache/nginx/client_temp --http-proxy-temp-path=/var/cache/nginx/proxy_temp --http-fastcgi-temp-path=/var/cache/nginx/fastcgi_temp --http-uwsgi-temp-path=/var/cache/nginx/uwsgi_temp --http-scgi-temp-path=/var/cache/nginx/scgi_temp --user=nginx --group=nginx --with-http_ssl_module --with-http_realip_module --with-http_addition_module --with-http_sub_module --with-http_dav_module --with-http_flv_module --with-http_mp4_module --with-http_gunzip_module --with-http_gzip_static_module --with-http_random_index_module --with-http_secure_link_module --with-http_stub_status_module --with-http_auth_request_module --with-mail --with-mail_ssl_module --with-file-aio --with-ipv6 --with-http_spdy_module --with-cc-opt='-O2 -g -pipe -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic'

revelant portion of /etc/nginx/modsecurity.conf (~line 174)

SecRequestBodyLimit 1000000000000
SecRequestBodyNoFilesLimit 1000000000000
....
SecDebugLog /var/log/modsecurity-debug.log
SecDebugLogLevel 6

Uploads >= 1MB are failing for me. nginx is forwarding these requests to an upstream server which handles the actual upload, FWIW.
I enabled the debugging log tried 2 uploads. 1 file < 1MB and another file equal to 1MB.
Please note I removed server information and the actual URI's in the following log snippets. Log for successful file upload:

[21/Jul/2015:16:47:34 +0000] Input filter: Forwarded 2063 bytes.
[21/Jul/2015:16:47:34 +0000]  Input filter: Sent EOS.
[21/Jul/2015:16:47:34 +0000] Input filter: Input forwarding complete.
[21/Jul/2015:16:47:34 +0000] Starting phase RESPONSE_HEADERS.
[21/Jul/2015:16:47:34 +0000] Output filter: Not buffering response body for unconfigured MIME type "application/json".
[21/Jul/2015:16:47:34 +0000] Output filter: Completed receiving response body (non-buffering).
[21/Jul/2015:16:47:34 +0000] Starting phase RESPONSE_BODY.
[21/Jul/2015:16:47:34 +0000] Output filter: Output forwarding complete.
[21/Jul/2015:16:47:34 +0000] Initialising logging.

Log for failing file upload:

[21/Jul/2015:17:01:02 +0000] Input filter: Forwarded 8192 bytes.
[21/Jul/2015:17:01:12 +0000] Initialising logging.
[21/Jul/2015:17:01:12 +0000] Starting phase LOGGING.

Disabling ModSecurity completely causes both file upload attempts to work. Also, setting SecRequestBodyAccess Off in the modsecurity configuration file also causes both uploads to work.

Let me know if you want additional information and/or help testing.

@mwang911
Copy link

I think it still exists in modsecurity2.9.1 for nginx, and I use nginx1.8.1 now.
The problem is that the function modsecurity_request_body_retrieve is wrongly used in function input_filter, if modsecurity_request_body_retrieve return 1, it means there are more chunks left, so it should be called again until it doesn't return 1.
It works fine now after I changed that.

@wellumies
Copy link
Author

how soon can you commit a fix ;)

@zimmerle
Copy link
Contributor

Hi @mwang911,

Are you sure that you are inspecting a full request body after that modification? not only the first chunk?

I would like to suggest you guys to use the ModSecurity-nginx connector, available at: https://github.com/SpiderLabs/ModSecurity-nginx

Further information on libmodsecurity is available here: https://github.com/SpiderLabs/ModSecurity/tree/v3/master

@mwang911
Copy link

Hi zimmerle,
I just test the file uploading and it works well now. It could retrieve all request body for nginx not only the max 8k. It is a advice I want you to consider, because I didn't make full other tests. As far as i know, it is related to the retrieving of request body. The result is that nginx still think the request body has not been read.

@mwang911
Copy link

@wellumies
Hi, the ngx_refactoring has done that in the right way. Look at the function input_filter in https://github.com/SpiderLabs/ModSecurity/blob/nginx_refactoring/apache2/apache2_io.c .

You can compare this function with the .tar.gz file for nginx in https://www.modsecurity.org/download.html.

@victorhora
Copy link
Contributor

No longer a concern in libModSecurity. Marking it as won't fix for 2.x.

Further information about libModSecurity available here:
https://github.com/SpiderLabs/ModSecurity/tree/v3/master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests