Add an Option to Skip Body Inspections ( Closes #343 )

[thekief]

As dicussed in the issue, I would like to add 2 new directives that allow to skip the body inspection. There are a few usecases, where, e.g. encrypted data is set, and no useful inspection can be made.

While denying the body access may skip the inspection, it still results in the caching of data. Subsequently, there is an unneeded amount of resource consumption, memory, as well as time, involved.

While it would be cleaner to expose an API that allows users of libmodsecurity to check, if a path, e.g. has a reqeust body check, this involves a lot more work. The approach taken in this PR, shifts the functionality to that is only necessitates changes in the nginx module.

Regarding Tests: As for tests, I'm a bit unsure what the best way would be to test it. One way would be to try to upload a file, e.g. 100MB, to a location and check when the first data reaches it. If the body inspection is disabled, the first bytes will arrive much sooner, as the nginx module caches the body until it's fully received otherwise and only then forwards the data.

[airween]

Hi @thekief,

thanks for this PR, I think this is quite useful.

Regarding Tests: As for tests, I'm a bit unsure what the best way would be to test it.

If I suggest suggest a method:

• create a vhost config where you turn Off the request body inspection
• create a rule (into phase:2) that checks for a variable from the request body (eg ARGS_POST)
• send a request which should trigger that rule

For the response body skip:

• create another vhost config where you turn Off the response body inspection
• create a rule (into phase:4) that checks for a variable from the response body (eg RESPONSE_BODY)
• set a content for that vhost which triggers that rule

You can add cross checking too (eg. the first mentioned rule above executes in phase:4 and you try to mix the settings with these new options...)

[thekief]

Sorry for getting back so late. I created configurations, but as I'm terrible with nginx tests using Perl, may I just send you the configurations? 😅

[HanadaLee]

The abbreviation for response is usually rep or resp, or consider not using the abbreviation and directly name the directive: modsecurity_skip_request_body_filter and modsecurity_skip_response_body_filter

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[thekief]

@HanadaLee thank you for looking over the changes. I use resp to abbreviate response now

[airween]

@thekief could you rebase your PR? Then we can find some solution for testing.

[thekief]

Done. I also had to add another commit to address the number of directives mentioned in the readme.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[HanadaLee]

I found several blocking issues in the current implementation. In particular, the request-side directive currently disables much more than request-body inspection, and the new configuration fields do not inherit correctly.

1. modsecurity_skip_req_body_filter skips the entire ModSecurity transaction

In ngx_http_modsecurity_access_handler(), the new check returns before the ModSecurity context and transaction are created:

if (mcf->skip_req_body_filter == 1) {
    dd("Skipping request body filter");
    return NGX_DECLINED;
}

This does not only skip request-body buffering. It also skips:

connection processing;
URI processing;
request-header processing;
phase 1 rules;
response-header processing;
response-body processing;
transaction logging.

Because no context is created, the response filters and log handler subsequently treat ModSecurity as disabled for the entire request.

This contradicts the intended behavior described in issue #343, which is to skip body inspection while preserving the other ModSecurity checks.

The skip check should be moved after the transaction, URI, and request headers have been processed. It should only bypass ngx_http_read_client_request_body() and the body-appending logic.

The intended phase 2 behavior should also be defined explicitly. It may still be necessary to call:

msc_process_request_body(ctx->modsec_transaction);

without appending body data, so that phase 2 rules that do not depend on request-body variables can still run.

2. The new directives do not inherit from http or server

The fields are allocated through ngx_pcalloc(), so they initially have the value 0. They are not initialized to NGX_CONF_UNSET, and they are not handled in ngx_http_modsecurity_merge_conf().

As a result, a configuration such as:

server {
modsecurity_skip_req_body_filter on;

location /upload {
    proxy_pass http://backend;
}

}

does not enable the option inside /upload, because the child location has its own zero-initialized value.

Please initialize both fields in ngx_http_modsecurity_create_conf():

conf->skip_req_body_filter = NGX_CONF_UNSET;
conf->skip_resp_body_filter = NGX_CONF_UNSET;

and merge them in ngx_http_modsecurity_merge_conf():

ngx_conf_merge_value(c->skip_req_body_filter,
                     p->skip_req_body_filter, 0);

ngx_conf_merge_value(c->skip_resp_body_filter,
                     p->skip_resp_body_filter, 0);

Without this, the documented http and server contexts do not work as expected.

3. The response body filter no longer restores a cleared module context

The PR changes:

ctx = ngx_http_modsecurity_get_module_ctx(r);

to:

ctx = ngx_http_get_module_ctx(r, ngx_http_modsecurity_module);

The custom ngx_http_modsecurity_get_module_ctx() helper is important because it restores the transaction context from the request-pool cleanup handlers when the module context has been cleared, for example after an internal redirect.

Using ngx_http_get_module_ctx() directly can therefore cause response-body inspection to be silently skipped after an internal redirect.

This is a regression even when both new directives are disabled. The original helper should continue to be used.

4. Response buffering is still requested when response inspection is skipped

The response header filter unconditionally sets:

r->filter_need_in_memory = 1;

The new skip_resp_body_filter check only happens later in the body filter. Therefore, Nginx is still told that the response body must be kept in memory even when response-body inspection is disabled.

To achieve the intended resource-saving behavior, the configuration should also be checked in the header filter:

if (!mcf->skip_resp_body_filter) {
    r->filter_need_in_memory = 1;
}

Response-header processing should still continue normally.

The phase 4 behavior should also be clarified. Completely skipping msc_process_response_body() skips all phase 4 rules, including rules that may not depend on RESPONSE_BODY.

5. Documentation and tests

There are also several documentation issues:

The README says the connector adds seven directives, but there are six existing directives plus two new ones, so the total should be eight.
The new directives are documented after the Contributing section instead of in the directive list.
The response directive says that it skips caching of the “request body”.
The SecRequestBodyAccess description should say that it is set to Off.

Tests should cover at least:

phase 1 rules still running when request-body inspection is skipped;
request-body rules not receiving body data;
response-header and logging phases still running;
inheritance and overriding at http, server, and location levels;
response-body inspection after an internal redirect;
streaming behavior with proxy_request_buffering off.

[thekief]

Hey @HanadaLee ,
thank you for pointing that out. It looks like I messed up the rebase back in the day and never pushed the full fix again ...

Regarding the documentation and the number of directives:
I have a second PR open, which I expected to me merged prior to this one but it also got stuck in a limbo.

tbh, I'm not involded with any nginx projects anymore so I cannot really test it either without re-building a test setup and I don't really have time for it either.

If you want, I can check if I can find the intial patch and maybe you can get it merged. Otherwise, I'd close it as I don't want to keep it open without any progress.

[HanadaLee]

Hey @HanadaLee , thank you for pointing that out. It looks like I messed up the rebase back in the day and never pushed the full fix again ...

Regarding the documentation and the number of directives: I have a second PR open, which I expected to me merged prior to this one but it also got stuck in a limbo.

tbh, I'm not involded with any nginx projects anymore so I cannot really test it either without re-building a test setup and I don't really have time for it either.

If you want, I can check if I can find the intial patch and maybe you can get it merged. Otherwise, I'd close it as I don't want to keep it open without any progress.

Alright, if you don't mind, I'll probably submit a new PR based on your work, since I really need this feature.

[airween]

@HanadaLee: first of all, thank you for your participation.

@thekief and @HanadaLee: I would like to let you know guys that I've been working on v4, where there will be some new features in API, eg. the application can ask the engine does it need to send the request body for the check (based on SecResponseBodyMimeType settings).

Also please note, that there is a known wrong behavior, described in ModSecurity/#2465, which needs to be fixed before we touch this part of API. I plan to fix it in v4 too.

[HanadaLee]

@HanadaLee: first of all, thank you for your participation.

@thekief and @HanadaLee: I would like to let you know guys that I've been working on v4, where there will be some new features in API, eg. the application can ask the engine does it need to send the request body for the check (based on SecResponseBodyMimeType settings).

Also please note, that there is a known wrong behavior, described in ModSecurity/#2465, which needs to be fixed before we touch this part of API. I plan to fix it in v4 too.

I am excited to see a brand-new version of ModSecurity on the horizon. I am currently planning to add support for the following features and hope they can be considered during development:

1. modsecurity_bypass $xxxx (Skip ModSecurity functionality based on certain Nginx variables.)
2. modsecurity_dry_run on | off (No interception. only status logging.)
3. modsecurity_intervention_mode follow | deny | log -> only log, do not action (Actually, I'm still considering it, because it seems very similar to a dry run.)
4. $modsecurity_status: PASSED, REJECTED, REJECTED_DRY_RUN（like $limit_req_status）
5. $modsecurity_intervention_status: Returns the status code for ModSecurity intervention; recorded as NGX_OK (0) if no intervention occurred.
6. $modsecurity_triggered_rules: Add modsecurity log vars #374 (some bug, deferred)
7. Request body inspection bypass: Add an Option to Skip Body Inspections ( Closes #343 ) #356 (some bug, deferred)
8. H3 support: Add HTTP/3 Host header support for ModSecurity #364 (some bug, deferred)
9. ​​Configuration inheritance: Reuse already initialized rule set for each location #291 (some bug, deferred)

[thekief]

@airween is this PR something you'd see as something that directly relates to/affects owasp-modsecurity/ModSecurity#2465 ?

[airween]

I am excited to see a brand-new version of ModSecurity on the horizon.

Just for the record: it's not a brand new 😃.

But we definitely need to extend the API, therefore we need to increase the version.

is this PR something you'd see as something that directly relates to/affects owasp-modsecurity/ModSecurity#2465 ?

No, that's different. We should solve that inside of the library.

I'll reply your listed items later.