fix MULTIPART_UNMATCHED_BOUNDARY

[zehric]

The MULTIPART_UNMATCHED_BOUNDARY currently is set to 2 any time there is more than one boundary found in the request, even if all the boundaries were matched. This change makes it so that it will only be set to 2 if there was an unmatched boundary already found in the body somewhere.

[zehric]

@airween We discussed this change over email. Please help me bring this into master.

[zimmerle]

Hi @zehric,

Thank you for the pull request. The condition to turn that variable to 2 is described on a comment above the code that you have changed. Here -
https://github.com/SpiderLabs/ModSecurity/blob/v3/master/src/request_body_processor/multipart.cc#L1480-L1487

Is that description still valid?

See #1747 for further discussion on this particular issue.

[zehric]

It is still valid. Erwin and I had a discussion on this and he agrees it should be added.

[airween]

@zehric,

I'm sorry I can't help to merge any request, I'm just a contributor.

In our mail discussion I've suggested you take regression tests file(s) as well, but I don't see any of that. Please feel free to use this patch, and add to your PR. Also need to change the comment in the code.

[airween]

It is still valid. Erwin and I had a discussion on this and he agrees it should be added.

No, it's not valid. But in our discussion you wrote that:

Some lines like SecRule MULTIPART_UNMATCHED_BOUNDARY "!@eq 0" might be wrong after this change.

That's why you have to add a new correct description.

[airween]

To everyone who wants to check the behavior, here is a standalone regression test.

You can see that the valid boundary will gives value 2 for MULTIPART_UNMATCHED_BOUNDARY variable. This PR fixes it, and gives 0.

[zimmerle]

I am intrigued. Since I am not seen any previous discussion about the subject, can any of you clarify the objective of this patch?

I agree on the original comment on the pull request. I also agree with the comment on the code. But, I think the code - after the patch - is doing something else.

So, @zehric can you give me an example (request) of the issue that you want to mitigate.

[martinhsv]

So, this fix is really only relevant if someone is using

SecRule MULTIPART_UNMATCHED_BOUNDARY "@gt 0" ...

instead of

SecRule MULTIPART_UNMATCHED_BOUNDARY "@eq 1

Right?

[zehric]

@zimmerle I don't think the comment needs to change, the new behavior is in line with what the comment says.

Take this well-formed request:

Content-Type: multipart/form-data; boundary=0000

--0000
Content-Disposition: form-data; name="name"

Brian Rectanus
--0000
Content-Disposition: form-data; name="image"; filename="image.jpg"

BINARYDATA
--0000--

The current code will result in m_flag_unmatched_boundary == 2 even though there are no unmatched boundaries whatsoever in the entire body, because there is more than one matched boundary. That seems wrong to me as it should be 0 if there are no anomalies found at all.

Also, @martinhsv, unrelatedly I had a question to a closed PR here, could you please check it out? #2214

[zimmerle]

Thank you @zehric -- Indeed the example clarify the issue. I want to make a more detailed review of the functionality, not only your patch. We keep changing the same portion of code back and forward, let me understand it better and them I jump to review your patch. I will post the details here so you can follow up. I will be ready for it on Tuesday.

[martinhsv]

IMHO, there's nothing wrong with this pull request in isolation. It does fix use cases where the flag should be 0 but currently has an incorrect value of 2. And it doesn't make any other use cases worse.

However, I do have some misgivings about this overall solution (#1747 and #1924) for modified MULTIPART_UNMATCHED_BOUNDARY checking.

My first concern is that we have removed so many use cases from resulting in flag=1 that I start to wonder how useful the check still is (partly keeping in mind that rule 200004 (in https://github.com/SpiderLabs/ModSecurity/blob/v3/master/modsecurity.conf-recommended) tests only for the more permissive "@eq 1, many users will simply use the default without assessing matters further).

Absent an identified impedance mismatch between this permissive ModSecurity behaviour and any web server, it's hard to assess how much of a concern this is.

A couple of options that could be considered if we want to tighten things slightly (i.e. increase the number of cases that trigger rule 200004 even with the permissive "@eq 1" setting:

A) Set the flag to 1 if the 'unmatched boundary' appears to be in a boundary position. For example:

--1BOUNDARY
Content-Disposition: form-data; name="my_id"

aabb
--2BOUNDARY
Content-Disposition: form-data; name="my_name"

rrst
--1BOUNDARY
Content-Disposition: form-data; name="yyy"

wwxx
--1BOUNDARY--

We could potentially test for a case like this by looking for a part header on the line immediately following the unmatched-boundary and then have this case result in flag=1

B) A second way we could narrow the permissiveness slightly is to consider the 'unmatched boundary' to be a part of legitimate data only if the tentatively-identified boundary exceeds the length requirement of <=70 characters. This should address the known PEM use case, but would admittedly be at risk of false positives in some other cases.

================================

A second concern is that the current solution isn't even a complete solution for the identified false-positive scenario that began these flag modifications.

It is valid to want to be more permissive in cases where the part's data value legitimately begins with '--'.
However, consider this content:

--1BOUNDARY
Content-Disposition: form-data; name="my_id"

aabb
--1BOUNDARY
Content-Disposition: form-data; name="my_name"

rrst
--1BOUNDARY
Content-Disposition: form-data; name="yyy"

--wwxx
--1BOUNDARY--

In this case, the code (both before and after this pull request) will set the flag at 1. Whereas if the '--' appeared at the beginning of the data in either of the two earlier parts, the flag would only be set to '2'. There's no particularly good reason to assume that if data beginning with '--' might be legitimate in a multipart part, that it is illegitimate in (only) the final part.

================================

All of that said, perhaps what I have raised here is not that serious and that this pull request can proceed; additional issues could potentially be dealt with separately and later.

But I thought I should mention these, not just for the sake of this pull request, but also for the v2.9 pull request that is still outstanding ( #2193 )

[airween]

hi @martinhsv,

But I thought I should mention these, not just for the sake of this pull request, but also for the v2.9 pull request that is still outstanding ( #2193 )

thanks - I remember this PR, and thought I'll align that for this behavior. But first I'd like to see this PR will merged - or dropped.

[zimmerle]

@zehric sorry for the delay on merging this. As you can see there is a discussion around the functionality that needs to be address prior coding. Marking this as 3.1.1.

[martinhsv]

I had intended to close this unmerged, but now that we're into the transition period, I'll leave the final disposition to the new custodians.

Much of what I'll summarize here is included in earlier discussions in #2193 and #2681 .

In brief:

1. the current functionality for MULTIPART_UNMATCHED_BOUNDARY is broken in v3 (see, for example, my findings at fix MULTIPART_UNMATCHED_BOUNDARY #2417 (comment) )
2. even the original implementation of MULTIPART_UNMATCHED_BOUNDARY in v3 (and the current implementation in v2) is prone to high false positives
3. I found no evidence that the mere presence of the two-character sequence '--' in a part body is suspicious, much less a danger to a parsing mismatch between ModSecurity and the protected web app
4. Assuming (3) is correct, any desired detection of '--' as a value would be better left to rule sets, not a builtin part of the engine

My recommendation would be to remove the functionality for MULTIPART_UNMATCHED_BOUNDARY from ModSecurity v3 (and v2 assuming it continues for a non-brief period as a maintained codeline).

One use case that may be worth separate consideration is if the entire boundary string for the request is found within the data portion of a part, then perhaps signal INVALID_PART (that scenario is forbidden by the relevant RCFs so could be legitimately considered suspicious). I am unaware of any current web technologies where this scenario poses a danger, but it could be considered as a safety measure, if the community is otherwise fine with deprecation/obsoletion of MULTIPART_UNMATCHED_BOUNDARY.

[sonarcloud]

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud