ModSecurity icon indicating copy to clipboard operation
ModSecurity copied to clipboard

append data to variables

Open rcbarnett-zz opened this issue 12 years ago • 21 comments

MODSEC-187: Like adding number to a variable (TX.setvar: var=+1), it would be useful to be able to append a string to a variable (ex: TX.setvar:var=>new_string).

In usual cases, this could oviously be done manually: TX.setvar:var=%{TX.setvar}new_string

But you cannot do the same when you variable name depends on a variable: TX.setvar:var_%{TX.number}=>new_string

rcbarnett-zz avatar Oct 17 '13 20:10 rcbarnett-zz

Original reporter: marcstern

rcbarnett-zz avatar Oct 17 '13 20:10 rcbarnett-zz

rbarnett: How about adding macro expansion to the + setvar action? So you could so something like this -

setvar:tx.foo=+%{matched_var}

So, if tx.foo already had data in it, it would append the MATCHED_VAR data to it. Essentially, we would need to add macro support and make sure that the + and - variables understand the different between adding/subtracting digits vs. appending/removing macro string data.

rcbarnett-zz avatar Oct 17 '13 20:10 rcbarnett-zz

marcstern: Macro expansion is already working inside setvar/setenv... I don't really see the point about expansion, except if you need to move the parsing at run-time instead of config time.

Any way, I think that an explicit "append" operator is safer than a "magic" one determining if the string is a number or not. If I want to append a string, it may contain a number...

rcbarnett-zz avatar Oct 17 '13 20:10 rcbarnett-zz

marcstern: Too bad we have to wait for 3.0, as this functionality can be used to block HTTP parameter pollution attacks :-(

rcbarnett-zz avatar Oct 17 '13 20:10 rcbarnett-zz

bpinto: It can be changed Marc, we are hiring a new devel to the team :) Anyway i will reschedule this. Maybe for some 2.7.x.

rcbarnett-zz avatar Oct 17 '13 20:10 rcbarnett-zz

bpinto: Marc,

This is not clear to me. Could you please give me more details ? maybe some real situations and an example of SecRule.

Thanks

rcbarnett-zz avatar Oct 17 '13 20:10 rcbarnett-zz

marcstern: Here is a (unoptimised) example to detect HPP. We want to concatenate all args with the same name into one. Ex: var=1&var=2 => HPP_var=1,2

Same rule as CRS 960022, but for all args.

This would be something like SecRule ARGS "."
"phase:2,setvar:'TX.HPP_%{MATCHED_VAR_NAME}=>%{TX.HPP_%{MATCHED_VAR_NAME}},%{MATCHED_VAR}'"

Note the double (recursive) macro replacement: %{TX.HPP_%{MATCHED_VAR_NAME}} I don't expect this to be supported. However, by having a concatenation operator similar to the '+' (ex: '>'), we could write it:

setvar:'TX.HPP_%{MATCHED_VAR_NAME}=>,%{MATCHED_VAR}'

rcbarnett-zz avatar Oct 17 '13 20:10 rcbarnett-zz

bpinto: Ok Marc,

I would suggest actions attach and detach

attach:var_name;value detach:var_name;value

or

string-set:var+=value string-set:var-=value

rcbarnett-zz avatar Oct 17 '13 20:10 rcbarnett-zz

marcstern: 1. attach/detach does not sound very intuitive to me

  1. I am not sure about the "detach" feature. Is it to remove something from the string? I do not see the use case.
  2. the string-set syntax is not consistent with the setvar one. "string-setvar:var=+value" would be more in-line or even more intuitive: "appendvar:var=value"
  3. what about simply adapting the current syntax to make it generic (and unambiguous)?
    • adding a number: setvar:var+=value
    • substracting a number: setvar:var-=value
    • appending a string: setvar:var>=value
    • prepending a string: setvar:var<=value
    • possibly others in the future ... "=+" and "=-" would stay for backward compatibility (for some time)

and the same could be done for setenv (2 birds with one stone)

rcbarnett-zz avatar Oct 17 '13 20:10 rcbarnett-zz

bpinto: I like the <= and >=. Sounds good. I will let you know when i have updates about it.

thanks

rcbarnett-zz avatar Oct 17 '13 20:10 rcbarnett-zz

rbarnett: why not keep the same syntax/placement for the modifiers?

  • adding a number: setvar:var=+value
  • substracting a number: setvar:var=-value
  • appending a string: setvar:var=>value
  • prepending a string: setvar:var=<value

rcbarnett-zz avatar Oct 17 '13 20:10 rcbarnett-zz

bpinto: Yes. must use the same logic.

rcbarnett-zz avatar Oct 17 '13 20:10 rcbarnett-zz

marcstern: The idea behind putting the character before the = sign instead of after was because you can always chose the variable name, but not the content. You may really want to set/append/prepend a string containing the + character at the first place. And, the more operator characters you add (currently + - < > but maybe others in the future), the more chance you have to get a conflict with a value you need to use.

I just thing it would be a safer choice, but it is obviously functionally equivalent.

rcbarnett-zz avatar Oct 17 '13 20:10 rcbarnett-zz

Any chance to implement this? Issue #903 is another way to solve HTTP parameter pollution but this could be a very interesting generic feature

marcstern avatar Jun 18 '15 12:06 marcstern

Setvar with with macro expansion should be enough to achieve such functionality. I cannot foresee a use case scenario where a limitation is imposed.

zimmerle avatar Dec 01 '20 13:12 zimmerle

From my example above: SecRule ... "setvar:'TX.HPP_%{MATCHED_VAR_NAME}=%{TX.HPP_%{MATCHED_VAR_NAME}},%{MATCHED_VAR}'" This would require double (recursive) macro replacement: %{TX.HPP_%{MATCHED_VAR_NAME}} This obviously doesn't work. Note that the first part works (TX.HPP_%{MATCHED_VAR_NAME}=).

However, by having a concatenation operator similar to the '+' (ex: '>'), we could write it: setvar:'TX.HPP_%{MATCHED_VAR_NAME}=>,%{MATCHED_VAR}'

marcstern avatar Dec 01 '20 17:12 marcstern

From my example above: SecRule ... "setvar:'TX.HPP_%{MATCHED_VAR_NAME}=%{TX.HPP_%{MATCHED_VAR_NAME}},%{MATCHED_VAR}'" This would require double (recursive) macro replacement: %{TX.HPP_%{MATCHED_VAR_NAME}} This obviously doesn't work. Note that the first part works (TX.HPP_%{MATCHED_VAR_NAME}=).

However, by having a concatenation operator similar to the '+' (ex: '>'), we could write it: setvar:'TX.HPP_%{MATCHED_VAR_NAME}=>,%{MATCHED_VAR}'

What is the use case for setting a key based in a variable, not the value?

zimmerle avatar Dec 01 '20 23:12 zimmerle

From my example above: SecRule ... "setvar:'TX.HPP_%{MATCHED_VAR_NAME}=%{TX.HPP_%{MATCHED_VAR_NAME}},%{MATCHED_VAR}'" This would require double (recursive) macro replacement: %{TX.HPP_%{MATCHED_VAR_NAME}} This obviously doesn't work. Note that the first part works (TX.HPP_%{MATCHED_VAR_NAME}=). However, by having a concatenation operator similar to the '+' (ex: '>'), we could write it: setvar:'TX.HPP_%{MATCHED_VAR_NAME}=>,%{MATCHED_VAR}'

What is the use case for setting a key based in a variable, not the value?

@martinhsv pointed me to a valid use case scenario for this one. I understand its value. Working on it.

zimmerle avatar Dec 02 '20 00:12 zimmerle

Implemented at 95dc23a5aa30e659ed312999b45808a393e92e28. Code is on QA. Still need some love. Definitively needs better test cases.

With that: https://github.com/SpiderLabs/ModSecurity/blob/95dc23a5aa30e659ed312999b45808a393e92e28/test/test-cases/regression/action-setvar.json#L30-L34

We expect to have this: https://github.com/SpiderLabs/ModSecurity/blob/95dc23a5aa30e659ed312999b45808a393e92e28/test/test-cases/regression/action-setvar.json#L6-L9

zimmerle avatar Dec 02 '20 00:12 zimmerle

New test cases at b8478b1

zimmerle avatar Dec 02 '20 15:12 zimmerle

That's great. Any chance to have it in 2.9.x?

marcstern avatar Dec 02 '20 17:12 marcstern