Discussion of the new XML processing feature

[airween]

Describe the bug

It's not a bug but a discussion about a new feature, how can we extend the XML processing.

There is a feature request from a customer that we should extend the engines' XML parsing capability. Of course, we should add this request to both engine with same behavior.

Current behavior

Consider this payload:

<?xml version="1.0" encoding="UTF-8"?>
<root>
  <level1>
    <level2>
      <node>foo1</node>
      <node>bar1</node>
    </level2>
    <level2>
      <node>foo2</node>
      <node>bar2</node>
    </level2>
  </level1>
</root>

This payload will appear in current state in the engines:

(mod_security2)

[/post][9] Target value: "  foo1  bar1  foo2  bar2"

(libmodsecurity3)

[/post] [9] Target value: "  foo1  bar1  foo2  bar2" (Variable: XML:/*)

(lines from debug.logs)

Problem

The problem is that exclusions for sub-parts and specific nodes does not work. See the example:

SecRule XML:/* "@rx ^foo.*" \
	"id:10001,\
	phase:2,\
	t:none,\
	log,\
	pass,\
	ctl:ruleRemoveTargetById=930120;XML:/level1/level2/node"

because the XML variable holds the concatenated node values, not a key:value pairs like JSON. Therefore it's impossible to create any exclusion against any rules.

Possible solution

Consider this converted strcture (XML to JSON):

{
  "root": {
    "level1": {
      "level2": [
        {
          "node": [
            "foo1",
            "bar1"
          ]
        },
        {
          "node": [
            "foo2",
            "bar2"
          ]
        }
      ]
    }
  }
}

This payload will expanded like this:

(mod_security2)

[/post][9] Adding JSON argument 'root.level1.level2.level2.node.node' with value 'foo1'
[/post][9] Adding JSON argument 'root.level1.level2.level2.node.node' with value 'bar1'
[/post][9] Adding JSON argument 'root.level1.level2.level2.node.node' with value 'foo2'
[/post][9] Adding JSON argument 'root.level1.level2.level2.node.node' with value 'bar2'

(libmodsecurity3)

[/post] [4] Adding request argument (JSON): name "json.root.level1.level2.array_0.node.array_0", value "foo1"
[/post] [4] Adding request argument (JSON): name "json.root.level1.level2.array_0.node.array_1", value "bar1"
[/post] [4] Adding request argument (JSON): name "json.root.level1.level2.array_1.node.array_0", value "foo2"
[/post] [4] Adding request argument (JSON): name "json.root.level1.level2.array_1.node.array_1", value "bar2"

The idea is to transform the XML structure in a similar way.

Example:

(libmodsecurity3)

[/post] [4] Adding request argument (XML): name "xml.root.level1.level2.array_0.node.array_0", value "foo1"
[/post] [4] Adding request argument (XML): name "xml.root.level1.level2.array_0.node.array_1", value "bar1"
[/post] [4] Adding request argument (XML): name "xml.root.level1.level2.array_1.node.array_0", value "foo2"
[/post] [4] Adding request argument (XML): name "xml.root.level1.level2.array_1.node.array_1", value "bar2"

Possible risks

• if we introduce this "new" collection under an existing one, then it will causes false positive matches
• cost of parsing an XML structure is very high

How can we avoid/handle the risks?

We can put the decision in the hands of the user, whether he wants to see the new collection under the ARGS or not - so introduce a new configuration keyword, eg. SecParseXMLintoArgs (consider the optional runtime config, eg. ctl:parseXMLintoArgs)

As in case of JSON, introduce a new configuration keyword which controls the maximum number of XML levels that can be analyzed, eg. SecRequestBodyXMLDepthLimit (see SecRequestBodyJSONDepthLimit)

More todo's

We have to:

• analyze XML parser performance
  • should we change from libxml2 to another parser? Libexpat? Or other?
• check the effect of SecArgumentsLimit in case of JSON parsing
• design and apply this behavior on XML parsing
• explore the possibility of additional XML validation methods (eg. XXE (XML External Entity) detection)
• to decide the issue of compatibility or uniform behavior within versions

For the last item: the behavior of JSON parsing in two versions are different. Consider the payload {"a":1,"b":[{"a1":"a1val"},{"a1":"a2val"}]} (see that there is a list!) which is equivalent with this XML:

<?xml version="1.0" encoding="UTF-8"?>
<root>
    <a>1</a>
    <b>
        <element>
            <a1>a1val</a1>
        </element>
        <element>
            <a1>a2val</a1>
        </element>
    </b>
</root>

which produces these results:

(mod_security2)

[/post][9] Adding JSON argument 'a' with value '1'
[/post][9] Adding JSON argument 'b.b.a1' with value 'a1val'
[/post][9] Adding JSON argument 'b.b.a1' with value 'a2val'

(libmodsecurity3)

[/post] [4] Adding request argument (JSON): name "json.a", value "1"
[/post] [4] Adding request argument (JSON): name "json.b.array_0.a1", value "a1val"
[/post] [4] Adding request argument (JSON): name "json.b.array_1.a1", value "a2val"

Note, that please check the list items with the same keys! I think we should follow the libmodsecurity3's behavior - but the the XML and JSON won't be compatible. (Which implies the next question: do we want to align the mod_security2's behavior?)

Any feedback are welcome!

[theseion]

XML parser

does ModSecurity use the libxml2 SAX parser? When I read SecRequestBodyXMLLimit it sounds to me like it's not. Presuming that ModSecurity could collect all elements that rules might want to inspect (which would rule out the use of sub-tree wild cards), ModSecurity could simply keep track of those elements and discard everything else, only a single pass would be needed and no extra memory than what is needed to store the elements that might be inspected.

Wild cards

One very frustrating thing at the moment is that JSON targets can't be matched using wild cards. For example, if I wanted to match / exclude attribute from all nodes, I couldn't write json.level1.*.attribute but would have to to list every combination. Listing every possible combination may actually not be possible at all. This is not only a convenience issue but can also make it easy to bypass rules.

JSON

while the argument names in libmodsecurity3 are much better than in v2, the numbering of list elements is a potential problem for when you want to match any element in a potentially large list. A rule would need exclude the complete array or enumerate every entry in the list, which isn't possible. This will always give attackers a way to bypass such a rule by simply placing the payload at the first index that isn't being inspected.

General

I suggest focusing on one feature at a time. For me, that would currently be giving the user the full ability to inspect XML contents and nodes. Other features can be added later on. IMO, one really good feature is often much more valuable than many of mediocre quality.

[marcstern]

mod_security2 uses https://gitlab.gnome.org/GNOME/libxml2/

[marcstern]

On the long term, it looks logical to have the same behaviour in v2 & v3, and also with XML & JSON.
We have to manage that path (compatibility), and also to choose the best features.
One question that need to be raised: do we consider XML as a priority? Some people still need it, obviously, but how many? Does it worth the effort on the long term? Ther's no judgement here, just an open question.

[marcstern]

Remark about v2 JSON parsing: you also have arrays appearing in the ARGS names, but without index, like "json.b.array.a1".
My 2 cents: from a security point of view, it's better than having an index as the order of the arrays makes no sense for a parser (and I don't think you can guarantee the order).

[marcstern]

About limiting the parsing: This could be a gain in case of huge payload.
We may want to parse only some items or exclude some.

[marcstern]

About Wildcards in JSON: regex in targets is definitely a must

[marcstern]

Another feature asked by many people is the possibility to parse a JSON from a custom variable (like the value of a cookie, maybe after base64-decode - yes, everybody thinks about JWT).
This feature should probably be discussed from scratch when designing a new parsing (but, as @theseion said, features should be implemented one at a time).

[marcstern]

About Wildcards in JSON: regex in targets is definitely a must
Btw, it's possible to create a rule to match the targets and add them one by one.
It's far from optimal, but it works.

[airween]

XML parser

does ModSecurity use the libxml2 SAX parser?

yes. Both version uses libxml2.

When I read SecRequestBodyXMLLimit it sounds to me like it's not.

Sorry, but I don't understand this. Where is the SecRequestBodyXMLLimit keyword? I can't find it either in the documentation or in my initial comment.

Presuming that ModSecurity could collect all elements that rules might want to inspect

yes - the question is here that what ModSecurity might want to inspect? Eg. is it necessary the tags' arguments?

Wild cards

One very frustrating thing at the moment is that JSON targets can't be matched using wild cards. For example, if I wanted to match / exclude attribute from all nodes, I couldn't write json.level1.*.attribute

Why? This works for me:

SecRule ARGS:/level1\.*/ "@rx foobar" \
    "id:1007,\
    phase:2,\
    pass,\
    log,\
    msg:'Phase 1: Parameters: %{MATCHED_VAR_NAME} %{MATCHED_VAR}'"

the request:

curl -H "Content-Type: application/json" -d '{"level1":{"key1":{"arg":"foobar"},"key2":{"arg":"foobar"}},"level2":{"key1":{"arg":"foobar"},"key2":{"arg":"foobar"}}}' http://localhost/post

then I see in the log:

ModSecurity: Warning. Pattern match "foobar" at ARGS:level1.key1.arg. [file "/home/airween/src/coreruleset/rules/REQUEST-900-EXCLUSION-RULES-BEFORE-CRS.conf"] [line "236"] [id "1007"] [msg "Phase 1: Parameters: ARGS:level1.key1.arg foobar"] [hostname "localhost"] [uri "/post"] [unique_id "ZoLG_jDbBliqNtRf162A5QAAAAA"]
ModSecurity: Warning. Pattern match "foobar" at ARGS:level1.key2.arg. [file "/home/airween/src/coreruleset/rules/REQUEST-900-EXCLUSION-RULES-BEFORE-CRS.conf"] [line "236"] [id "1007"] [msg "Phase 1: Parameters: ARGS:level1.key2.arg foobar"] [hostname "localhost"] [uri "/post"] [unique_id "ZoLG_jDbBliqNtRf162A5QAAAAA"]

but don't see the args with name level2.....

May be this is something you need.

JSON

while the argument names in libmodsecurity3 are much better than in v2, the numbering of list elements is a potential problem for when you want to match any element in a potentially large list. A rule would need exclude the complete array or enumerate every entry in the list, which isn't possible. This will always give attackers a way to bypass such a rule by simply placing the payload at the first index that isn't being inspected.

Ahm, I think I see what you mean. Needs to figure out the correct handling.

General

I suggest focusing on one feature at a time. For me, that would currently be giving the user the full ability to inspect XML contents and nodes.

In case of content do you think about the whole XML raw content?

Now the demand is that we need the XML's key:value pairs, especially generate exclusions. (Regarding to nodes.)

Other features can be added later on. IMO, one really good feature is often much more valuable than many of mediocre quality.

Sorry, what "other features" do you think about?

[airween]

One question that need to be raised: do we consider XML as a priority? Some people still need it, obviously, but how many? Does it worth the effort on the long term? Ther's no judgement here, just an open question.

I don't know how many users expects it - a feature request received in private, which is quite urgent, but they use the mainline mod_security2, so we can't provide it as a "custom feature", because later that would lead collisions.

But yes, your question is legal - this is not the most priority, but regarding the circumstances, we can't avoid that.

[theseion]

Sorry, but I don't understand this. Where is the SecRequestBodyXMLLimit keyword? I can't find it either in the documentation or in my initial comment.

I meant to write SecRequestBodyXMLDepthLimit

[theseion]

Wild cards
One very frustrating thing at the moment is that JSON targets can't be matched using wild cards. For example, if I wanted to match / exclude attribute from all nodes, I couldn't write json.level1.*.attribute

Why? This works for me:

It works with @rx but not with ctl:removeTargetById, AFAIK.

[airween]

Remark about v2 JSON parsing: you also have arrays appearing in the ARGS names, but without index, like "json.b.array.a1". My 2 cents: from a security point of view, it's better than having an index as the order of the arrays makes no sense for a parser (and I don't think you can guarantee the order).

Okay, pro: security reason, con: can't reference to an exact item.

This is why I started this discussion to collect pros and cons for functionalities.

[airween]

Wild cards
One very frustrating thing at the moment is that JSON targets can't be matched using wild cards. For example, if I wanted to match / exclude attribute from all nodes, I couldn't write json.level1.*.attribute
Why? This works for me:

It works with @rx but not with ctl:removeTargetById, AFAIK.

Ah, I see - thanks.

[theseion]

In case of content do you think about the whole XML raw content?

Not raw necessarily. I just think that being able to inspect values and nodes properly has to be the most important thing. Raw sounds expensive, maybe that's not necessary.

[theseion]

Sorry, what "other features" do you think about?

You wrote additional validation, for example.

[airween]

I meant to write SecRequestBodyXMLDepthLimit

Thanks. But why do you think that reading this keyword it sounds to you like ModSecurity does not use libXML?

[theseion]

SAX, not libxml.

[theseion]

libxml2 has a SAX parser, so XML can be streamed. For streams, a depth limit doesn't make much sense (I think).

[airween]

In case of content do you think about the whole XML raw content?

Not raw necessarily. I just think that being able to inspect values and nodes properly has to be the most important thing. Raw sounds expensive, maybe that's not necessary.

Raw appears in REQUEST_BODY (by a bug in libmodsecurity3, and perhaps you can force it with ctl:forceRequestBodyVariable in v2).

But for your idea: could you write an example?

[airween]

Another feature asked by many people is the possibility to parse a JSON from a custom variable (like the value of a cookie, maybe after base64-decode - yes, everybody thinks about JWT). This feature should probably be discussed from scratch when designing a new parsing (but, as @theseion said, features should be implemented one at a time).

This seems to me like a new transformation - may be we should open a new discussion for that.

[theseion]

Tree based XML parser:

tree := parse(xmlString)
for (node in nodeWalk(tree)) {
  // eval rule
}

Stream based XML parser (SAX):

stream := parseStreaming(xmlString)
while (!stream.atEnd()) {
  node := stream.next()
  if (anyRuleNeedsNode(node) {
    cachedNodes.add(node)
  }
}
...
// eval rules using cachedNodes

Examples of what access could look like

Access any node below a given level with a certain name:

ctl:removeTargetById=999999;ARGS:xml.level1.*.myNode

Access any attribute below a given level with a certain name:

ctl:removeTargetById=999999;ARGS:xmlAttributes.level1.*.myAttribute

Access to prelude:

SecRule ARGS:XML_PRELUDE "@rx DOCTYPE"

Access to trailer:

SecRule ARGS:XML_TRAILER "@rx NOT PART OF MY XML.*"

In short, raw access is probably a good idea, but there should be other options, as applying a regular expression to a huge string is never a good idea. Usually, the scope can be reduced significantly, provided that it's possible to access everything.

[marcstern]

Remark about v2 JSON parsing: you also have arrays appearing in the ARGS names, but without index, like "json.b.array.a1". My 2 cents: from a security point of view, it's better than having an index as the order of the arrays makes no sense for a parser (and I don't think you can guarantee the order).

Okay, pro: security reason, con: can't reference to an exact item.

This is why I started this discussion to collect pros and cons for functionalities.

Why would you want check the second item only, knowing that I could evade your check by switching both items?

[airween]

Why would you want check the second item only, knowing that I could evade your check by switching both items?

I don't want to check only the second item - I would expect that the engine check all items in the list.

[airween]

Tree based XML parser:

...
Stream based XML parser (SAX):

These are called in libxml2 DOM parser and SAX parser.

May be we should inspect the efficiency and performance of both parsers.

Examples of what access could look like

Access any node below a given level with a certain name:

ctl:removeTargetById=999999;ARGS:xml.level1.*.myNode

Access any attribute below a given level with a certain name:

ctl:removeTargetById=999999;ARGS:xmlAttributes.level1.*.myAttribute

Ah, I see, so with these prefixing we can divide the the nodes' key:pair list but also we can access to attributes. Thanks for the idea, we should take a look.

Access to prelude:

SecRule ARGS:XML_PRELUDE "@rx DOCTYPE"

Access to trailer:

SecRule ARGS:XML_TRAILER "@rx NOT PART OF MY XML.*"

In short, raw access is probably a good idea, but there should be other options, as applying a regular expression to a huge string is never a good idea. Usually, the scope can be reduced significantly, provided that it's possible to access everything.

Right. But first, as you wrote, we should start with one function, then we can continue with the others.

[airween]

I made a small example which could be the base of future XML parser. The parser uses libxml2's SAX parser, the newest version (v2 - the old methods and structures are deprecated).

Please take a look at that: https://github.com/airween/saxparser_example

You should try that with other XML's, and make investigations with Valgrind or another memory testers.

Any feedback is welcome.

[airween]

Unfortunately there wasn't any comment, so here is my plan:

• I implement extended XML processing, meaning all XML nodes can appear under the ARGS collection
• Also the keys can appear under ARGS_NAMES collection
• It seems like in case of JSON the ARGS_POST is not filled so XML behavior will follow this
• there will be a new directive: SecParseXMLintoArgs with possible values On and Off - Off will be the default value
• in case of On the nodes and their values will be available as I described above
• ctl:parseXMLintoArgs will be also available to control this feature under runtime
• SecArgumentsLimit will be considered; if the number of nodes is more than this value, also the REQUBODY_ERROR will be set to 1

Possible feature:

• we discussed above that it does not necessary to inspect the depth (because SAX is a stream parser), but I think it can be a useful feature, eg. for back ends - SecRequestBodyXMLDepthLimit can be implemented. The default value can be 0, which means there is no limit. In other case, it the value reaches the limit REQBODY_ERROR will be set to 1.

[marcstern]

Remark about v2 JSON parsing: you also have arrays appearing in the ARGS names, but without index, like "json.b.array.a1". My 2 cents: from a security point of view, it's better than having an index as the order of the arrays makes no sense for a parser (and I don't think you can guarantee the order).

Okay, pro: security reason, con: can't reference to an exact item.
This is why I started this discussion to collect pros and cons for functionalities.

Why would you want check the second item only, knowing that I could evade your check by switching both items?

So the syntax without index is better. In v2, json.b.array.a1 will match all keys named "a1" in the array.

[airween]

So the syntax without index is better. In v2, json.b.array.a1 will match all keys named "a1" in the array.

I can't decide if it is better or not, but the fact is that we can't solve indexing with SAX.

[theseion]

Don't you think it's possible to abuse SecArgumentsLimit? Depending on the implementation, you could hide the attack values at the end of a large XML, or hide other args (query, post). We don't have a rule, AFAIK, that blocks on REQBODY_ERROR in CRS, so it would be pretty simple to bypass the engine.
I'm aware this wouldn't be a new issue, as the same would hold true for JSON processing.

Could SecParseXMLintoArgs take a regular expression to somehow filter what would end up in the ARGS collection? That might help save resources and reduce processing time when checking ARGS.

[airween]

Don't you think it's possible to abuse SecArgumentsLimit? Depending on the implementation, you could hide the attack values at the end of a large XML, or hide other args (query, post). We don't have a rule, AFAIK, that blocks on REQBODY_ERROR in CRS, so it would be pretty simple to bypass the engine. I'm aware this wouldn't be a new issue, as the same would hold true for JSON processing.

The default modsecurity.conf file has a rule that handles this situation, so I don't think there is a way around it using this method.

Could SecParseXMLintoArgs take a regular expression to somehow filter what would end up in the ARGS collection? That might help save resources and reduce processing time when checking ARGS.

It could but I think that would be a risk, I mean if the rule writer wants to control what arguments could be processed, then the bypass is more possible. Anyway, if there will be a demand later, we can add this feature.

[marcstern]

Could SecParseXMLintoArgs take a regular expression to somehow filter what would end up in the ARGS collection? That might help save resources and reduce processing time when checking ARGS.

It could but I think that would be a risk, I mean if the rule writer wants to control what arguments could be processed, then the bypass is more possible. Anyway, if there will be a demand later, we can add this feature.

I propose to have this discussion in a separate thread and to extend it to all ARGS. There are very good arguments about white-listing/black-listing ARGS parsing, even for other other encodings. This should probably be discussed with other SecLang projects.

[airween]

A new idea came up during development, which does not modify the original plan but extends it.

Consider you want to parse the XML nodes into ARGS, but you don't need XML target anymore.

With the original concept you have to create an exclusion to turn off XML target (and avoid the possible FP's - which was the original intention).

It would be easy to add a third option to SecParseXMLintoArgs (a side note - would be better SecParseXmlIntoArgs?), so the possible values:

• Off (default) - nothing changes
• On - engine will parse the XML nodes into ARGS collection with prefix xml and keeps the XML target's behavior
• OnlyArgs - engine will parse the XML nodes into ARGS collection with prefix xml AND WON'T FILL XML target's

Please share your ideas.

[RedXanadu]

The OnlyArgs option is a good idea.

Assuming that a user has CRS (or some other rule set) deployed then if they're using the OnlyArgs option they won't need to remember to disable the vanilla XML target, e.g. with a ctl:ruleRemoveTargetByTag=paranoia-level;XML action. That makes it slightly simpler.

With OnlyArgs leaving the vanilla XML target/collection empty at the engine level that will save on the execution time of having a ctl:ruleRemoveTargetByTag action. That makes it slightly faster.

SecParseXmlIntoArgs would match the case (upper/lower) of the pre-existing directive SecXmlExternalEntity. It makes sense to continue with that established form of capitalisation.

[dune73]

I like this proposal too.

Here is another thing, I am not sure we thought about. With XML you can have args in elements, but you can also have them in attributes. See https://www.w3schools.com/xml/xml_attributes.asp

I think CRS covers both with the XML:/*. How about this transfer into ARGS?

[airween]

I think CRS covers both with the XML:/*. How about this transfer into ARGS?

@theseion already hinted at it here.

Access any attribute below a given level with a certain name:

ctl:removeTargetById=999999;ARGS:xmlAttributes.level1.*.myAttribute

So using ARGS:xmlAttributes prefix may be would be useful. But first I would like to finish the XML nodes (for both engine), and then we can start to make the plans :)