-
Notifications
You must be signed in to change notification settings - Fork 1.6k
file uploads over 8k fail when using ModSecurity 2.7.5 and Nginx 1.4.2 #142
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
Comments
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 |
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 |
BUG: Garbage at the end of content_type header.
BUG: Requests in excess of 8k are not forwarded. +again:
|
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 |
Any comment on the second bug where file chunks are not passed to backend? |
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, |
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 |
You have to clone the repository and then switch to the "trunk" branch, using: git clone https://github.com/SpiderLabs/ModSecurity.git |
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. |
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.
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. |
Has this issue been addressed, i still cant get over 5K+ uploads with modsec on nginx ! @wellumies: have you tried older version's ? |
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 |
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:
|
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" Strace and/or gdb can be used, for that, reduce the nginx workers to one, and plug the worker_processes 1; Remember to configure/build nginx with --with-debug support. |
Hi Felipe! I glad you are making progress. Is the workaround that when we reduce the Regards, Veli Pekka Jutila 2013/11/26 Felipe Zimmerle [email protected]
|
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.
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 |
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
|
cl->next = NULL; cl->buf->last_buf = 1; *ll = cl; } else { Tested, the result is negative the problem remained. |
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 |
2.8.0 did not fix this? |
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:
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.
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:
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. |
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, |
The following change fixes segfault in ngx_atoi() mentioned earlier: 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. |
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? |
I can tentatively confirm that using nginx_refactoring I was able to fix this particular problem. |
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. 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. |
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. |
I just built the standalone ModSecurity module from the
revelant portion of
Uploads >= 1MB are failing for me. nginx is forwarding these requests to an upstream server which handles the actual upload, FWIW.
Log for failing file upload:
Disabling ModSecurity completely causes both file upload attempts to work. Also, setting Let me know if you want additional information and/or help testing. |
I think it still exists in modsecurity2.9.1 for nginx, and I use nginx1.8.1 now. |
how soon can you commit a fix ;) |
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 |
Hi zimmerle, |
@wellumies You can compare this function with the .tar.gz file for nginx in https://www.modsecurity.org/download.html. |
No longer a concern in libModSecurity. Marking it as won't fix for 2.x. Further information about libModSecurity available here: |
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"
The text was updated successfully, but these errors were encountered: