SecRequestBodyAccess off skips the phase:2 rules #2465
Comments
|
Sorry to say, but same behavior in case of Test case here. Problem is similar: the function returns by true if the body access is off, the evaluation won't called. |
|
Hmm. I guess I can understand why it was coded that way (and some might even challenge whether it's a bug at all). Given that phase:2 occurs at 'request body has been received', and one does not want to access the request body, it's not immediately obvious why one would write a rule to occur at phase:2. I.e. if your rule is checking request headers, why not make it a phase:1 rule? For example, this would seem to be the case with the cited rule 913100. On the other hand, I can see at least some cases where this would be a problem -- particularly with generic rule sets. For example, suppose a generic rule is intended to perform some detection against ARGS (both query args and post args), and so it is set up as a phase:2 rule. But a particular administrator is not interested in examining the request body and so sets SecResponseBodyAccess to off. In this case use of the rule will have been inadvertently turned off for query args as well. |
|
You hit the nail on the head with your reasoning why most rules are written in phase 2. A quick peek at the Trustwave rule set tells me that it's over 99% of the rules. The ratio is better with CRS, but this is a serious problem and I hope you can fix this soon. |
|
Thanks for quick response - I just followed the mod_security2 behavior. While it works as I wrote that expected, I assume the v3 should follow that way - or make a separated documentation, how the new version works. Anyway, using of |
|
|
Not executing the rules seems to be a little too radical for my taste. On the other hand, I understand the value of not running it for performance reasons and other points cited below. Furthermore, it is easy to maintain the rules by having the execute-ever approach, given that it will allow cross-phases variables in a single Rule. The cross-phase variables is controversial. Although it facilitates the development of the rules, oftentimes it leads to circumstances where:
Those points aren't issues on Apache, at least last time I checked, but they are issues on Nginx. Owing to it treats the request on this streaming fashion. The too late to block is the factor that drives me to think towards a situation that ModSecurity should be warning the user that the rule may not be doing what the user hopes for, at least, not in the time that he expects. Together with the effort to support intermediate phases, this could give us a good ground for new features. Version 2 has some counterpoints on when to execute the phase:1 via the |
|
Another use case: when the variable is a collection name, eg. I think this is justified, because the rule counts scores in the REQUEST phases, and it must to wait the end of these phases. |
|
@zimmerle: I think there is nothing wrong with giving the user a way to skip phases completely. It just should not be implicit and undocumented. If the option exists, it should be via a dedicated directive. I also like the idea of ModSecurity warning about rules not doing what people expect. The traditional phase:1 rule with ARGS_POST is silently ignored and that is very taxing on newbies. Personally, I would not mind a 500 in such a situation as the rule is simply broken, but that's a question of taste. What is not clear to me following your response: How do you intend to handle this? With |
The last rule in a phase does not necessarily be the first rule of the subsequent phase. It could be the last in the current phase. Order-wise this is equivalent - Notice that in the former you are taking an action when the headers may be already delivered to the end application. In other words: actionB happens a phase before actionA. |
|
Some facts were somewhat related to that issue - The recommended configuration file, with the description of why we think it is quintessential to have Request Body Inspection On: One that may be writing the rules is reading the logs that state - The commit which makes the behavior different from v2: 379f370 The issue that propelled the change in question #1531 Reverting 379f370 will make the behavior works is a similar fashion of v2. Not yet recommended. As a long term plan, we aim to remove those configurations. We could leave for the engine to fill the variables based on the utilization. If not used, there will be automatically disabled. |
|
I do not think you should remove configuration options. But that's for a long term discussion. Near term: Will you revert 379f370 or how do you plan to fix this until the long term solution pans out? |
|
Any news on this front or a plan to share? Infos from CRS: We merged a PR (-> 1941) that shifts all the CRS rules that can run in phase:1 into phase:1. This allows ModSecurity v3 users to mitigate this security vulnerability by running the development version of CRS (v3.4/dev) and edit the rule 949110 to run in phase 1. We are now preparing a minor release (likely 3.3.1) that will allow blocking in phase 1 as a configuration option. |
|
Nothing substantial to report yet. This matter is currently under active discussion. |
|
Could we be part of the discussion, please? This issue affects our users, so we would like to chime in too. @zimmerle told us several times to report issues here on github so things could be discussed in public. If you discuss this behind closed doors now, this would be a contraction. |
|
Update from the OWASP ModSecurity Core Rule Set project:
https://github.com/coreruleset/coreruleset With these two changes, we provide you with a workaround for this libModSecurity3 security issue that will let every attacker bypass your WAF when you have disabled request body access. We plan to backport and release CRS 3.3.1 in the coming days. Please check https://coreruleset.org, the CRS mailinglist or back here for infos when this happened. I have not heard any information about the Trustwave Spiderlabs ModSecurity rules (SLR). My estimation is that disabling request body access leads to a bypass of almost all the SLR rules. So a fix is due. In the meantime, try and enable request body access if you are affected from the vulnerability. |
|
Sec[Request|Response]BodyAccess is no longer necessary. Back in the day, it was essential to save computational resources. With progress, ModSecurity managed to have the parser smart enough to figure with any variable that uses request|response body is in use; if so, turn on the processing automatically. 😍 Considering how smart the parser is nowadays, it is natural to deprecate those outdated configuration flags. Moreover, mitigating the doubt on the semantic of the configuration. Namely: this issue, #1531, #1886, #2273, #2467, owasp-modsecurity/ModSecurity-nginx#124, 379f370, #1643, 42a472a, owasp-modsecurity/ModSecurity-nginx#84, and owasp-modsecurity/ModSecurity-nginx#104. One may argue in favor of having a configuration SecForce[Request|Response]BodyAccess Off. Particularly to address not-so-elegant rules that make unnecessary usage of variables that do not intend 🙃. Undoubtedly a fair point. The phase will be processed as long as there is a rule to be addressed in that particular phase. Nonetheless, on the occasion of SecForce[Request|Response]BodyAccess development, we may want to raise this subject again. The PoC with Sec[Request|Response]BodyAccess marked as deprecated is already implemented and under test here: ae128ad As usual, this is open for appraisal. I will be delighted to discuss the technical matter. |
Do you think this isn't necessary in these days? Or why do you classify this as an argument?
The point is here the automatically. I'm not sure this is a good idea (I mean to deprive users of the right to decide).
Sorry, I don't share your opinion. It definitely isn't natural for most users (just see the mentioned issues in initial comment). These "flags" (like Sec[Request|Response]BodyAccess) allows to users to make more flexible the engine. I mean if these "flags" will disappear, and a user wants to check the performance of the built WAF (eg. turn off the body inspect), the only chance is to turn off the affected rules (which are on phase:2/phase4) - or am I wrong?
Okay, I see you've committed the modifications which marked as deprecated. The other part of this idea (automatically processing based on the used variables) is already done? Or is there any plan? |
I am afraid I did not paraphrase this. Maybe, however, disable SecRequestBodyAccess is not essential to many users.
Perhaps I did not make myself clear. I am saying that the engine will enable such configuration as long as the user demands it. Looking at the illustration below, because the user has used REQUEST_BODY, the engine recognizes that he/she wants to examine the request body; therefore, the engine enables the body processing. Before the purposed changeAfter
I am sorry to say, but you are not right. There are other manners for not proceed with the inspection of the request body, for instance: Straight from the modsecurity.conf-recommended file - Regardless, considering a smoother alternative, we have already suggested the existence of For clearness, you may want to expatiate why the enable/disable of the request body weighs a performance test. What is the subject of such a test, performance of what?
I am not sure what is your understanding by done; The PoC is implemented and published in the link I have shared. I am sure there is space for embellishment and improvements in the code. Scrutiny is always welcomed. |
|
Hello, @zimmerle
Does this normal way to use it? As far as I understand this construction just disable processing of any rule that checks request body. But other rules on phase 2 going to be works fine? My humble opinion, if you interested (from user point of view, as I hope).
I think this a great. But seems like mostly can be used by ruleset developers, because they decide use REQUEST_BODY or not. You already provided them early. So this part mostly about why this question is shown up. |
I'm afraid we moved a little away from the original issue. First, please help me to clarify your terminology. From your point of view, body processing == whole phase:2? Then take a look to this rule: This is a non-commercial rule from Comodo, especially for ModSecurity-nginx combo. There are tons of like this, also like in other commercial and/or non-commercial rule sets (I mean: mixed variables from phase:1/2, and rule triggered in phase:2). Based on your example below, the new workflow will realize that the rule contains the But what can a user does if he/she wants to disable the processing of request body? Important: disable the processing of request body, not the whole phase:2! Why users want to disable the processing of req body? Please read the mentioned issues:
How do you know this? What about the users who wants to disable it?
Yes, I saw this, but could you help me to clarify, what's the difference between the new and old "flags"? |
|
Just one note - please take a look to this regression test: No SecRequestBodyAccess directive regression test As you can see, in the configuration section there isn't Now see the output (the expected result is 200, because the request body processing isn't enabled, and rule processed in phase:2). Hmmm... this definitely looks like if the (You can change the variable by I've checked this behavior on my Nginx instance, with same result. (Could you check and confirm that libmodsecurity3 has a default value with I don't know what's the expected behavior, the mod_security2 follows the opposite way: if the variable is not set explicit as But if this is the libmodsecurity3 expected behavior, then I don't see the point why you want to remove the directive in the next versions, and why do you wants to add a new one with same behavior. |
|
@zimmerle I get the feeling the proposed change / solution for this regression is trying to do things in an intransparent way, kind of outsmarting the user. That usually leads to a disruption of existing use cases. I prefer an engine to have an explicit way to configure it. Even more an engine that has ambiguous variables like ARGS that can be stuffed in phase 1 but also in phase 2. Under the line I do not see much benefit in the proposed change, but new problems. The directives |
SecRequestBodyLimit allows ModSecurity to verify the request body size. If the size is above a threshold, SecRequestBodyLimitAction will be taken. I don't recommend SecRequestBodyLimit as a replacement for SecRequestBodyAccess. The behavior of SecForce[Request|Response]BodyAccess is a discussion to be held be on the occasion of its development, as mentioned here: [1].
Your opinion really matters. I would like to understand why the proposed SecForce[Request|Response]BodyAccess does not address that behavior.
REQUEST_BODY was a merely example of a variable that makes usage of the request body. Together with REQUEST_BODY, we have others variables that depends on the request body such as:
All those variable depends on the request body to have its value filled. If any of those variables are in use, then request body inspection comes naturally enabled. Otherwise, disable by SecForceRequestBodyAccess Off. In practice, we are not removing functionality but adding the capability of not processing the request body if it is not in use by a rule; That wasn't possible before that proposal. That change is good for usability and performance. |
In essence, this may be related to the use case of the user who loaded a set of rules and later decides to disable those rules due to having SecRequestBodyAccess to Off. I raise that use case scenario here: [1]. To address that niche use, I also have suggested the existence of SecForceRequestBodyAccess. Did you had considered using SecForceRequestBodyAccess in such a case? Performance-wise I would recommend you have a separate file for the rules that depends on the request body; only load this file whenever you have the intention to use it, as oppose to load and disable. |
Actually (as I wrote in my next comment, the SecRequestBodyAccess is default On, if there isn't present in config file) I don't see the difference between the suggested SecForceRequestBodyAccess and the current one. |
I prefer to stick with the technical facts and concrete examples; this proposal suggests configuration labels/semantic a bit more consistent for the end-user. Let's exemplify using use case scenarios - Before this proposal (The use case A)Request Body ProcessedRequest Body ProcessedRequest Body Processed (or not, up to the branch in question. see [3])Request Body NOT ProcessedAfter this proposal (The use case B)Request Body ProcessedRequest Body NOT ProcessedRequest Body NOT ProcessedPerhaps the example would help to show the benefit clearly. There were some concerns about possible limitations introduced by the deliberation of the use case B made at [2], specifically on explicit-ability, debugging limitations and possible new issues. @dune73, Do you mind describing a bit why do you think those will be an issue? Even better if you could illustrate via examples. |
I am afraid that this is not accurate. You may want to have a look at the illustration on [4], specifically on the Item 2 of the use case B. |
|
Thank you @martinhsv. That clarification is dearly welcome. Things and the path forward may look clear for you developers, but for us it's more of an Enigma. Can you share a roadmap when you will be implementing / releasing the fix that will no longer skip a phase with existing rules? |
Thanks! Now there is only one thing left: clarify the fate of variable I still don't see the difference between the old and the new variable - see this table:
May be I forgot something (then please correct me), but looks like the behaviors of these variables are same (except that now the So I don't see the reason to remove the |
|
Hi @airween , I'll try to reiterate and rephrase what @zimmerle has described above as it pertains to your latest inqury ... On the surface it may look like SecRequestBodyAccess is being replaced with identical functionality, merely under a different name (e.g. 'SecForceRequestBodyAccess Off', or 'SecDisableRequestBody On'). However, there are a couple of differences:
Under the current proposal, the internal switch to 'access request body' has an initial state of false. It then gets set to true if any of the rules in the rule set use any variables that may be populated by the request body. So, when you say of the configuration item 'not set' == 'Allow access to req. variables', you are correct in that merely having a request variable present itself triggers the request body processing. In practice, almost all rule sets will trigger this 'access_request_body == true' state -- even if the only request-body-populated-variables present are some of the basic ones included in modsecurity.conf-recommended (e.g. REQBODY_ERROR, ...) Is there an advantage to this, as opposed to simply defining that the initial 'access request body' state is always true? Perhaps very little advantage if the request body were the only consideration. However, this same approach is being proposed for the response body, and here the advantages are more tangible. First, it is expected to be a more common ModSecurity deployment to ignore the response body content than to ignore the request body. Second, it is a more foreseeable scenario that someone might indeed have no response-body-dependent variables in their rule set -- in which case they could benefit automatically without having to set a separate configuration item to 'ignore response body'. If we accept that this approach offers some value at least for response bodies, then there is some advantage to taking a common approach for both cases (request bodies and response bodies).
There may still be value in being able force this setting to be 'off'. For one thing, with even a small-ish set of rules, it could be nuisance to comb through one's rules to identify that last, overlooked rule that is still, inadvertently using a variable populated by body content. Granted, one could achieve much the same effect with the BodyLimit configuration items, but it was felt that a simpler mechanism to simply turn off body processing might be seen as helpful to users. However, given how (1) above describes how we deduce whether body access should be 'on', there may not be much of a case for forcing body access to be 'on'. If that assumption is correct, then not being able to force 'on' would be a notable difference compared to what the pre-existing config items support. Should we retain the SecRequestBodyAccess and SecResponseBodyAccess names? Given (1) and (2) above, the semantics of the control are slightly (but perhaps sufficiently) different to warrant configuration items with new names. Note that if we do proceed with only allowing body processing to be forced 'off', then depending upon the name chosen, there may be no need for a parameter to be associated with the configuration item name (e.g. 'SecDisableRequestBody' would completely define what was intended). It's really about trying to make the settings easier to understand for the long term. Retaining the old names might risk creating as much confusion as we seek to alleviate. As always, feedback is welcome. If providing use cases that you think might not be well supported under this proposal, please provide as much of a specific description as possible. |
Hi @dune73, gentlemen, I took a chance to perform a quick testing of CRS v3.3.1-rc1 with the The weird thing is that HTTP response is completely broken when (I may have missed something from the entire discussion here - sorry if I did - just wanted to bring the above to your attention. Cheers!) |
|
Hey @defanator, thank you for trying this out for us. The behavior is indeed quite unexpected - and very much not desired. We could reproduce your problem, but only with an outdated modsec/nginx connector. You write you are running with the latest master. So given our own research, this should not happen in your setup. So could you please confirm, the problem is persistent with latest master ModSec and latest master ModSec->Nginx connector? How about the latest official releases of ModSec and ModSec->Nginx connector. |
|
@dune73 the issue can be 100% reproduced with the following versions of libmodsecurity && nginx connector module: CRS v3.3.1-rc1: diff for CRS: I haven't checked with libmodsecurity 3.0.4 and nginx connector 1.0.1 yet. |
|
Thank you very much @defanator. This helps a lot. If you could check 3.0.4 and 1.0.1 too, that would be neat. For the record: We are investigating this issue and a 2nd unexpected ModSec v3 misbehavior that CRS 3.3.1-RC1 triggers. |
|
hi @defanator,
here are the versions what I'm using: airween@debian-sid:/usr/local/src/DEBIAN/Modsecurity$ git describe
v3.0.4-96-g310cbf89
airween@debian-sid:/usr/local/src/DEBIAN/ModSecurity-nginx $ git describe
v1.0.1-12-g22e53abNote, that I cloned the repository about 8 days ago, but looks like there isn't any relevant modification in that two commits. As you can see in the name of directory path, I'm using Debian, and made packages from sources. I use Debian's Nginx package, which is 1.18 is SID. I installed these packages to an another machine where I test Coreruleset and ModSec, there is a configured CRS: $ git branch | grep ^\*
* v3.3.1/rc1
$ git describe
2.2.7-1991-gf2d4136this is the only difference I found against your config (only relevant lines): $ diff crs-setup.conf.example /etc/modsecurity/crs/crs-setup.conf
311,318c311,318
< #SecAction \
< # "id:900110,\
< # phase:1,\
< # nolog,\
< # pass,\
< # t:none,\
< # setvar:tx.inbound_anomaly_score_threshold=5,\
< # setvar:tx.outbound_anomaly_score_threshold=4"
---
> SecAction \
> "id:900110,\
> phase:1,\
> nolog,\
> pass,\
> t:none,\
> setvar:tx.inbound_anomaly_score_threshold=5,\
> setvar:tx.outbound_anomaly_score_threshold=4"
336,348c335,345
< # Please note that blocking early will hide potential alerts from you. This
< # means that a payload that would appear in an alert in phase 2 (or phase 4)
< # does not get evaluated if the request is being blocked early. So when you
< # disabled blocking early again at some point in the future, then new alerts
< # from phase 2 might pop up.
< #SecAction \
< # "id:900120,\
< # phase:1,\
< # nolog,\
< # pass,\
< # t:none,\
< # setvar:tx.blocking_early=1"
<
---
> # Please note that blocking early will hide potential alerts from you. This means
> # that payload that would appear in an alert in phase 2 (or phase 4) do not get
> # evaluated if the request is being blocked early. So when you disabled
> # blocking early again, then new alerts from phase 2 might pop up.
> SecAction \
> "id:900120,\
> phase:1,\
> nolog,\
> pass,\
> t:none,\
> setvar:tx.blocking_early=1"Looks like Update I've checked without In the error log I got: It's really interesting that there are two lines with message I assume this can cause this behavior - but I don't know the exact reason: looks like the engine blocks the request in phase:1 (action Can you share your error.log? |
|
@airween here's the error log: I was about to collect debug log as well, but nginx fails to start with debug version of the connector module with my current configuration, producing segfault. Will share additional details a bit later. BTW, I'm using reproducible environment from https://github.com/defanator/modsecurity-performance/ for tests. You can adjust CRS version in https://github.com/defanator/modsecurity-performance/blob/master/pillars/versions.sls to |
|
Backtrace from coredump obtained after running Output of the |
|
With the libmodsecurity 3.0.4 and modsecurity-nginx 1.0.1 there's no segfault, but the CRS v3.3.1-rc2 with tx.blocking_early is still behaving incorrectly: nginx debug log: |
|
Hi @defanator,
thanks for sharing this resource, I built your environment and successfully reproduced the behavior.
Just a quick note: I think in this case not the rule set produces this behavior. First of all, this modification in your 102,107d101
<
< error_page 500 502 503 504 /50x.html;
< location = /50x.html {
< root html;
< }
< Thanks to @theMiddleBlue for solution. I think this behavior comes when Nginx uses Let's see a minimalist rule set: The logic of this set is a bit similar to CRS A side note: when I started Nginx with this config, I got this line in the log: which is weird, I don't have 42 local rules - perhaps the reason is the engine counts the loaded rules as many times the I ran your request against this set, and I got same behavior as you described. And I have this lines in the log: As you can see, here are two triggered rules which wants to stop the processing of transactions: the first is the 901111 in phase:1, then the 901131 in phase:3. But after the first rule the connection will not terminate - perhaps the proxy (because you configured this server as proxy) waits for the answer from the upstream - don't know. I also don't know what happens when the second rule triggered - may be it blocks to send the response headers... really don't know. Finally, it's also a big mystery why fixes the But based on these information I think this is a connector bug, not the rule set. |
|
@airween good to see you were able to trigger that as well. I'm not blaming CRS - I admit that the bug could be either in library or in connector module, that's fine. The root cause (as I see it) is that nginx connector tries to do all the processing as early as possible (in the pre-access phase, iirc), and by some unclear reasons it does NOT send response headers in the discussed configuration before starting to send the body (calling Most likely, adding error_page logic produces additional internal redirect which involves sending response headers. I'll try to pick some time to dig into this later. Thanks for your investigations and minimal config. Re: rules number count - yes, the counter increases every time Update: rules counter question was also raised here: defanator/modsecurity-performance#3 |
|
Hi @defanator,
oh, sure - sorry if I was ambiguous.
I'm really happy that you found the root case.
ah, good to know.
you're welcome.
I don't know. As a customer, I'm curious how many rules I have. If a rule loaded twice (I mean loaded for two different contexts), IMHO it's only one rule. But it's not an important question... Should I open an issue under ModSecurity-nginx, or it isn't necessary? |
I don't have any strong opinion here. As you can see from the sources, nginx connector simply counts everything returned by every |
oh, sorry again - I thought about connector problem (to open an issue), not the rule counting. As I wrote, the counting issue isn't an important thing. |
|
@airween yes please - go ahead then! Thanks. |
|
@defanator : Could be very clear what exactly 50fc347 fixes? There are quite a few deviations being discussed here and I'd hate to run into misunderstandings. |
Now on v3/master 🚀 🚀 it was under tests at Workflow: Quality Assurance - 50fc347, QA is happy about it 😆😆😆😆 |
It fixes segfault on starting up nginx with debug version of the ModSecurity-nginx connector module and loaded CRS. |
|
We have published a blog post covering this vulnerability and what users can do about it while there is no official fix. Sorry this took so long, but we ran into several issues while testing our workaround. These issues also include another ModSecurity3 bug (see above). |
Describe the bug
If the
SecRequestBodyAccessisoff, then the engine skips all rules which hasphase:2action.To Reproduce
Here you can find a regression test case.
In the debug log you can see this line:
Expected behavior
As you can see, the request contains an XML payload (CT doesn't count...), and only one (relevant) rule with
id:1. The rule has two variable:REQUEST_URIandREQUEST_BODY. First one available in phase 1, but the second one only in phase 2, so the rule set to phase 2. The request was constructed that both variable matches, but the test returns with 200.The expected behavior is 403, because the config disables only the processing of request body, not the whole phase 2.
If you replace the
phase:2byphase:1in rule 1, then the request will blocked as it expected.Server (please complete the following information):
Additional context
There are two another issues related to this wrong behavior:
#1940 - CRS issue - this helped me to describe this behavior
#2273 - ModSecurity - SecRequestBodyAccess Off doesn't fully disable request body processing
I checked the source, and found a very strange solution to handle the
SecRequestBodyAccess- looks like theappendBody()allocates a buffer even thought the variable set it tooff. In addition, the chosen body processor also woke up, and the payload will parsed. Checking the body access is comes only at this point, that's why the #2273 exists. I assume that users disable the body access to save CPU/memory (as reporter also wrote there), but in this isn't fulfilled.The another strange behavior is if the
SecRequestBodyAccessisoff, then whole phase 2 will ignored (in line 895-911), because the evaluation called later, in L938.Note: I didn't checked, but I suppose the
ctl:requestBodyAccess=offwould give same result.The text was updated successfully, but these errors were encountered: