Fixed false positive MULTIPART_UNMATCHED_BOUNDARY errors

[airween]

Couple of months ago I've posted a report to Owasp-modsecurity-core-rule-set list, where I signed that the file upload does not works:
http://lists.owasp.org/pipermail/owasp-modsecurity-core-rule-set/2017-August/002458.html

The problem came with a PEM encoded file, that's why I called my branch uploadpem.

Now I could focused to the problem, and looks like the multipart handler contains a bug: if the body of multipart data contains a line at the begin with "--", libmodsecurity interprets it as boundary - it doesn't matter what's that: a PEM format file, eg. X509 CERT (-----BEGIN CERTIFICATE-----), traditional signature (--\n), or any other.

I've made a JSON test for the regression-test, which contains some "real" example:

https://gist.githubusercontent.com/airween/522eaebf38510c025961f4810614949b/raw/2021642dcdcede9a2a572633868a65b8ecd4b522/request-body-parser-multipart-with-hypen.json

Before you merge (if you will :)), then you can check it: the current libmodsecurity (v3/master branch) has failed with most of that.

This patch is very simple: I just modified the source, that if there is a matched boundary least which declared in header, and there is a final (matched) boundary, then it doesn't matter, what is the content. I've checked the RFC (1341), looks like this is the right way.

I've found some mailing list message related to this topic, and some Gihub issue, but there wasn't any solution. Hope that you'll merge accept and merge this, and now all problems will go away...

[lifeforms]

Very cool. This validation can be a little trigger happy, I'm sure many users will appreciate this!

[zimmerle]

Thank you for providing the test case altogether with the change.

[airween]

You're welcome - I'm enjoying to participate :)

[zimmerle]

Hi,

That is indeed a very interesting discussion. This specific variable is documented here:

• https://github.com/SpiderLabs/ModSecurity/wiki/Reference-Manual-%28v2.x%29#multipart_unmatched_boundary

The idea of the variable was to flag the existence of something that looks like a boundary. That was made to avoid the scenario that a forgiven parser may interpret the data differently from the specifications leading the data to bypass without ModSecurity inspection. Therefore leading to an evasion. I am not sure if the proposed fix lead to the same level of detection that we have today. Although I complete agree that the variable is currently being set without good discretion.

I had something else in mind that may avoid the unnecessary raise of the flag, yet, give same level of protection. The idea is to check if a given data - which looks like a boundary - was found twice in a block. If so, it sets to "1" the MULTIPART_UNMATCHED_BOUNDARY otherwise it won't bother. This will solve the PEM encoded upload an all the cases proposed in the original patch test case.

For the reference:
https://github.com/SpiderLabs/ModSecurity/tree/v3/dev/suggestion_1747

What do you thing about it?

[airween]

Hi Felipe,

thanks for the feedback - I've planned to ask you, what's your opinion about this fix. After the long delay, I thought that will not acceptable in 100% - but it's no problem :).

Note, that the specification of this variable is clear, and I think that's very useful - but only if it works as well. As I signed in the lead of this fix, I've run this problem at my webmail instance - I could't upload any PEM format text, but also couldn't any plain text, which contains a simple "--" started line(s) (eg. copy a mail text with the signature).

Now I don't see, how breaks this fix the detection level - I think the logic of parser didn't changed. If there is any unmatched boundary, the variable will be set. And - as you can see at the regression test case - the reference boundary value comes from the multipart header. This means, that there must be three equal boundary in a multipart part: first in the header, second in the open boundary tag, and finally the close boundary tag. If any component missing, the variable will set.

Anyway, I was also prepared with Plan B :). I can modify this fix like this:

• if there isn't any multipart, or all boundary is right, variable set to 0
• if there are any unmatched, then value set to 1
• BUT if the unmatched is a PEM header/footer, AND all there components exists, variable set to 2

With this logic, only need to modify the config from "!@eq 0" to "@eq 1", or "@eq 2" and user can check more state.

I'll see your suggestion as soon, but I need some time to make the checks.

Thanks again,

a.

[zimmerle]

@airween Just opening the subject for discussion :) Good ideas came out from discussions like this :)

[airween]

Sorry to ask, but what do you think about it: "just opening the subject for discussion"?

I'm not a practical Github user :)

[airween]

Hi Felipe,

sorry to tell you, but I'm afraid the v3/dev/suggestion_1747 solution isn't a good choice. I've added the new tests to that branch (see PR #1792), please take a look that.

Especially these two test cases:
https://github.com/airween/ModSecurity/blob/92ab96d9618ab034e650535cb5f9353ac377a1de/test/test-cases/regression/request-body-parser-multipart.json#L2898
https://github.com/airween/ModSecurity/blob/92ab96d9618ab034e650535cb5f9353ac377a1de/test/test-cases/regression/request-body-parser-multipart.json#L2776

In first case, you can see a fully valid HTTP multipart request. Your solution is still blocking this request, because - as you wrote - if a boundary occurs twice or more at least, then it interprets as attack. I think that's not a solution to prevent the false positive MULTIPART_UNMATCHED_BOUNDARY errors :).

The second test is a bigger problem - there is a fully invalid request, but the ModSecurity now allows it. As you can see, the lead boundary of multipart differs from the reference. That would be interprets as "Ignoring data before first boundary", but there isn't first boundary... I think that's a very critical issue.

So, I've modified my patch - now if there is one boundary at least (which equals with the reference), AND there is the finaly boundary (which also equals with reference boundary), the flag will changed to "2", instead of "0".

I named this two modes as "strict mode" (which is the "classic" mode, and anybody can use it without any modification), and "permissive mode", which helps to prevent the fail UNMATCHED_BOUNDARY issues.

Please review the test cases and the new modification - where the multipart request is correct, but contains any boundary-like line(s), the "!@eq 0" rule will catch it, and you'll get HTTP/403. If the rule contains "@eq 1", in these cases the result will HTTP/200.

If you have any idea/question, just let me know :).

a.

[zimmerle]

Hi @airween,

I like your second plan best. It will allow an easy adoption for those who have the !eq 0.

https://github.com/SpiderLabs/ModSecurity/blob/95048d5fcfe43147ab0269bff69e2353817cb7c7/modsecurity.conf-recommended#L79-L80

Even considering a new semantic to the variable, it should not affect the users that are using it now.

Thank you for patch!