fix: false positives from PHP config directives and functions (933120 PL1, 933151 PL2)

[ssigwart]

Closes #3637

[dune73]

Thank you for the PR @ssigwart.

Don't merge just yet. Maybe there are alternatives to simply kicking out the keywords.

[dune73]

I like your update. Did not play around with it so far, but I think this could be a good workaround.

Also: Performance in chained rules is less of an issue since they are only executed if the original rule matches.

[ssigwart]

Thanks. I removed the extra capture.

I've been using this rule on a production site since yesterday and it seems to be working well.

[EsadCetiner]

Is there anything blocking this pr? An alternative has been found to outright kicking out the false positive keywords.

[ssigwart]

@EsadCetiner, I don't think your question was directed towards me, but just in case, I'm not aware of anyone waiting on changes from me on this.

[dune73]

No, this was addressed towards the team, @ssigwart. I removed the label. Ready for review.

[azurit]

We probably don't need to set variable tx.933120_possible_directive and use tx.1 directly. @ssigwart Can you test it?

In the current form of the rule we may simplify the logging with these changes:

• replace %{TX.933120_TX_0} with %{TX.0} in logdata action
• remove line setvar:'tx.933120_tx_0=%{tx.0}',\
• add capture action to the last rule

By the way, we probably can do the similar with rule 933151.

[ssigwart]

@azurit, I updated it. I did noticed that the log message wasn't showing the most useful information, so I added variables to capture the original matched variable name and value.

I'm looking into rule 933151 now. I have to figure out how to trigger it first before I make updates to the rule.

[ssigwart]

I updated rule 933151 too now.

The following will work:
http://localhost:8080/?test=great%20ceiling%20heights%20(9ft)

The following will still trigger rule:
http://localhost:8080/?test=ceiling%20(9ft)

[ssigwart]

@azurit, what do you think of the test failure? It's no longer detecting array_diff foo (, which seems correct since it's not array_diff (.

[azurit]

@azurit, what do you think of the test failure? It's no longer detecting array_diff foo (, which seems correct since it's not array_diff (.

Payload array_diff foo ( definitely should not be part of this rule and i doubt if it's a valid PHP syntax at all.

[azurit]

Test for array_diff foo ( was added 7 years ago by @lifeforms and i believe it's a bug. @ssigwart pls fix it - remove foo.

[azurit]

Hmm, all tests for 933151 should be rechecked, i see multiple problems in there, for example:

• duplicate tests 1 and 6
• test 6 is searching \( found within in the logs but that will never happen as logging was already fixed
• i don't understand why all tests are sending input data (which in few cases are also payloads)

@theseion What do you think about tests for 933151? See above.

[theseion]

I agree, the tests should be fixed. @theMiddleBlue did a huge clean up of other tests that had the same issue (body data + URI). I think it's copy pasta. The tests should also be properly separated into URI and body data tests. And test 6 should be dropped.

[ssigwart]

I updated the tests, so they're all passing now.

[azurit]

Looks good. I suggest you to add yourself into the author field of the tests header (lifeforms, ssigwart).

[ssigwart]

Looks good. I suggest you to add yourself into the author field of the tests header (lifeforms, ssigwart).

Thanks. I updated the author field.

[azurit]

LGTM!

@Xhoenix Can you merge this one? Thanks.