ModSecurity-nginx icon indicating copy to clipboard operation
ModSecurity-nginx copied to clipboard

Fix error_page internal redirect false positive logging

Open jeremyjpj0916 opened this issue 5 years ago • 9 comments

Fixes: #182

Problem here was error_page redirects don't maintain original client request data. Hence ModSecurity has false information when evaluating the clients HTTP request a second time around during the request(HTTP Method Header is always reset to GET, URI is also not the original from client request). It is also inefficient to evaluate the request data a second time around after the internal redirect, hence cutting out those phases the second time around after the redirect makes sense and is a performance improvement.

To figure it out I was reviewing prior commits, specifically:

https://github.com/SpiderLabs/ModSecurity-nginx/commit/ce1d438bc665c28853eb5b429aad515d42027919

Which regarding this commit did indeed ensure the log phase still runs during error_page internal redirect, but just had not considered the implications of re-processing the NGX request phases(phases prior to the proxy_pass directive for instance) again.

Currently running these changes in our dev and stage environments personally. Will be pushing to our prod in the next week or so, will run as a fork until this is merged.

jeremyjpj0916 avatar May 22 '20 07:05 jeremyjpj0916

@zimmerle @martinhsv @defanator @victorhora Any thoughts here?

Should be good to #ShipIt

jeremyjpj0916 avatar May 28 '20 17:05 jeremyjpj0916

still pending? oof.

jeremyjpj0916 avatar Jun 21 '20 06:06 jeremyjpj0916

@defanator Can you have a look at this?

zimmerle avatar Jul 03 '20 12:07 zimmerle

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

github-actions[bot] avatar Aug 03 '20 00:08 github-actions[bot]

Github bot being bad again 💀.

jeremyjpj0916 avatar Aug 14 '20 01:08 jeremyjpj0916

Can say I have ran this in my fork of code for a long time 10/10 does indeed work 😆 .

jeremyjpj0916 avatar Sep 08 '20 06:09 jeremyjpj0916

@jeremyjpj0916 copying here as well - it is believed that https://github.com/SpiderLabs/ModSecurity-nginx/commit/4f26b48998db5119ca818b0909b6b14e08ebb544 should resolve at least some of observed misbehaviours reported in #182.

If it doesn't, I would appreciate a test case demonstrating the issue with minimal configuration involved.

defanator avatar Feb 18 '21 07:02 defanator