Fix MULTIPART_UNMATCHED_BOUNDARY FP errors

[airween]

There was a bug and a fix (#1747) in libmodsecurity3.

We ran into same problem with v2, so I made this patch, which fixes this bug as same way like in v3.
Hope that you'll allow, and will fix these issues:
#652
#1804
#2021

[fzipi]

Still having this problem.. and there is a patch available....

Can someone merge this one?

[nickcyberpunk]

Hi,
is this patch going to be accepted/merged ?

I'm also facing similar issues on modsec.

Thanks

[martinhsv]

I'm going to close this unmerged.

The code here mirrors the v3 implementation where some significant problem have been identified, including:

• the functionality removes so many use cases from triggering the flag to equal 1 that it's questionable how useful the check for MULTIPART_UNMATCHED_BOUNDARY still is
• while the intention of the implementation was to have a way to avoid signalling that condition when there is legitimate PEM content in one of the parts (i.e. it begins with '--' like a multipart boundary does); this implementation does not do that successfully if the PEM content is in the final of the multipart parts
• even if none of the multipart parts has a value beginning with '--', MULTIPART_UNMATCHED_BOUNDARY will incorrectly get set to 2

What really needs to happen with this functionality, is that it should be reviewed again in v3, with either a substantially revised solution implemented, or the '=2' code should be removed entirely. If the former happens, then that solution can be ported to v2.

In the immediate period for v2, likely the best way to handle FPs for PEM data is likely to stop using rule 200004 from modsecurity.conf-recommended for those cases where PEM (or similar) data is expected.

[airween]

The code here mirrors the v3 implementation

Yes, because that implementation had allowed after a discussion.

where some significant problem have been identified, including:

* the functionality removes so many use cases from triggering the flag to equal 1 that it's questionable how useful the check for MULTIPART_UNMATCHED_BOUNDARY still is

so many? Can you show me a few ones?

* while the intention of the implementation was to have a way to avoid signalling that condition when there is legitimate PEM content in one of the parts (i.e. it begins with '--' like a multipart boundary does); this implementation does not do that successfully if the PEM content is in the final of the multipart parts

note, that this implementation wasn't created only to avoid the FP of the PEM content. We have ran into several other issues.

* even if **none** of the multipart parts has a value beginning with '--', MULTIPART_UNMATCHED_BOUNDARY will incorrectly get set to 2

Yes, this bug has discovered later - but it is still unmerged.

What really needs to happen with this functionality, is that it should be reviewed again in v3, with either a substantially revised solution implemented, or the '=2' code should be removed entirely. If the former happens, then that solution can be ported to v2.

right. Is there any plan/priority to fix it?

In the immediate period for v2, likely the best way to handle FPs for PEM data is likely to stop using rule 200004 from modsecurity.conf-recommended for those cases where PEM (or similar) data is expected.

as I wrote above, the PEM file was just an example. In many cases we ran into this problem with a regular PDF/JPG...

I'm not sure it's a good idea to remove that rule.

Btw: I think this fix is even better than the current state, and merge would be better than remove the rule 200004.

[martinhsv]

Hello @airween ,

Much of this discussion might have been better located in the new issue for 'either a substantially revised solution implemented, or the '=2' code should be removed entirely.', but since I haven't created it yet, I'll respond to your main points here.

[Re: "...the functionality removes so many use cases from triggering the flag to equal 1..."] "Can you show me ... "

Basically, most cases of actual unmatched boundaries no longer result in the flag equal to 1. For example:

-----------------------------423513681827540048931513055996
Content-Disposition: form-data; name="a"

1
-----------------------------wrong-not-matched
Content-Disposition: form-data; name="b"

2
-----------------------------423513681827540048931513055996
Content-Disposition: form-data; name="c"

3
-----------------------------423513681827540048931513055996--

Re: "... but is still unmerged"

Yes, that's right. And it remains unmerged precisely because it does not address the fundamental design issues with the overall '=2' functionality that I first noted in that item in Oct. 2020.

"Btw: I think this fix is even better than the current state, and merge would be better than remove the rule 200004."

Even given all of the problems that I have outlined? We could engage further on why you think that's the case but that may be better done in the new issue that I plan to create (as mentioned at the top of this response).

"Is there any plan/priority ... "

Not yet. Not everything can be a top priority item. Activity on the project can and should be guided by what are deemed by the community to be high priority items. Indeed, such input is part of the reason that #2606 was implemented in an earlier timeframe than otherwise might have been the case. If it is the opinion of the CRS group that coming to some more permanent resolution on this matter is their most important issue, I will, of course, take that into account.

[martinhsv]

Btw, in case it is helpful to you, I have created a pull request with additional test cases that demonstrate the failure to set the variable to '1': #2681

[airween]

Hi @martinhsv,

thanks for the answers.

Much of this discussion might have been better located in the new issue for 'either a substantially revised solution implemented, or the '=2' code should be removed entirely.', but since I haven't created it yet, I'll respond to your main points here.

IMHO it's not a good idea to stay back to a wrong state. (Definition of wrong state: see later)

[Re: "...the functionality removes so many use cases from triggering the flag to equal 1..."] "Can you show me ... "

Basically, most cases of actual unmatched boundaries no longer result in the flag equal to 1. For example:

may be we can consider it as philosophical question, but the given example does not contain any unmatched boundaries.

-----------------------------423513681827540048931513055996
Content-Disposition: form-data; name="a"

1
-----------------------------wrong-not-matched
Content-Disposition: form-data; name="b"

2
-----------------------------423513681827540048931513055996
Content-Disposition: form-data; name="c"

3
-----------------------------423513681827540048931513055996--

Consider the request:

-----------------------------423513681827540048931513055996
Content-Disposition: form-data; name="a"

== My attachment starts here ==
1
-----------------------------wrong-not-matched
Content-Disposition: form-data; name="b"

2
== My attachment ends here ==
-----------------------------423513681827540048931513055996
Content-Disposition: form-data; name="c"

3
-----------------------------423513681827540048931513055996--

Do you still consider it as unmatched boundary? (This is why I think the previous state was wrong.)

Re: "... but is still unmerged"

Yes, that's right. And it remains unmerged precisely because it does not address the fundamental design issues with the overall '=2' functionality that I first noted in that item in Oct. 2020.

Value 2 was a forced solution. I think the definition of unmatched boundaries is still not clear (or wrong). The example above is not that (from my point of view). The correct value in that case should be 0, not 2 (and not 1, of course).

"Btw: I think this fix is even better than the current state, and merge would be better than remove the rule 200004."

Even given all of the problems that I have outlined? We could engage further on why you think that's the case but that may be better done in the new issue that I plan to create (as mentioned at the top of this response).

I'm afraid our assumptions are different about "problems" :).

I can't consider the given example as unmatched boundary. So yes, I think that's a better option.

"Is there any plan/priority ... "

Not yet. Not everything can be a top priority item.

Okay, it's no problem. But can we postpone the removing of this feature until the new PR is available?

Activity on the project can and should be guided by what are deemed by the community to be high priority items. Indeed, such input is part of the reason that #2606 was implemented in an earlier timeframe than otherwise might have been the case. If it is the opinion of the CRS group that coming to some more permanent resolution on this matter is their most important issue, I will, of course, take that into account.

Thanks for the offer. This means, if somebody send a new PR, you will handle it with priority?

Thanks again.

[martinhsv]

Hi @airween ,

In my example, the request body does not violate any RFCs or anything like that. From a standards point of view, everything is legitimate. The line with 'wrong-not-matched' is simply the second line of data for the part with the name 'a'. And the line after that (that begins with 'Content-Disposition') is the third line of the 'a' part, and so on.

However, I say with fairly strong conviction that that pattern is a use cases that was intended to signal MULTIPART_UNMATCHED_BOUNDARY=1 when that functionality was introduced to ModSecurity.

But suppose you're not convinced that I'm correct. Then what cases do you think, should result in MULTIPART_UNMATCHED_BOUNDARY=1 ? I would suggest that knowing an answer to that question would be a requirement before implementing changes.

This relates to your final question. A PR represents an implementation of some functionality. In assessing it one must ask: does it implement the requirements (without breaking anything)? But if we don't know what the requirements are, then it's pretty hard to conclude that a particular code change meets the requirements.

Fundamentally we have this question: Is there any point to anything akin to what MULTIPART_UNMATCHED_BOUNDARY does? Are there any actual web server technologies where boundary-like strings could result in parser inconsistencies, and hence an opportunity for evasion?

It is possible that there were one or more such cases back in 2007 when the MULTIPART_UNMATCHED_BOUNDARY was originally introduced (although even at that time the functionality was noted as being 'Not very reliable'). But perhaps there are no such cases now and perhaps haven't been for more than a decade. If that is indeed the case, we should probably move in the direction of deprecating MULTIPART_UNMATCHED_BOUNDARY. There's no point having functionality that both doesn't do anything useful and requires management to handle FPs.

[airween]

hi @martinhsv,

In my example, the request body does not violate any RFCs or anything like that.

I didn't even say the opposite :)

From a standards point of view, everything is legitimate. The line with 'wrong-not-matched' is simply the second line of data for the part with the name 'a'. And the line after that (that begins with 'Content-Disposition') is the third line of the 'a' part, and so on.

yes, that's correct.

But suppose you're not convinced that I'm correct. Then what cases do you think, should result in MULTIPART_UNMATCHED_BOUNDARY=1 ? I would suggest that knowing an answer to that question would be a requirement before implementing changes.

That's a good question, and I have been thinking about this too since your last comment. :)

Fundamentally we have this question: Is there any point to anything akin to what MULTIPART_UNMATCHED_BOUNDARY does?

perhaps no, there isn't.

Are there any actual web server technologies where boundary-like strings could result in parser inconsistencies, and hence an opportunity for evasion?

I'm not sure, but it may need to be researched. I'm afraid it's very dependent on implementation. I mean, there are as many interpretations as there are implementations. ModSecurity has its own - and as we can see, that's not a real good one.

It is possible that there were one or more such cases back in 2007 when the MULTIPART_UNMATCHED_BOUNDARY was originally introduced (although even at that time the functionality was noted as being 'Not very reliable'). But perhaps there are no such cases now and perhaps haven't been for more than a decade. If that is indeed the case, we should probably move in the direction of deprecating MULTIPART_UNMATCHED_BOUNDARY. There's no point having functionality that both doesn't do anything useful and requires management to handle FPs.

Absolutely agree this.

May be the solution will that you have wrote above: make the MULTIPART_UNMATCHED_BOUNDARY variable deprecated. I'm going to make some comparison and research about how webservers handle the MP requests. Let me check that.