modsecurity + auth_request hang

[chaoyun]

hi, Experts:
I meet some issue why enable modSecurity plus with auth_request + http/2.

If a request with POST data, it will hang there. But "GET" method works well.

There's a similar issue discussed in the kubernetes/ingress-nginx#1932
Disable modSecurity, it can work. Disable auth_request, it can also work. Using http1.1 can work too. Only the combination of "modSecurity"+"auth_request"+http1.1 cannot work.
Do you have any test for this case?

[chaoyun]

@defanator Hi, I see you fixed this issue #131. But with my latest test, seems it doesn't work. In your testcase, I see you use http/2, right?

[defanator]

@chaoyun in connector tests we are using both HTTP/1 and HTTP/2 where applicable. I believe there were some TODO-marked tests specifically for HTTP/2, and auth_request can be one of those. I need to refresh my memory about that topic.

[chaoyun]

@defanator Thanks for the reply. Waiting for your update. I checked the log in the modSecurity, seems it parsed the request, and there's no further log in both modSecurity and nginx. I don't know where it hung.

[zimmerle]

@chaoyun, @defanator

Is this still an issue?

[defanator]

@zimmerle last time I checked there was an issue with HTTP/2. I'll try to pick some time to refresh this one.

[zimmerle]

Thank you @defanator. Warming up a release. Checking on what could be closed.

[dcherniv]

@defanator @zimmerle just got bit by this one too. Specifically kibana is completely broken with modsecurity + oauth proxy, which is a common combination people use

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

[dcherniv]

/reopen

[victorhora]

The "nostale" tag has been set for this one and it's now reopened. We'll get to it when possible. Thank you.

[PaulCharlton]

see also:
kubernetes/ingress-nginx#5723
owasp-modsecurity/ModSecurity#2341

definitive workaround:
add use-http2: false to the Nginx listen block or k8s Nginx-ingress ConfigMap

At this point, we can write a unit test that sets conditions: (1) "POST + non-empty body", (2) side-car sub request Auth, (3) modsecurity enabled, (4) force HTTP2 + TLS session. iterate until the unit test works.

I imagine that since modsecurity is inspecting the request body, even when modsecurity SecRulesEngine Off, and that it is somehow interfering with the state of the HTTP2 protocol handler. I traced it down to a single byte in the payload, which is the blank line between the headers and the body. It appears to be consumed by modsecurity, but not consumed when modsecurity is off. For some reason as yet unknown to me, this impacts the sub-request to the Auth, but does not impact the upstream server request body.

[PaulCharlton]

@victorhora @zimmerle with the information above, this should be relatively easy to track down

[PaulCharlton]

this code looks really suspicious, and there are no unit tests configured with http2.
the phrase r->main->count--; can be invoked twice and there is no explanation for why it is Nginx version dependent.

src/ngx_http_modsecurity_pre_access.c

        r->request_body_in_single_buf = 1;
        r->request_body_in_persistent_file = 1;

        rc = ngx_http_read_client_request_body(r,
            ngx_http_modsecurity_request_read);
        if (rc == NGX_ERROR || rc >= NGX_HTTP_SPECIAL_RESPONSE) {
#if (nginx_version < 1002006) ||                                             \
    (nginx_version >= 1003000 && nginx_version < 1003009)
            r->main->count--;
#endif

            return rc;
        }

and

void
ngx_http_modsecurity_request_read(ngx_http_request_t *r)
{
    ngx_http_modsecurity_ctx_t *ctx;

    ctx = ngx_http_get_module_ctx(r, ngx_http_modsecurity_module);

#if defined(nginx_version) && nginx_version >= 8011
    r->main->count--;
#endif

    if (ctx->waiting_more_body)
    {
        ctx->waiting_more_body = 0;
        r->write_event_handler = ngx_http_core_run_phases;
        ngx_http_core_run_phases(r);
    }
}

the most important thing is that no internal NGINX state should be changed as a result of modsecurity reading the request body. It is possible that Nginx should be giving modules a copy of the request buffers rather than the original request buffers, however, this module should have some sanity checking that "Nginx_state_before_processing==nginx_state_after_processing"

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

[mtorromeo]

This is still an issue. The nostale label does not seem to work...

/reopen

[mtorromeo]

I noticed that in tests/modsecurity-request-body-h2.t there are already 4 tests that are specifically for this exact issue but are contained in a TODO block.

I removed the TODO and after running the tests with different versions of nginx I found out that the tests are failing with the stable release 1.18.0 but are passing with mainline nginx 1.19.6

[mtorromeo]

I just finished bisecting the nginx repository to find the exact commit that made the tests pass and this is it: nginx/nginx@6c89d75

I am not sure if a workaround is possible.

[defanator]

@mtorromeo thanks for digging into this! I have marked those tests as TODO and was going to specify the exact version where the issue has been resolved. Hopefully to find some time this week for that.

[mtorromeo]

I was hoping to find an easy fix and it's a bit disappointing to find out that it's actually a bug of nginx itself and that such a critical issue was not even backported to stable but what can you do...

[defanator]

@mtorromeo adjusted TODO versioning here: 3f016fc

(there's another couple of passing TODOs related to alerts in error log; I'll handle those separately)