fix: response body (based on #326)

[airween]

This patch is based on #326 but there was a problem: if Nginx uses any custom error page, then because of the the internal redirect a new transaction started and it generated an audit.log entry with empty messages field. See this commit.

I found PR #273 by @liudongmiao but that pull request was very old, and I wasn't able to build the module with that. I picked up the modifications and added to the base PR.

Credits to: @liudongmiao, @g00g1.

@Dr-Lazarus-V2, @g00g1, @liudongmiao, @jeremyjpj0916 - guys please could you test this branch?

Suggested test cases:

• Nginx as proxy
• Nginx as standalone HTTP server
• use custom error pages
• without custom error pages
• circumstances above with different phases (use CRS/custom rules to check REQUEST_URI (rules with phase:1 but Nginx calls msc_process_uri(), REQUEST_HEADERS (phase:1), eg ARGS (phase2), etc...) (with response 403))

Feel free to ask here or on Slack.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
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

[jeremyjpj0916]

Does this fix losing the context of the tx due to nginx internal redirects(POSTS all recorded as GETs etc.)? If so thats a good first step in the right direction.

[airween]

Does this fix losing the context of the tx due to nginx internal redirects(POSTS all recorded as GETs etc.)? If so thats a good first step in the right direction.

Yes, it does. The base PR (#326) didn't do that but I could add #273 (see my initial commit) which does. Both two PR's are necessary. I also faced the duplicated tx in case of internal redirect, this is why I started to research.

[Dr-Lazarus-V2]

Will have a look and test this out!

[Dr-Lazarus-V2]

Hey I have done some initial testing, it appears to work as intended.

[liudongmiao]

@airween For ngx_http_modsecurity_get_module_ctx part, I'd suggest to use with the original realip part, which checks r->internal || r->filter_finalize too.

When I use it with the old modsecurity, I don't know why I removed the internal and flter_finalize part.

https://github.com/nginx/nginx/blob/476d6526b2e8297025c608425f4cad07b4f65990/src/http/modules/ngx_http_realip_module.c#L544-L568

[airween]

Hi @liudongmiao,

@airween For ngx_http_modsecurity_get_module_ctx part, I'd suggest to use with the original realip part, which checks r->internal || r->filter_finalize too.

When I use it with the old modsecurity, I don't know why I removed the internal and flter_finalize part.

https://github.com/nginx/nginx/blob/476d6526b2e8297025c608425f4cad07b4f65990/src/http/modules/ngx_http_realip_module.c#L544-L568

thanks for suggestion - you mean here I should replace that function with your suggestion?

Could you send a review with your solution?

[airween]

@liudongmiao ping.

[liudongmiao]

@airween Yes, I suggest you use original version from nginx, and check whether use nginx version or my version.

[liudongmiao]

@airween However, my patched version works well since the pr I maked 2 years ago.

[airween]

@airween Yes, I suggest you use original version from nginx, and check whether use nginx version or my version.

Thanks - just to clarify: you mean we should add the extra condition to check if the ctx is null or not, right? So now I copied from your solution this:

 ctx = ngx_http_get_module_ctx(r, ngx_http_modsecurity_module); if (ctx == NULL) {

but we should change to this:

 ctx = ngx_http_get_module_ctx(r, ngx_http_modsecurity_module); if (ctx == NULL && (r->internal || r->filter_finalize)) {

It would be nice to know what the r->filter_finalize does, because Nginx's documentation does not explains that (r->internal is covered in the docs).

[liudongmiao]

@airween In my old cases, I just want to get context, ignore any nginx's internal status.

You should check it carefully.

[airween]

@airween In my old cases, I just want to get context, ignore any nginx's internal status.

You should check it carefully.

Thanks - then I keep it as it is. We can add other conditions if everyone has an issue.

Thanks for all the help guys, merging now.