chat on slack

airween [7:04 PM]
here is an issue:
https://github.com/SpiderLabs/ModSecurity/issues/1960

and a possible solution:
https://github.com/airween/ModSecurity/tree/v3/issue-1960
GitHub
SecRuleEngine ignore DetectionOnly · Issue #1960 · SpiderLabs/ModSecurity
Describe the bug it seems that the latest v3/master completely ignores the DetectionOnly SecRuleEngine configuration. When a rule match, I get always the default disruptive action even if SecRuleEn...
GitHub
airween/ModSecurity
ModSecurity is an open source, cross platform web application firewall (WAF) engine for Apache, IIS and Nginx that is developed by Trustwave's SpiderLabs. It has a robust event-based programmin...

should we discuss about?

victorhora [2:14 PM]
hey @airween :slightly_smiling_face:
Thanks for your contribution there
I did a very quick review and looks good
My only initial concern was if the value of `it->status` is expected to be filled later on and it won’t be if `DetectionOnlyRuleEngine` is set...
I didn’t checked this fully but that’s the only thing that comes in my mind now hehe :slightly_smiling_face:
Can you submit a pull request to trigger the tests on the buildbots? :slightly_smiling_face:

airween [2:51 PM]
hi @victorhora, thanks for feedback
yes, may be you're right: the `it->status` must be filled, but if you review the code, the assignment evaluated only if the action is disruptive - if not, this condition doesn't run
https://github.com/airween/ModSecurity/blob/a2e9ae1fa721693c2c1fdc47b245670556dc2352/src/transaction.cc#L1325-L1342
GitHub
airween/ModSecurity
ModSecurity is an open source, cross platform web application firewall (WAF) engine for Apache, IIS and Nginx that is developed by Trustwave's SpiderLabs. It has a robust event-based programmin...
my problem is which value need to assign the `it->status`? 200?

airween [3:00 PM]
I just informed from the comment:
     ```/**
      * Rules will be evaluated but it won't generate any disruptive action.
      *
      */
DetectionOnlyRuleEngine,```

airween [5:08 PM]
ok, I've modified this part, if the SecRuleEngine is DetectOnly, then the default status code in case of disruptive actions is 200.
PR sent, buildbot started :slightly_smiling_face:

victorhora [6:14 PM]
hey @airween!
Sorry my delay
nice!
Well, actually I was bringing the question if we may have issues with it->status being empty… I’m not sure if it was a problem actually hehe
but hey, your PR looks good and it should fix the issue for sure :slightly_smiling_face:
I was just wondering if the value shouldn’t be empty or if it’s fine as it was with your initial change
Need to check to be sure

airween [6:19 PM]
right

victorhora [6:19 PM]
Because thinking again, forcing it->status to 200 and if it the user was requesting a page that doesn’t exist, Nginx should return 404, even if DetectionOnly is set, right?

airween [6:19 PM]
I didn't find any initial value...

victorhora [6:20 PM]
let me have another look

airween [6:20 PM]
but if the rule checked in phase 1 or 2, modsec doesn't know yet what will the response...
200 or 404 or 5XX

victorhora [6:21 PM]
indeed
I wonder how the Nginx will behave for sure
@zimmerle what do you think? :slightly_smiling_face:

airween [6:24 PM]
or
may be we should catch it inside of disruptive actions body
https://github.com/airween/ModSecurity/blob/a2e9ae1fa721693c2c1fdc47b245670556dc2352/src/actions/disruptive/drop.cc#L31-L38
GitHub
airween/ModSecurity
ModSecurity is an open source, cross platform web application firewall (WAF) engine for Apache, IIS and Nginx that is developed by Trustwave's SpiderLabs. It has a robust event-based programmin...

victorhora [6:27 PM]
indeed, that’s also a possibility

airween [6:27 PM]
    ```if (transaction->m_it.status == 200) {
        transaction->m_it.status = 403;
}```
replace this to
    ```if (transaction->getRuleEngineState() != RulesProperties::DetectionOnlyRuleEngine) {
        if (transaction->m_it.status == 200) {
            transaction->m_it.status = 403;
        }
}```

victorhora [6:28 PM]
Can you add that as a comment to the PR?

airween [6:28 PM]
which one? :slightly_smiling_face:

victorhora [6:28 PM]
Then we can discuss with @zimmerle and decide the best course of action for this one
The one you just sent
:slightly_smiling_face:
As an alternative solution

airween [6:29 PM]
well, right :slightly_smiling_face:
but keep the PR as it is?

victorhora [6:29 PM]
Both should fix the issue IMO, we just need to decide the best course of action there

airween [6:29 PM]
sure

victorhora [6:29 PM]
yup :slightly_smiling_face:

airween [6:29 PM]
right

victorhora [6:30 PM]
Thanks! :smile:

airween [6:34 PM]
https://github.com/SpiderLabs/ModSecurity/pull/2031#issuecomment-466555782
GitHub
V3/issue 1960 by airween · Pull Request #2031 · SpiderLabs/ModSecurity
Fixed #1960.
anyway, this was the first solution... :slightly_smiling_face:
but I've dropped it for some reason... :slightly_smiling_face:
okay, waiting for @zimmerle to discuss it
there are some other issues :stuck_out_tongue:

zimmerle [9:55 AM]
moin )
reading

zimmerle [10:01 AM]
replied to a thread:

We need to have to be kindly on that "parse" the idea is not to stick to the RFC, but as wide as possible without changing the cookie content. With that aprrocah we will be able to catch something that the backend application understands as a valid cookie, therefore avoiding evasion.
That is the main rationally; we have to understand if all modifications, follow this paradigm. I will check your code.

zimmerle [10:07 AM]
@airween: ModSecurity has a state machine that defines the work status: Detection, Block, Disable. By changing the code to add a default value to the intervention, you may create a loose state where it is enabled but not working. That may be an impediment for a runtime rule change the status back to Enable. What about the optimization for whenever modsec is disabled? are the logging callbacks not being called on this new state?
i would prefer to understand a little bit further why it is acting in such a fashion. Is likely to have a bug somewhere

airween [10:15 AM]
hi @zimmerle :slightly_smiling_face:
thanks for feedbacks
and wait for your suggestions
I think my (new) cookie parsing method stands closer than previously, but check it

zimmerle [10:19 AM]
sorry for de delay. I will look into it
rush here

airween [10:19 AM]
(I mean it's closer to the expected result than the previous, and closer to that you described above, like the backends interprets) (edited) 
no problem :slightly_smiling_face:

airween [10:36 AM]
answer to the second part (issue #1960): I think I didn't added the default value to the invention, moreover I hide it when the SecRuleEngine is DetectOnly. That's what we discussed with @victorhora may be that's not the good solution. Anyway, the regression tests are passed with this modification.

airween [5:36 AM]
hi @zimmerle and @victorhora how do you like this solution: https://github.com/SpiderLabs/ModSecurity/pull/2032
for this issue https://github.com/SpiderLabs/ModSecurity/issues/1960?

This is an another way, and I think we can avoid the unnecessary false responses.
GitHub
Fixed bug described in issue-1960, another solution by airween · Pull Request #2032 · SpiderLabs/ModSecurity
Fixed #1960. Another solution.
GitHub
SecRuleEngine ignore DetectionOnly · Issue #1960 · SpiderLabs/ModSecurity
Describe the bug it seems that the latest v3/master completely ignores the DetectionOnly SecRuleEngine configuration. When a rule match, I get always the default disruptive action even if SecRuleEn...

airween [5:49 AM]
for the cookie parsing: additional help to understand, what I think about it

https://pastebin.com/Q1mS0DRR
Pastebin
[C++] /*   * cookie parsing algorithms for ModSecurity3   *    * g++ -O0 -g cookiepars - Pastebin.com
```g++ -O0 -g cookieparse.cpp -o cookieparse
./cookieparse "  key1=  foo ; key2=bar=apple;  key3 = peach,orange;  key 4  = banane ; key5"
  key1=  foo ; key2=bar=apple;  key3 = peach,orange;  key 4  = banane ; key5
^     ^       ^    ^    ^      ^              ^        ^                    
======
  key1=  foo ; key2=bar=apple;  key3 = peach,orange;  key 4  = banane ; key5
  ^    ^       ^    ^           ^     ^               ^       ^         ^```   
the markers shows how catch the algorithms the keys and values. In case of current method, the extremist format of a cookie string, like in the example the keys and values are confused. But with the new version, all interpreted as well (I think - I compared them with Apache (PHP) and Python Bottle).

victorhora [10:09 PM]
Haven’t seen the cookie parsing, but had a quick look on PR #2032. I think the solution looks elegant, but I’m wondering we may have problems if we use disruptive actions other than “block” (e.g. deny).

The thing I’m still curious to understand is which change-set lead to that issue in the first place. :confused:

airween [4:26 AM]
if you see the regression test:
https://raw.githubusercontent.com/SpiderLabs/ModSecurity/e17f37404c20332060347b31c241b09b19bb2959/test/test-cases/regression/issue-1960.json

you can see, I always use `deny`. I'll consult with theMiddle, he is the reporter of this issue, may be he has some idea about the test cases.

victorhora [6:07 PM]
Hey @airween!
ah indeed!

airween [6:09 PM]
hi @victorhora! :slightly_smiling_face:

victorhora [6:09 PM]
But I’m not sure since the rule for the chain is using “block” action
SecRule ARGS \“@rx exec(?:\\s|)\\(\” \“id:1,phase:1,log,t:none,t:lowercase,t:cmdLine,block,msg:‘PHP exec function call’\“”
So I can’t recall the behaviour, since I see that you use “deny” on the SecDefaultAction

airween [6:10 PM]
(trying to understand... :slightly_smiling_face: )

victorhora [6:10 PM]
So it would be good to test using “deny” directly (without relying  on the SecDefaultAction)
basically to test this, your test case would only need to have two rules:
“SecRuleEngine DetectionOnly”,
“SecRule ARGS \“@rx exec(?:\\s|)\\(\” \“id:1,phase:1,log,t:none,t:lowercase,t:cmdLine,deny,msg:‘PHP exec function call’\“”
and instead of the rule ID 1 having “block” it would have “deny”

airween [6:12 PM]
and what is the expected behaviour?
ah, I see what do you think! :slightly_smiling_face:

victorhora [6:12 PM]
since it’s DetectOnly, it should just log I think :slightly_smiling_face:

airween [6:12 PM]
yes

victorhora [6:12 PM]
But can’t tell for sure unti l test hehe

airween [6:13 PM]
and how does it works now?
ah :smile:

victorhora [6:13 PM]
something that crossed my mind

airween [6:13 PM]
no problem :slightly_smiling_face:

victorhora [6:13 PM]
I’ll test and let you know

airween [6:13 PM]
I'll check it soon, finishing a mail

victorhora [6:14 PM]
@zimmerle is working on major refactoring for a number of things so he’s a bit away

airween [6:14 PM]
right, sounds good

victorhora [6:15 PM]
And I’m similarly busy with other things here :disappointed: But I promise I’ll have a look at it on the weekend or Monday :slightly_smiling_face:
Thanks for your patience and contributions, they are very welcome :slightly_smiling_face:

airween [6:15 PM]
you're welcome :slightly_smiling_face:
I'm really enjoying to do this

victorhora [6:16 PM]
feel free to more if you have other observations as well

airween [6:16 PM]
sure, I've many other ideas... :smile:

victorhora [6:16 PM]
Yeah, let’s make ModSec great! :smile:

airween [6:17 PM]
but first I'ld like to "align" ModSec3 that it works as ModSec2 with CRS
I mean the ModSec3 runs all regression tests as PASSED as ModSec2 (with Apache, of course)

victorhora [6:18 PM]
yeah, that’s something we’ve been looking to do as well. But it’s tricky since sometimes we have to introduce quirks/bugs from ModSec2 in order to behave the same
So it’s really tricky sometimes :confused:

airween [6:18 PM]
there aren't so much back
I've started from about 800 FAILED test... now they're about 36 I guess
and I think it's just 4, may be 5 big "group"

victorhora [6:19 PM]
wow, wasn’t aware it was that many hehe

airween [6:20 PM]
meantime I working on Apache connector too
but I think that will be a bigger work, some failed tests comes from there

victorhora [6:20 PM]
I should have time to look into the cookie one next week as well

airween [6:20 PM]
right

victorhora [6:20 PM]
ah yeah, Apache Connector is missing some stuff to be considered stable

airween [6:20 PM]
thanks
at this week I've started to read and learn the Apache API... :stuck_out_tongue:

victorhora [6:21 PM]
Hopefully we can have version 1.0 sometime this year :slightly_smiling_face:

airween [6:21 PM]
would be good
this test case doesn't work
sorry
it works :slightly_smiling_face:
https://pastebin.com/P2706Xxs
line 81
Pastebin
[JSON] [    {      "enabled":1,      "version_min":300000,      "title":"Testing setvar - Pastebin.com
   ```"expected":{
      "http_code":200
    },
    "rules":[
      "SecRuleEngine DetectionOnly",
      "SecRule ARGS \"@rx exec(?:\\s|)\\(\" \"id:1,phase:1,log,t:none,t:lowercase,t:cmdLine,deny,msg:'PHP exec function call'\""
    ]```
yeah, with `SecRuleEngine DetectOnly` suppress the deny and block actions
but
if I changed the `DetectOnly` to `on`, then the block doesn't catches the request, the response will 200
is that right?
https://github.com/SpiderLabs/ModSecurity/wiki/Reference-Manual-(v2.x)#block

```Description: Performs the disruptive action defined by the previous SecDefaultAction.``` 
GitHub
SpiderLabs/ModSecurity
ModSecurity is an open source, cross platform web application firewall (WAF) engine for Apache, IIS and Nginx that is developed by Trustwave's SpiderLabs. It has a robust event-based programmin...
block depends on the SecDefaultAction
so I think then the result is right - am I right?

victorhora [8:23 PM]
hi @airween, I’m not sure if I follow what you mean
> yeah, with `SecRuleEngine DetectOnly` suppress the deny and block actions

you mean that you tested that “SecRuleEngine DetectOnly” suppression of the engine is working both for “block” and “deny” actions?
> if I changed the `DetectOnly` to `on`, then the block doesn’t catches the request, the response will 200

> block depends on the SecDefaultAction

yup, regardless of the SecRuleEngine configuration (On/DetectOnly) “block” relies on whatever is defined on SecDefaultAction. If SecDefaultAction is not defined then it would not deny or log (depending on how log directives are defined)

airween [4:01 AM]
_you mean that you tested that “SecRuleEngine DetectOnly” suppression of the engine is working both for “block” and “deny” actions?_ - yes
as I remember, I've extended the regression test - I'll check it, and push it

airween [5:45 PM]
I just pushed the regression test what we've talked about yesterday
https://github.com/SpiderLabs/ModSecurity/pull/2032/files#diff-cb547f0c5fcdd82423e1fab734b6b5b9
GitHub
Fixed bug described in issue-1960, another solution by airween · Pull Request #2032 · SpiderLabs/ModSecurity
Fixed #1960. Another solution.
https://github.com/SpiderLabs/ModSecurity/pull/2032/files#diff-cb547f0c5fcdd82423e1fab734b6b5b9R114

https://github.com/SpiderLabs/ModSecurity/pull/2032/files#diff-cb547f0c5fcdd82423e1fab734b6b5b9R151
GitHub
Fixed bug described in issue-1960, another solution by airween · Pull Request #2032 · SpiderLabs/ModSecurity
Fixed #1960. Another solution.
GitHub
Fixed bug described in issue-1960, another solution by airween · Pull Request #2032 · SpiderLabs/ModSecurity
Fixed #1960. Another solution.
https://github.com/SpiderLabs/ModSecurity/pull/2032/files#diff-cb547f0c5fcdd82423e1fab734b6b5b9R188
GitHub
Fixed bug described in issue-1960, another solution by airween · Pull Request #2032 · SpiderLabs/ModSecurity
Fixed #1960. Another solution.