Skip to content

MULTIPART_UNMATCHED_BOUNDARY=1 functionality is broken #2681

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

MULTIPART_UNMATCHED_BOUNDARY=1 functionality is broken #2681

wants to merge 3 commits into from

Conversation

martinhsv
Copy link
Contributor

This pull request is not intended for near-term merging, but rather, to illustrate one of the problems with current v3 functionality for MULTIPART_UNMATCHED_BOUNDARY.

(This pull request is not intended to illustrate cases where either a) the value should be set to '2' but is not or b) the value is incorrectly set to '2' when it should be '0'.)

Rule 200004 as provided in modsecurity.conf-recommended tests:

SecRule MULTIPART_UNMATCHED_BOUNDARY "@eq 1" ...

In current v3/master, if there are 'n' apparent boundaries (basically lines beginning with '--'), then if any of the 2nd through n-2-th are non-matches, they will fail to result in MULTIPART_UNMATCHED_BOUNDARY=1.

E.g. if there are a total of 10 apparent boundaries, if any of the 2nd, 3rd, 4th, 5h, 6th, 7th or 8th of them should signal as 'unmatched', MULTIPART_UNMATCHED_BOUNDARY will not get set to 1.

This is illustrated in the 3 new tests in this file. They all fail in current v3/master but they all pass in v3/master as at June 7,2018.

@martinhsv martinhsv added the 3.x Related to ModSecurity version 3.x label Jan 29, 2022
"Content-Disposition: form-data; name=\"a\"",
"",
"1",
"----------------------------wrong-unmatched",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not an unmatched boundary.

},
"rules":[
"SecRuleEngine On",
"SecRule MULTIPART_UNMATCHED_BOUNDARY \"@eq 1\" \"id:1,phase:2,deny,status:403\""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You expect value 1 but the PR #1747 has modified the logic, that in this case the engine gives 2. See the detailed information in #1801.

"Content-Disposition: form-data; name=\"b\"",
"",
"2",
"----------------------------wrong-unmatched",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not an unmatched boundary.

},
"rules":[
"SecRuleEngine On",
"SecRule MULTIPART_UNMATCHED_BOUNDARY \"@eq 1\" \"id:1,phase:2,deny,status:403\""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as in case of test #2.

"Content-Disposition: form-data; name=\"c\"",
"",
"3",
"----------------------------wrong-unmatched",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not an unmatched boundary.

},
"rules":[
"SecRuleEngine On",
"SecRule MULTIPART_UNMATCHED_BOUNDARY \"@eq 1\" \"id:1,phase:2,deny,status:403\""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as in case of test #2.

@airween
Copy link
Member

airween commented Jan 31, 2022

Hi @martinhsv,

thanks for the examples.

This pull request is not intended for near-term merging, but rather, to illustrate one of the problems with current v3 functionality for MULTIPART_UNMATCHED_BOUNDARY.

I've made a quick research about term of "unmatched multipart boundary". Unfortunately I found this expression only at ModSecurity, so I'm a bit disappointed, because I don't know the exact meaning of this expression.

(This pull request is not intended to illustrate cases where either a) the value should be set to '2' but is not or b) the value is incorrectly set to '2' when it should be '0'.)

May be that would be good to see these cases too. I mean I'm curious when this value has set up to 2 but it wouldn't be, or reverse.

Rule 200004 as provided in modsecurity.conf-recommended tests:

SecRule MULTIPART_UNMATCHED_BOUNDARY "@eq 1" ...

In current v3/master, if there are 'n' apparent boundaries (basically lines beginning with '--'), then if any of the 2nd through n-2-th are non-matches, they will fail to result in MULTIPART_UNMATCHED_BOUNDARY=1.

Yes, and I think that's wrong. Consider the line which begins with --, but that's not a boundary, just a signature separator from an e-mail body (eg. you want to protect your webmail with ModSecurity).

Like:

Content-Type: multipart/form-data; boundary=--0000
----0000
Content-Disposition: form-data; name="_msg_body"

Hi Martin,

this is the test message.

Regards,

--
airween
----0000--

I can't consider the line -- as boundary, but the engine does. This is wrong.

E.g. if there are a total of 10 apparent boundaries, if any of the 2nd, 3rd, 4th, 5h, 6th, 7th or 8th of them should signal as 'unmatched', MULTIPART_UNMATCHED_BOUNDARY will not get set to 1.

I think the keyword here is apparent boundaries - no, they are not boundaries. I think we must consider them as regular lines. Every line which begins with --, but not matched with the boundary string is a regular content.

This is illustrated in the 3 new tests in this file. They all fail in current v3/master but they all pass in v3/master as at June 7,2018.

Yes, that's what I fixed.

Please take a look to my example with "e-mail signature separator".

Btw: thanks again your works, I think this is a good chance to re-think the terms - and the expected behavior, of course. As I wrote above, I didn't find any definition of "unmatched boundary" outside of ModSecurity. Also as you see my opinion, these lines are not unmatched boundaries.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Related to ModSecurity version 3.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants