Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optionally nocanon #1031

Closed

Conversation

mariodsantana
Copy link

[Resubmission of pull #961 after rebasing for more alternatives.]

These two small patches fix issues I'm having when trying to do some tricky proxy actions. For example if I do:

SecDefaultAction "phase:2,log,setvar:SESSION.badness=1,proxy:'http://192.168.190.12%{REQUEST_URI_RAW}"

I've been applying them to new releases without issue for my use case. I posted them a while back as "issues" but they never got answered there? Submitting as a pull request now in case that's the preferred method. The fixes are:

  1. when executing disruptive actions, it's necessary to execute the intercept action as well. I don't think this will break anything, but it certainly fixes this scenario where mod_proxy doesn't expand the REQUEST_URI_RAW macro. Maybe we should only do this for proxy actions?
  2. give the user the option not to "canonicalize" the proxy URL - if we want to send the URI unchanged to the backend server to whom we are proxying, specify "[nocanon]" as the first characters in the target URL.

@@ -2513,6 +2513,11 @@ static void msre_perform_disruptive_actions(modsec_rec *msr, msre_rule *rule,
}
}
}
if (actionset->intercept_action_rec->metadata->type == ACTION_DISRUPTIVE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

"proxy-nocanon" should be checked here, right?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think so...? Here's my understanding:

The loop before this line looks through all the actions in actionset->actions and run the execute callback for each. For some reason, though, it doesn't seem like the actionset->intercept_action_rec is part of the list in actionset->actions -- so I added a few lines to check whether actionset->intercept_action_rec is an intercept action, and if so to run the execute callback for it.

In other words, this particular change doesn't have anything to do with no/canon behavior. It just allows the "execute" callback of the modsec's glue into mod-proxy to run, allowing mod-proxy to do some setup work - in my particular case it allows mod-proxy to expand macros in preparation for the proxy request.

That's my understanding, anyway!

Copy link
Contributor

Choose a reason for hiding this comment

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

I've got your point. You are right.

@zimmerle
Copy link
Contributor

zimmerle commented Jan 4, 2016

Hi @mariodsantana,

Thank you for the patch. I made some comments inline in the code.

@uxbod
Copy link

uxbod commented Jan 4, 2016

Hello and a Happy New Year!

Am running NGINX 1.9.9 with the nginx_refactored branch in a proxy configuration. For some reason, most probably configuration, the following rule never appears to get applied:

SecRule REMOTE_ADDR "@pmFromFile /etc/asl/whitelist" phase:1,pass,t:none,id:318743,nolog,skipAfter:END_RBL

Even though I have added the IP to the whitelist file it is still hitting this rule:

SecRule REQUEST_HEADERS:Host "^[\d.:]+$"
"chain,phase:2,rev:2,t:none,pass,msg:'Atomicorp.com WAF Rules: Suspicious activity detected - Host header is a numeric IP address', severity:'2',id:'331032',severity:'5',tag:'no_ar'"
SecRule REMOTE_ADDR "!@ipMatch 127.0.0.1,::1" "t:none"

Any thoughts as to why its being missed in phase 1 ?

Thanks, Phil

@mariodsantana
Copy link
Author

Thanks for the feedback, @zimmerle - I've responded to your comments.

@zimmerle
Copy link
Contributor

zimmerle commented Jan 6, 2016

Hi @mariodsantana,

Thank you again for you path.

Your patch is now on our buildbots. Made a small modification, as you can see here:
c711808

@zimmerle
Copy link
Contributor

zimmerle commented Jan 6, 2016

@zimmerle
Copy link
Contributor

zimmerle commented Jan 6, 2016

Merged! It will be released as part of ModSecurity v2.9.1

@mariodsantana
Copy link
Author

Thanks!
On Jan 6, 2016 5:50 AM, "Felipe Zimmerle" [email protected] wrote:

Merged! It will be released as part of ModSecurity v2.9.1


Reply to this email directly or view it on GitHub
#1031 (comment)
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants