Add missing urldecodeuni operations

[csjperon]

Missed from previous PR #578

[csjperon]

Note that I intentionally skipped 920230 See the tail end of the discussion on #578

[dune73]

Good catch. Thanks for the pull.

Tested this an it looks good to me.

One thing for future PRs: Would you mind giving your branches a different name than the one in the official repo? (-> diffing v3.0.0-rc2 against v3.0.0-rc2 is tricky).

[dune73]

Tested. Looks good.

(trying out new github functionality with these formal reviews)

[csjperon]

Ah yeah.. apologies about the branch name

[dune73]

No worries. If I was not such a git n00b, I would know how to do it despite the branch name. ;)

[dune73]

Any opposition to merging?

[lifeforms]

@dune73 Unfortunately I haven't been able to follow this issue closely due to other commitments. It's quite a big change, but as I said earlier to you, I'm not fully up to date on the guidelines around urlDecodeUni so I may not be qualified to comment.

If I'm correct, does it mean that on Apache, the payloads will now be url-decoded twice before scanning (once by Apache, once by CRS), but on other web servers it will happen once (only by CRS)? Could this difference not allow for evasions, since we will scan a twice-decoded version but the Apache backend will be passed it url-decoded once? This sounds scary to me. It could allow %xx / %uxxxx sequences to disappear into another character. But I haven't been able to think of evasions from the top of my head, since % is not often a magic character in other contexts that I know of.

My preference would be to ensure that ModSecurity passes us the same input on all server types, so if the input to the rules differ, shouldn't it be a ModSecurity fix rather than a CRS workaround that might impact the biggest platform (Apache)?

But, I'm jumping into this very late, as I haven't been able to follow the discussion leading up to the PR, for which I apologize.

[csanders-git]

This is true -- I believe anything in Phase 2 will automatically be decoded already and as a result it will be decoded twice potentially allowing bypasses, that is a nasty problem. @lifeforms

[csanders-git]

I'm not sure we can make this change as a result of what @lifeforms bought up. We should either note in our documentation that ARGS is expected to be urldecoded or say that this change only has support in something like modsec v3 (@zimmerle take note this decision affects you)

In any event for the time being we should probably put this off for discussion for a bit and push it to 3.1 if consensus can't be reached as it doesn't affect the core platforms.

[dune73]

This is a painful topic.

Would this mean we need to roll back PR #578 and clean out all the cases where this has been applied "traditionally"?

[dune73]

We have not really come to a conclusion here, I think. With the 1st decode-patch merged and the 2nd one not merged, we are in a fairly unclean situation.

Will we really leave this as is?

I mean we can actually postpone a fix to 3.0.1 if we want to.

[csanders-git]

I think we have to do something like that. We need to experiment with trying to get access to ARGS in non-decoded form.