From ca99ccd23fd92c002a3901e890b4b6ea394f292b Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Tue, 24 Jun 2025 21:12:26 +0200 Subject: [PATCH 1/6] Fix unexpected behavior if the XML tag is empty --- apache2/msc_xml.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/apache2/msc_xml.c b/apache2/msc_xml.c index 8b0fa766..74945a08 100644 --- a/apache2/msc_xml.c +++ b/apache2/msc_xml.c @@ -85,14 +85,14 @@ static void msc_xml_on_end_elementns( arg->name = xml_parser_state->currpath; arg->name_len = strlen(arg->name); - arg->value = xml_parser_state->currval; - arg->value_len = strlen(xml_parser_state->currval); + arg->value = (xml_parser_state->currval == NULL) ? apr_pstrndup(msr->mp, "", 1) : xml_parser_state->currval; + arg->value_len = (xml_parser_state->currval == NULL) ? 0 : strlen(xml_parser_state->currval); arg->value_origin_len = arg->value_len; arg->origin = "XML"; if (msr->txcfg->debuglog_level >= 9) { msr_log(msr, 9, "Adding XML argument '%s' with value '%s'", - xml_parser_state->currpath, xml_parser_state->currval); + xml_parser_state->currpath, arg->value); } apr_table_addn(msr->arguments, From 89d3ad38c5a964f760af240e3ace3467bb729a64 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Tue, 24 Jun 2025 21:34:12 +0200 Subject: [PATCH 2/6] Introduced a new variable to hold currval length --- apache2/msc_xml.c | 11 +++++++++-- apache2/msc_xml.h | 1 + 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/apache2/msc_xml.c b/apache2/msc_xml.c index 74945a08..dfb622cd 100644 --- a/apache2/msc_xml.c +++ b/apache2/msc_xml.c @@ -44,6 +44,7 @@ static void msc_xml_on_start_elementns( // this is necessary because if there is any text between the tags (new line, etc) // it will be added to the current value xml_parser_state->currval = NULL; + xml_parser_state->currpathbufflen = 0; // if there is an item before the current one we set that has a child if (xml_parser_state->depth > 1) { @@ -73,7 +74,11 @@ static void msc_xml_on_end_elementns( if (msr->txcfg->debuglog_level >= 4) { msr_log(msr, 4, "Skipping request argument, over limit (XML): name \"%s\", value \"%s\"", log_escape_ex(msr->mp, xml_parser_state->currpath, strlen(xml_parser_state->currpath)), - log_escape_ex(msr->mp, xml_parser_state->currval, strlen(xml_parser_state->currval))); + log_escape_ex(msr->mp, + (xml_parser_state->currval == NULL ? apr_pstrndup(msr->mp, "", 1) : xml_parser_state->currval), + (xml_parser_state->currvalbufflen == 0 ? 1 : xml_parser_state->currvalbufflen) + ) + ); } msr->msc_reqbody_error = 1; msr->xml->xml_error = apr_psprintf(msr->mp, "More than %ld ARGS (GET + XML)", msr->txcfg->arguments_limit); @@ -86,7 +91,7 @@ static void msc_xml_on_end_elementns( arg->name = xml_parser_state->currpath; arg->name_len = strlen(arg->name); arg->value = (xml_parser_state->currval == NULL) ? apr_pstrndup(msr->mp, "", 1) : xml_parser_state->currval; - arg->value_len = (xml_parser_state->currval == NULL) ? 0 : strlen(xml_parser_state->currval); + arg->value_len = (xml_parser_state->currvalbufflen == 0) ? 1 : xml_parser_state->currvalbufflen; arg->value_origin_len = arg->value_len; arg->origin = "XML"; @@ -123,6 +128,7 @@ static void msc_xml_on_characters(void *ctx, const xmlChar *ch, int len) { ((xml_parser_state->currval != NULL) ? xml_parser_state->currval : ""), apr_pstrndup(msr->mp, (const char *)ch, len), NULL); + xml_parser_state->currvalbufflen += len; // check if the memory allocation was successful if (xml_parser_state->currval == NULL) { msr->xml->xml_error = apr_psprintf(msr->mp, "Failed to allocate memory for XML value."); @@ -175,6 +181,7 @@ int xml_init(modsec_rec *msr, char **error_msg) { msr->xml->xml_parser_state->pathlen = 4; // "xml\0" msr->xml->xml_parser_state->currpath = apr_pstrdup(msr->mp, "xml"); msr->xml->xml_parser_state->currval = NULL; + msr->xml->xml_parser_state->currvalbufflen = 0; msr->xml->xml_parser_state->currpathbufflen = 4; // initialize the stack with item of 10 // this will store the information about nodes diff --git a/apache2/msc_xml.h b/apache2/msc_xml.h index 3d23c60d..dd232fba 100644 --- a/apache2/msc_xml.h +++ b/apache2/msc_xml.h @@ -31,6 +31,7 @@ struct msc_xml_parser_state { char * currpath; char * currval; size_t currpathbufflen; + size_t currvalbufflen; apr_pool_t * mp; }; From f9e81f2c78188b486dee6d76fd274d06e15d0a71 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Tue, 24 Jun 2025 22:02:00 +0200 Subject: [PATCH 3/6] Leave strlen() if not necesseraly; use own length storage --- apache2/msc_xml.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/apache2/msc_xml.c b/apache2/msc_xml.c index dfb622cd..b964daad 100644 --- a/apache2/msc_xml.c +++ b/apache2/msc_xml.c @@ -36,6 +36,7 @@ static void msc_xml_on_start_elementns( xml_parser_state->pathlen += (taglen + 1); char *newpath = apr_pstrcat(msr->mp, xml_parser_state->currpath, ".", (char *)localname, NULL); xml_parser_state->currpath = newpath; + xml_parser_state->currpathbufflen += taglen; int *new_stack_item = (int *)apr_array_push(xml_parser_state->has_child_stack); *new_stack_item = 0; @@ -44,7 +45,7 @@ static void msc_xml_on_start_elementns( // this is necessary because if there is any text between the tags (new line, etc) // it will be added to the current value xml_parser_state->currval = NULL; - xml_parser_state->currpathbufflen = 0; + xml_parser_state->currvalbufflen = 0; // if there is an item before the current one we set that has a child if (xml_parser_state->depth > 1) { @@ -73,7 +74,7 @@ static void msc_xml_on_end_elementns( if (apr_table_elts(msr->arguments)->nelts >= msr->txcfg->arguments_limit) { if (msr->txcfg->debuglog_level >= 4) { msr_log(msr, 4, "Skipping request argument, over limit (XML): name \"%s\", value \"%s\"", - log_escape_ex(msr->mp, xml_parser_state->currpath, strlen(xml_parser_state->currpath)), + log_escape_ex(msr->mp, xml_parser_state->currpath, xml_parser_state->currpathbufflen), log_escape_ex(msr->mp, (xml_parser_state->currval == NULL ? apr_pstrndup(msr->mp, "", 1) : xml_parser_state->currval), (xml_parser_state->currvalbufflen == 0 ? 1 : xml_parser_state->currvalbufflen) @@ -89,7 +90,7 @@ static void msc_xml_on_end_elementns( msc_arg * arg = (msc_arg *) apr_pcalloc(msr->mp, sizeof(msc_arg)); arg->name = xml_parser_state->currpath; - arg->name_len = strlen(arg->name); + arg->name_len = xml_parser_state->currpathbufflen; arg->value = (xml_parser_state->currval == NULL) ? apr_pstrndup(msr->mp, "", 1) : xml_parser_state->currval; arg->value_len = (xml_parser_state->currvalbufflen == 0) ? 1 : xml_parser_state->currvalbufflen; arg->value_origin_len = arg->value_len; @@ -111,9 +112,11 @@ static void msc_xml_on_end_elementns( // -1 is needed because we don't need the last '.' char * newpath = apr_pstrndup(msr->mp, xml_parser_state->currpath, xml_parser_state->pathlen - 1); xml_parser_state->currpath = newpath; + xml_parser_state->currpathbufflen = xml_parser_state->pathlen - 2; // -2 because of the '\0' and the last '.' xml_parser_state->depth--; xml_parser_state->currval = NULL; + xml_parser_state->currvalbufflen = 0; } static void msc_xml_on_characters(void *ctx, const xmlChar *ch, int len) { @@ -180,9 +183,9 @@ int xml_init(modsec_rec *msr, char **error_msg) { msr->xml->xml_parser_state->depth = 0; msr->xml->xml_parser_state->pathlen = 4; // "xml\0" msr->xml->xml_parser_state->currpath = apr_pstrdup(msr->mp, "xml"); + msr->xml->xml_parser_state->currpathbufflen = 3; // "xml" msr->xml->xml_parser_state->currval = NULL; msr->xml->xml_parser_state->currvalbufflen = 0; - msr->xml->xml_parser_state->currpathbufflen = 4; // initialize the stack with item of 10 // this will store the information about nodes // 10 is just an initial value, it can be automatically incremented From 8cb7fc82fecd182369bcffd93ab81f48d9c0a18c Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Tue, 24 Jun 2025 22:10:20 +0200 Subject: [PATCH 4/6] Set correct length of currpath --- apache2/msc_xml.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apache2/msc_xml.c b/apache2/msc_xml.c index b964daad..8f4056f5 100644 --- a/apache2/msc_xml.c +++ b/apache2/msc_xml.c @@ -112,7 +112,7 @@ static void msc_xml_on_end_elementns( // -1 is needed because we don't need the last '.' char * newpath = apr_pstrndup(msr->mp, xml_parser_state->currpath, xml_parser_state->pathlen - 1); xml_parser_state->currpath = newpath; - xml_parser_state->currpathbufflen = xml_parser_state->pathlen - 2; // -2 because of the '\0' and the last '.' + xml_parser_state->currpathbufflen = xml_parser_state->pathlen - 1; xml_parser_state->depth--; xml_parser_state->currval = NULL; From e56d62960eee41e4ed92c8acafe5cad80047601a Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Thu, 26 Jun 2025 17:42:49 +0200 Subject: [PATCH 5/6] Set correct pathlen --- apache2/msc_xml.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apache2/msc_xml.c b/apache2/msc_xml.c index 8f4056f5..8df3280b 100644 --- a/apache2/msc_xml.c +++ b/apache2/msc_xml.c @@ -36,7 +36,7 @@ static void msc_xml_on_start_elementns( xml_parser_state->pathlen += (taglen + 1); char *newpath = apr_pstrcat(msr->mp, xml_parser_state->currpath, ".", (char *)localname, NULL); xml_parser_state->currpath = newpath; - xml_parser_state->currpathbufflen += taglen; + xml_parser_state->currpathbufflen += taglen + 1; int *new_stack_item = (int *)apr_array_push(xml_parser_state->has_child_stack); *new_stack_item = 0; @@ -98,7 +98,7 @@ static void msc_xml_on_end_elementns( if (msr->txcfg->debuglog_level >= 9) { msr_log(msr, 9, "Adding XML argument '%s' with value '%s'", - xml_parser_state->currpath, arg->value); + arg->name, arg->value); } apr_table_addn(msr->arguments, From 8879413abf507b1921f6feb292ee91e0f0064b01 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Thu, 26 Jun 2025 21:50:54 +0200 Subject: [PATCH 6/6] Add comment to explain the behavior --- apache2/msc_xml.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apache2/msc_xml.c b/apache2/msc_xml.c index 8df3280b..1deac6fc 100644 --- a/apache2/msc_xml.c +++ b/apache2/msc_xml.c @@ -36,7 +36,7 @@ static void msc_xml_on_start_elementns( xml_parser_state->pathlen += (taglen + 1); char *newpath = apr_pstrcat(msr->mp, xml_parser_state->currpath, ".", (char *)localname, NULL); xml_parser_state->currpath = newpath; - xml_parser_state->currpathbufflen += taglen + 1; + xml_parser_state->currpathbufflen += taglen + 1; // +1 for the '.' character here too int *new_stack_item = (int *)apr_array_push(xml_parser_state->has_child_stack); *new_stack_item = 0;