Skip to content
This repository was archived by the owner on May 14, 2020. It is now read-only.

Conversation

@fgsch
Copy link
Contributor

@fgsch fgsch commented May 31, 2018

Add or remove capture as appropriate.

"id:941100,\
phase:2,\
block,\
capture,\
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure if this is needed or we can replace %{tx.0} with %{matched_var} in setvar:'tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/XSS-%{matched_var_name}=%{tx.0}'"

@csanders-git
Copy link
Contributor

The comment above needs to be tested. @fgsch said that these are mixed throughout but currently there is no harm merging the current content without that.

@csanders-git
Copy link
Contributor

Leaving this open so someone can test it to make sure it works.

@dune73 dune73 self-assigned this Jun 4, 2018
@dune73
Copy link
Contributor

dune73 commented Jun 5, 2018

Responding here to the review question of @fgsch:

Surprisingly, you have to use %{matched_var}:

Here is what the debug log says on apache / modsec 2.9.2:

%{tx.0} (original) : Resolved macro %{tx.0} to: content-type
%{matched_var} : Resolved macro %{matched_var} to: <img src=xss onerror=alert(1)>

I tried to find out where it got the content-type from, but I could not.

I also tested with nginx / modsec 3.0.2. There, both variants work.

Please change to %{matched_var}and I'll merge.

@fgsch
Copy link
Contributor Author

fgsch commented Jun 5, 2018

@dune73 thanks. I will remove the capture and use %{matched_var} instead.

Related to this, I can see many rules using tx.0 in the same context, which is a bit surprising.
Is this something we should review as well in a separate PR?

@fgsch fgsch force-pushed the capture-tidyup branch from 46a4ba6 to 2741281 Compare June 5, 2018 10:07
@fgsch
Copy link
Contributor Author

fgsch commented Jun 5, 2018

I've removed the capture but kept the %{tx.0} to keep this PR focused on the original issue.

We can address the use of %{tx.0} separately.

@dune73
Copy link
Contributor

dune73 commented Jun 5, 2018

Yes, I was puzzled by this relevation too. Actually I wonder if we need this construct at all. It's in so many rules and all I see is eaten ressources. Also plays into reorganizing 980xxx.

@dune73
Copy link
Contributor

dune73 commented Jun 5, 2018

Ready to merge?

@fgsch
Copy link
Contributor Author

fgsch commented Jun 5, 2018

It is from my POV.

@dune73
Copy link
Contributor

dune73 commented Jun 5, 2018

A merge it is then. Thank you for the PR. And please keep this {tx.0} in mind.

@dune73 dune73 merged commit 48f5771 into SpiderLabs:v3.1/dev Jun 5, 2018
@fgsch fgsch deleted the capture-tidyup branch June 5, 2018 12:42
@fgsch fgsch mentioned this pull request Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants