feat: improved XMLArgs processing #3363

airween · 2025-04-20T18:53:54Z

what

This PR adds a new feature within XML processing. It's same as the #3358 but for v3.

why

See v2 patch.

references

fzipi

~~I don't see this being the same patch as v2. Here you implemented the extra ctl:parseXMLintoArgs: it is not in v2.~~ My bad, this is indeed implemented.

src/actions/ctl/parse_xml_into_args.cc

fzipi · 2025-04-26T13:08:14Z

src/actions/ctl/parse_xml_into_args.cc

+bool ParseXmlIntoArgs::init(std::string *error) {
+    std::string what(m_parser_payload, 17, m_parser_payload.size() - 17);


17? 🤦 It looks like the lenght of the SecParseXMLIntoArgs, but alone is so arbitrary.

You're definitely right, but I just wanted to follow the conventions (see other files) (even if it's not the prettiest solution). If we want to clean up the code, then we should do that as a sub project.

Does the project want this? It is worth tracking then (e.g. creating a new issue with the subproject?)

Does the project want this?

Not necessarily. But we can live together with this.

Just a few example:

audit_engine.cc

audit_log_parts.cc

request_body_access.cc

rule_egine.cc

...

It is worth tracking then (e.g. creating a new issue with the subproject?)

Sure, it's been like this for almost ten years. Nobody reported, you are the first, so I can't say this has a priority.

Sounds perfect. But (simple) tickets like this might get additional people into the dev side ;)

But (simple) tickets like this might get additional people into the dev side ;)

Amen! :)

src/actions/ctl/parse_xml_into_args.h

fzipi · 2025-04-26T13:12:45Z

src/request_body_processor/xml.cc

+    void MSC_startElement(void *userData,
+        const xmlChar *name,
+        const xmlChar *prefix,
+        const xmlChar *URI,
+        int nb_namespaces,
+        const xmlChar **namespaces,
+        int nb_attributes,
+        int nb_defaulted,
+        const xmlChar **attributes) {


Same here on this declaration. Is this the new standard?

Sorry, could you show a "reference"? What is the "standard"? What is the "old" standard?

The signature of the function is just in the same line, normally.

As I had pointed out in the other PR, there are already several different ways of formatting long function signatures in the code base. This is one of them.

fzipi · 2025-04-26T13:12:56Z

src/request_body_processor/xml.cc

+        void *userData,
+        const xmlChar *name,
+        const xmlChar* prefix,
+        const xmlChar* URI) {


Same as above.

airween · 2025-04-26T18:51:50Z

I don't see this being the same patch as v2. Here you implemented the extra ctl:parseXMLintoArgs: it is not in v2.

It's there.

headers/modsecurity/rules_set_properties.h

src/actions/ctl/parse_xml_into_args.cc

src/parser/seclang-scanner.ll

src/request_body_processor/xml.cc

src/request_body_processor/xml.h

Co-authored-by: Max Leske <[email protected]>

airween · 2025-04-27T18:51:00Z

A few tests were failed after some commit - I have to check the reason.

airween · 2025-04-28T20:55:05Z

I added a few new commits here as at #3358:

bf707de - changed the directive SecXMLintoArgs to SecXmlIntoArgs
e8dc60e - there is a small change in the node value's parsing: if the node value contains a multi-byte character the SAX calls the function multiple times, so we need to concatenate those sub-strings, not creating a new string every time
89442ed - changed the directives in tests and add a new test with a multibyte character

fzipi · 2025-04-29T14:03:07Z

Just out of curiosity, have you tried enabling this and sending a big xml file?

airween · 2025-04-29T15:11:19Z

Just out of curiosity, have you tried enabling this and sending a big xml file?

What do you mean "a big xml file"?

Btw the size of the file is just one thing... There are couple of limits in the engine when you're sending an XML payload and want to process it as ARGS, like:

SecRequestBodyNoFilesLimit - the default value is 128kB
SecArgumentsLimit - default value is 1000

I sent an XML with size > 400kB (I increased the SecRequestBodyNoFilesLimit), then I get:

ModSecurity: XML parsing error: More than 1000 ARGS (GET + XML) [hostname "localhost"] [uri "/post"] [
ModSecurity: Access denied with code 400 (phase 2). Match of "eq 0" against "REQBODY_ERROR" required. [file "/etc/modsecurity/modsecurity.conf"] [line "78"] [id "200002"] [msg "Failed to parse request body."] [data "XML parsing error: More than 1000 ARGS (GET + XML)"] [severity "CRITICAL"] [hostname "localhost"] [uri "/post"]

Actually, the behavior of this function is exactly the same as that used for JSON. There is no difference. You could ask "have you tried enabling this and sending a big JSON file?". But the JSON works as is, and this feature is optional.

If you tell me what you're curious about, I'll try it - with an XML and a JSON too.

fzipi · 2025-04-29T16:09:16Z

Just out of curiosity, have you tried enabling this and sending a big xml file?

The sentence starts with Just out of curiosity. So yes, just curious.

headers/modsecurity/rules_set_properties.h

theseion · 2025-04-30T04:50:34Z

headers/modsecurity/rules_set_properties.h

        m_secXMLExternalEntity(PropertyNotSetConfigBoolean),
+        m_secXMLParseXmlIntoArgs(PropertyNotSetConfigXMLParseXmlIntoArgs),


Suggested change

m_secXMLExternalEntity(PropertyNotSetConfigBoolean),

m_secXMLParseXmlIntoArgs(PropertyNotSetConfigXMLParseXmlIntoArgs),

m_secXmlExternalEntity(PropertyNotSetConfigBoolean),

m_secXmlParseXmlIntoArgs(PropertyNotSetConfigXMLParseXmlIntoArgs),

m_secXMLExternalEntity property is already a member of RulesSetProperties class. If we change here the format, then we must change that every place, eg. in seclang-parser.yy (and other line in the same file), and in few other files.

We could do this, but I'm afraid this wouldn't be a fair to those who use the project in some forked form.

(Consider a company uses libmodsecurity3 with their own modifications, and they follow the mainline source tree from our repository. Consider they uses these (modified) class members, functions that we changed their names. I think this is a modification at a level where at least a subversion number should be changed, I mean 3.0.XX to 3.1.XX - but I'm not sure we have any (other) strict reason to do that).

But because the old syntax is m_SecXML... therefore I followed that syntax in case of new members and functions, so I would keep this (unless we can change Xml to XML in case of second occurrence).

That's fine. I just marked all occurrences for consistency.

theseion · 2025-04-30T04:51:58Z

headers/modsecurity/rules_set_properties.h

+     * The ConfigXMLParseXmlIntoArgs enumerator defines the states for the configuration
+     * XMLParseXmlIntoArgs values.
+     * The default value is PropertyNotSetConfigXMLParseXmlIntoArgs.
+     */
+    enum ConfigXMLParseXmlIntoArgs {
+        TrueConfigXMLParseXmlIntoArgs,
+        FalseConfigXMLParseXmlIntoArgs,
+        OnlyArgsConfigXMLParseXmlIntoArgs,
+        PropertyNotSetConfigXMLParseXmlIntoArgs


Suggested change

* The ConfigXMLParseXmlIntoArgs enumerator defines the states for the configuration

* XMLParseXmlIntoArgs values.

* The default value is PropertyNotSetConfigXMLParseXmlIntoArgs.

*/

enum ConfigXMLParseXmlIntoArgs {

TrueConfigXMLParseXmlIntoArgs,

FalseConfigXMLParseXmlIntoArgs,

OnlyArgsConfigXMLParseXmlIntoArgs,

PropertyNotSetConfigXMLParseXmlIntoArgs

* The ConfigXmlParseXmlIntoArgs enumerator defines the states for the configuration

* XmlParseXmlIntoArgs values.

* The default value is PropertyNotSetConfigXmlParseXmlIntoArgs.

*/

enum ConfigXmlParseXmlIntoArgs {

TrueConfigXmlParseXmlIntoArgs,

FalseConfigXmlParseXmlIntoArgs,

OnlyArgsConfigXmlParseXmlIntoArgs,

PropertyNotSetConfigXmlParseXmlIntoArgs

I can change this, because this enum type is new, but in this case we will deviate from the above-mentioned syntax (m_SecXML...). What do you think?

Personally, I'd use the "new" spelling. I don't think it matters here.

theseion · 2025-04-30T04:54:25Z

src/request_body_processor/xml.cc

+/*
+* XMLNodes for parsing XML into args
+*/
+XMLNodes::XMLNodes(Transaction *transaction) 


Suggested change

XMLNodes::XMLNodes(Transaction *transaction)

XmlNodes::XmlNodes(Transaction *transaction)

I can change this syntax too because it's new, but the existing code uses XML everywhere. I think my syntax matches with that better.

theseion · 2025-04-30T04:55:11Z

src/request_body_processor/xml.cc

+        handler->onCharacters(userData, ch, len);
+    }
+}
+
 XML::XML(Transaction *transaction)


Suggested change

XML::XML(Transaction *transaction)

Xml::Xml(Transaction *transaction)

Same as I wrote here. This is an existing code, the XML class and its constructor is already exists. It wouldn't be fair to change (and if we change we must align all places in the code).

airween · 2025-05-01T20:23:11Z

The sentence starts with Just out of curiosity. So yes, just curious.

I saw, but honestly: your question is absolutely legitimate. That's why I'm asking you: what should we expect with what size of XML, and/or how many nodes of XML?

I wrote a small script which helps to generate different payloads in XML and JSON - but with same content. I tried them, and the problem is not in the conversion (JSON to ARGS and XML to ARGS), but in the rules' processing. With the same structure in XML and JSON, I got the same ARGS list in the engine, but above 1000 of built ARGS performance deteriorates sharply, and rules which check ARGS and ARGS_NAMES will be very slow. Now I want to add some other features to that script, eg. generate x-www-form-urlencoded payload, and try that too.

Which is clearly visible: the critical value is not the size of the file, but number of nodes (both in XML and in JSON).

And of course: without any rules there is no any performance issue even with a huge number of node JSON/XML file - indeed the aim is not to forget the rules, but that's also a good question with which rules should we test?

Co-authored-by: Max Leske <[email protected]>

sonarqubecloud · 2025-05-01T20:32:38Z

Quality Gate passed

Issues
74 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.3% Duplication on New Code

See analysis details on SonarQube Cloud

theseion · 2025-05-02T05:00:00Z

I saw, but honestly: your question is absolutely legitimate. That's why I'm asking you: what should we expect with what size of XML, and/or how many nodes of XML?

Unfortunately, this depends heavily on the installation. With SOAP, for example, you'll probably process 50-100 nodes per request, with some requests sending much larger XMLs. I don't know whether we can do much more than document the performance given different numbers of nodes and let users decide how to configure the engine.

And of course: without any rules there is no any performance issue even with a huge number of node JSON/XML file - indeed the aim is not to forget the rules, but that's also a good question with which rules should we test?

You could use rules with @unconditionalMatch and multiMatch, one each for ARGS, ARGS_NAMES, and XML. This should give you worst case performance for each target.

airween added 4 commits April 20, 2025 18:21

Finish XMLArgs processing in v3

9e41a53

Added new cc and h files

f62de58

Fix cppcheck errors

8ae8374

Fix more cppcheck warning

b42602f

airween added the 3.x Related to ModSecurity version 3.x label Apr 20, 2025

airween added 2 commits April 20, 2025 21:10

Add nullptr check conditions

3e95614

Add nullptr check conditions

029684c

airween requested review from theseion and fzipi April 26, 2025 09:40

fzipi suggested changes Apr 26, 2025

View reviewed changes

Change owner in legal text

22fee12

airween mentioned this pull request Apr 27, 2025

feat: improved XMLArgs processing #3358

Open

theseion requested changes Apr 27, 2025

View reviewed changes

airween and others added 16 commits April 27, 2025 20:18

Update comment

e367876

Co-authored-by: Max Leske <[email protected]>

Update comment

3dc9fe9

Co-authored-by: Max Leske <[email protected]>

Update error message

90be54e

Co-authored-by: Max Leske <[email protected]>

Update comment

f0aa070

Co-authored-by: Max Leske <[email protected]>

Update error message

eedfed8

Co-authored-by: Max Leske <[email protected]>

Update comment

5b1c6fb

Co-authored-by: Max Leske <[email protected]>

Update comment

fedc709

Co-authored-by: Max Leske <[email protected]>

Update comment

0fcd257

Co-authored-by: Max Leske <[email protected]>

Update explanation

bbe7eda

Co-authored-by: Max Leske <[email protected]>

Update comment

159f612

Co-authored-by: Max Leske <[email protected]>

Update comment

2000f4c

Co-authored-by: Max Leske <[email protected]>

Add explanation

0bf6020

Co-authored-by: Max Leske <[email protected]>

Update comment

72de7e8

Co-authored-by: Max Leske <[email protected]>

Update comment

0c7ea21

Co-authored-by: Max Leske <[email protected]>

Update comment

6742930

Co-authored-by: Max Leske <[email protected]>

Update comment

8947346

Co-authored-by: Max Leske <[email protected]>

airween and others added 2 commits April 27, 2025 20:28

Update comment

2135c89

Co-authored-by: Max Leske <[email protected]>

Update error message

91a45e7

Co-authored-by: Max Leske <[email protected]>

airween added 4 commits April 27, 2025 21:09

Align debug messages to fix regression tests

0b62b7e

Change directive format to strict camel case

bf707de

Change node value's parsing to concatenate instead of copy it every time

e8dc60e

Change directives in tests; add multibyte test case

89442ed

airween requested review from theseion and fzipi April 28, 2025 20:55

theseion requested changes Apr 30, 2025

View reviewed changes

Update comment

d228ea6

Co-authored-by: Max Leske <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improved XMLArgs processing #3363

feat: improved XMLArgs processing #3363

airween commented Apr 20, 2025

fzipi left a comment •

edited

Loading

fzipi Apr 26, 2025

airween Apr 26, 2025

fzipi Apr 27, 2025

airween Apr 27, 2025

fzipi Apr 27, 2025

airween Apr 27, 2025

fzipi Apr 26, 2025

airween Apr 26, 2025

fzipi Apr 26, 2025

theseion Apr 27, 2025

fzipi Apr 26, 2025

airween Apr 26, 2025

airween commented Apr 26, 2025

airween commented Apr 27, 2025 •

edited

Loading

airween commented Apr 28, 2025

fzipi commented Apr 29, 2025

airween commented Apr 29, 2025

fzipi commented Apr 29, 2025

theseion Apr 30, 2025

airween May 1, 2025

theseion May 2, 2025

theseion Apr 30, 2025

airween May 1, 2025

theseion May 2, 2025

theseion Apr 30, 2025

airween May 1, 2025

theseion Apr 30, 2025

airween May 1, 2025

airween commented May 1, 2025

sonarqubecloud bot commented May 1, 2025

theseion commented May 2, 2025

		bool ParseXmlIntoArgs::init(std::string *error) {
		std::string what(m_parser_payload, 17, m_parser_payload.size() - 17);

		m_secXMLExternalEntity(PropertyNotSetConfigBoolean),
		m_secXMLParseXmlIntoArgs(PropertyNotSetConfigXMLParseXmlIntoArgs),

	XMLNodes::XMLNodes(Transaction *transaction)
	XmlNodes::XmlNodes(Transaction *transaction)

	XML::XML(Transaction *transaction)
	Xml::Xml(Transaction *transaction)

feat: improved XMLArgs processing #3363

Are you sure you want to change the base?

feat: improved XMLArgs processing #3363

Conversation

airween commented Apr 20, 2025

what

why

references

fzipi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

airween commented Apr 26, 2025

airween commented Apr 27, 2025 • edited Loading

airween commented Apr 28, 2025

fzipi commented Apr 29, 2025

airween commented Apr 29, 2025

fzipi commented Apr 29, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

airween commented May 1, 2025

sonarqubecloud bot commented May 1, 2025

Quality Gate passed

theseion commented May 2, 2025

fzipi left a comment •

edited

Loading

airween commented Apr 27, 2025 •

edited

Loading