Make multipart boundary check a bit less strict by default

[victorhora]

This pull request is sort of a workaround for multipart/form-data requests that have multiple boundaries such as common Content-Disposition POST requests.

The issue is that the fix proposed at #1747 and merged at 4d0ca94 could lead to cases where Content-Disposition POST requests such as the one below are denied by default after this commit:

POST /test.html HTTP/1.1
Host: example.org
Content-Type: multipart/form-data;boundary=boundary
Content-Length: 174

--boundary
Content-Disposition: form-data; name="field1"

value1
--boundary
Content-Disposition: form-data; name="field2"; filename="example.txt"

value2
--boundary--

Ref: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition

I suggesting making rule 200004 less strict by replacing a "deny" with a "pass", but still alerting the WAF operator. The operator can choose the disable or change the rule to not log if need be.

In addition, a new rule (200006) would be created in order to actually block cases where an unmatched boundary is actually found in the request.

[victorhora]

@airween, your comments and thoughts here would also be very welcome :)

[airween]

Oh', I am honoured to be invited, thank you :).

After first review, I didn't see the reason, why do you need to create a new rule with less security, but I can imagine that it helps for the administrators in most cases. I think this proposal is closer to the natural use of ModSecurity.

I just reviewed the Owasp-CRS rule set, and didn't see any affected rule. Anyway, it would be good to notify the CRS team about this changes. I guess the ModSec users will know it from CHANGES :).

I'm afraid I can not add more than above, sorry. :)

Thanks again.

[victorhora]

The following cURL command can be used to to illustrate the difference further:

curl -i -X POST -H "Content-Type: multipart/form-data" -F "file1=@file1.txt" -F "file2=file2.txt" http://localhost:8080/?a=upload

Debug logs from v2/v3 are presented below. Both run using vanilla modsecurity.conf-recommended

TLDR; Behaviour is different from v2.

v2:

[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Second phase starting (dcfg 55ba3cb2e4b0).
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Input filter: Reading request body.
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][9] Multipart: Boundary: ------------------------0fe8d74093002bdc
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][9] Input filter: Bucket type HEAP contains 299 bytes.
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][9] Multipart: Added part header "Content-Disposition" "form-data; name=\"file1\"; filename=\"file1.txt\""
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][9] Multipart: Added part header "Content-Type" "text/plain"
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][9] Multipart: Content-Disposition name: file1
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][9] Multipart: Content-Disposition filename: file1.txt
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][9] Multipart: Added file part 55ba3cb7ab08 to the list: name "file1" file name "file1.txt" (offset 140, length 8)
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][9] Multipart: Added part header "Content-Disposition" "form-data; name=\"file2\""
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][9] Multipart: Content-Disposition name: file2
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][9] Multipart: Added data to variable: file2.txt
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][9] Multipart: Added part 55ba3cb7b1e8 to the list: name "file2" (offset 242, length 9)
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][9] Input filter: Bucket type EOS contains 0 bytes.
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][5] Adding request argument (BODY): name "file2", value "file2.txt"
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Request body no files length: 153
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Input filter: Completed receiving request body (length 299).
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Starting phase REQUEST_BODY.
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][9] This phase consists of 4 rule(s).
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Recipe: Invoking rule 55ba3cb02908; [file "/usr/local/nginx/conf/modsecurity.conf"] [line "54"] [id "200002"].
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][5] Rule 55ba3cb02908: SecRule "REQBODY_ERROR" "!@eq 0" "phase:2,auditlog,id:200002,t:none,log,deny,status:400,msg:'Failed to parse request body.',logdata:%{reqbody_error_msg},severity:2"
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Transformation completed in 1 usec.
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Executing operator "!eq" with param "0" against REQBODY_ERROR.
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][9] Target value: "0"
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Operator completed in 2 usec.
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Rule returned 0.
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][9] No match, not chained -> mode NEXT_RULE.
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Recipe: Invoking rule 55ba3cb0c640; [file "/usr/local/nginx/conf/modsecurity.conf"] [line "75"] [id "200003"].
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][5] Rule 55ba3cb0c640: SecRule "MULTIPART_STRICT_ERROR" "!@eq 0" "phase:2,auditlog,id:200003,t:none,log,deny,status:400,msg:'Multipart request body failed strict validation: PE %{REQBODY_PROCESSOR_ERROR}, BQ %{MULTIPART_BOUNDARY_QUOTED}, BW %{MULTIPART_BOUNDARY_WHITESPACE}, DB %{MULTIPART_DATA_BEFORE}, DA %{MULTIPART_DATA_AFTER}, HF %{MULTIPART_HEADER_FOLDING}, LF %{MULTIPART_LF_LINE}, SM %{MULTIPART_MISSING_SEMICOLON}, IQ %{MULTIPART_INVALID_QUOTING}, IP %{MULTIPART_INVALID_PART}, IH %{MULTIPART_INVALID_HEADER_FOLDING}, FL %{MULTIPART_FILE_LIMIT_EXCEEDED}'"
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Transformation completed in 1 usec.
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Executing operator "!eq" with param "0" against MULTIPART_STRICT_ERROR.
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][9] Target value: "0"
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Operator completed in 1 usec.
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Rule returned 0.
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][9] No match, not chained -> mode NEXT_RULE.
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Recipe: Invoking rule 55ba3caff1b8; [file "/usr/local/nginx/conf/modsecurity.conf"] [line "118"] [id "200004"].
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][5] Rule 55ba3caff1b8: SecRule "MULTIPART_UNMATCHED_BOUNDARY" "!@eq 0" "phase:2,auditlog,id:200004,t:none,log,deny,msg:'Multipart parser detected a possible unmatched boundary.'"
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Transformation completed in 1 usec.
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Executing operator "!eq" with param "0" against MULTIPART_UNMATCHED_BOUNDARY.
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][9] Target value: "0"
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Operator completed in 1 usec.
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Rule returned 0.
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][9] No match, not chained -> mode NEXT_RULE.
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Recipe: Invoking rule 55ba3cb2ccc0; [file "/usr/local/nginx/conf/modsecurity.conf"] [line "135"] [id "200005"].
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][5] Rule 55ba3cb2ccc0: SecRule "TX:/^MSC_/" "!@streq 0" "phase:2,log,auditlog,id:200005,t:none,deny,msg:'ModSecurity internal error flagged: %{MATCHED_VAR_NAME}'"
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][4] Rule returned 0.
[15/Oct/2018:09:24:53 --0400] [localhost/sid#55ba3caebc50][rid#55ba3cb6aba0][/][9] No match, not chained -> mode NEXT_RULE.

v3:

[153961061652.969219] [/?a=upload] [4] Starting phase REQUEST_BODY. (SecRules 2)
[153961061652.969219] [/?a=upload] [9] Multipart: Boundary: ------------------------ce4467f3b6a2ffc6
[153961061652.969219] [/?a=upload] [9] Multipart: Added part header "Content-Disposition" "form-data; name="file1"; filename="file1.txt"".
[153961061652.969219] [/?a=upload] [9] Multipart: Added part header "Content-Type" "text/plain".
[153961061652.969219] [/?a=upload] [9] Multipart: Content-Disposition name: file1.
[153961061652.969219] [/?a=upload] [9] Multipart: Content-Disposition filename: file1.txt.
[153961061652.969219] [/?a=upload] [9] Multipart: Added file part to the list: name "file1" file name "file1.txt" (offset 140, length 8)
[153961061652.969219] [/?a=upload] [9] Multipart: Added part header "Content-Disposition" "form-data; name="file2"".
[153961061652.969219] [/?a=upload] [9] Multipart: Content-Disposition name: file2.
[153961061652.969219] [/?a=upload] [9] Multipart: Added data to variable: file2.txt
[153961061652.969219] [/?a=upload] [9] Multipart: Added part to the list: name "file2" (offset 242, length 9)
[153961061652.969219] [/?a=upload] [4] Adding request argument (BODY): name "file2", value "file2.txt"
[153961061652.969219] [/?a=upload] [4] Multipart: Cleanup started (remove files Not set)
[153961061652.969219] [/?a=upload] [9] This phase consists of 4 rule(s).
[153961061652.969219] [/?a=upload] [4] (Rule: 200002) Executing operator "Eq" with param "0" against REQBODY_ERROR.
[153961061652.969219] [/?a=upload] [9] Target value: "0" (Variable: REQBODY_ERROR)
[153961061652.969219] [/?a=upload] [4] Rule returned 0.
[153961061652.969219] [/?a=upload] [9] Matched vars cleaned.
[153961061652.969219] [/?a=upload] [4] (Rule: 200003) Executing operator "Eq" with param "0" against MULTIPART_STRICT_ERROR.
[153961061652.969219] [/?a=upload] [9] Target value: "0" (Variable: MULTIPART_STRICT_ERROR)
[153961061652.969219] [/?a=upload] [4] Rule returned 0.
[153961061652.969219] [/?a=upload] [9] Matched vars cleaned.
[153961061652.969219] [/?a=upload] [4] (Rule: 200004) Executing operator "Eq" with param "0" against MULTIPART_UNMATCHED_BOUNDARY.
[153961061652.969219] [/?a=upload] [9] Target value: "2" (Variable: MULTIPART_UNMATCHED_BOUNDARY)
[153961061652.969219] [/?a=upload] [9] Matched vars updated.
[153961061652.969219] [/?a=upload] [9] Saving msg: Multipart parser detected a possible unmatched boundary.
[153961061652.969219] [/?a=upload] [4] Rule returned 1.
[153961061652.969219] [/?a=upload] [9] Running action: log

[victorhora]

A test case was added at 760e425 that should reproduce the same behaviour as reported above.

The interesting thing is that the result of the test case is different from what I expected. For some reason Nginx and Apache connectors have a different behaviour when it comes to validating the MULTIPART_UNMATCHED_BOUNDARY variable.

[victorhora]

I believe this demands further investigation, and being so I'm moving this to the 3.0.4 milestone so we can move ahead with the request and revisit this issue when we have more time.

[airween]

I'll check it soon, hope in next few days.

[airween]

So, I just could checked with v3, and I don't know what would be the expected result :), but I got these at first run:

  # File Name                                         Test Name                                                             Passed?   
--- ---------                                         ---------                                                             -------   
  1 variable-MULTIPART_UNMATCHED_BOUNDARY.json        Testing Variables :: MULTIPART_UNMATCHED_BOUNDARY                     passed!
  2 variable-MULTIPART_UNMATCHED_BOUNDARY.json        Testing Variables :: MULTIPART_UNMATCHED_BOUNDARY - DENY              failed!

As you can see, the 2nd test had failed.

As I see the body of request, the boundaries are wrong - here are the patterns:

79:        "Content-Type":"multipart/form-data; boundary=--------------------------756b6d74fa1a8ee2",
85:        "----------------------------756b6d74fa1a8ee2",
89:        "----------------------------756b6d74fa1a8ee2",
94:        "A----------------------------756b6d74fa1a8ee2",
99:        "----------------------------756b6d74fa1a8ee2--",

Now let see the boundaries:

79:        "--------------------------756b6d74fa1a8ee2",
85:        "----------------------------756b6d74fa1a8ee2",
89:        "----------------------------756b6d74fa1a8ee2",
94:        "A----------------------------756b6d74fa1a8ee2",
99:        "----------------------------756b6d74fa1a8ee2--",

Here you can see, the boundary in header doesn't match with any other boundaries (in body).

I think for that reason, the "@eq 1" rule rightfully denied the request.

Add plus two "-" signs to header in line 79, or remove two signs from boundary in 85 or 89, and 99 (the valid multipart contains two equal boundary (header and leading), and plus one with "--" signals at the end (trailer)).

Hope that's help.

[victorhora]

Hi @airween

Your comments and results of are very welcome here. Thank you :)

But just to make sure we are on the same page and that I'm not getting the multipart thing incorrectly, do you agree that the boundaries should be specified according to RFC1341:

In our case, the boundary is defined like so:
boundary=--------------------------756b6d74fa1a8ee2

Then, each subsequent Content-Disposition will carry the same boundary, but appending "--" to the beginning of the boundary like so:

----------------------------756b6d74fa1a8ee2

And the last boundary should be specified with a "--" to the beginning of the boundary and another "--" to the end. Like so:

----------------------------756b6d74fa1a8ee2--

Regarding the test case, yes it is failing but should be passing I think. I found it interesting that the regression_test ends up having a different behaviour from the Apache/Nginx when it came to validating the MULTIPART_UNMATCHED_BOUNDARY variable. Unless I did something wrong which is a possibility :P

[airween]

Well, you're absolutely right - I totally forgot the additionally "-" signs :). Sorry.
I'll continue this check.

[defanator]

I'll also ask here as it seems to be related one - is it normal that simple request like the following causes 403 triggered by 200004 rule:

# curl -iv -F param1=1 http://localhost/modsec-full/
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 80 (#0)
> POST /modsec-full/ HTTP/1.1
> Host: localhost
> User-Agent: curl/7.47.0
> Accept: */*
> Content-Length: 142
> Expect: 100-continue
> Content-Type: multipart/form-data; boundary=------------------------c0e877fcfd24e4f5
> 
< HTTP/1.1 100 Continue
HTTP/1.1 100 Continue

?

Corresponding request caught by listening on separate port:

# nc -l 9999
POST /modsec-full/ HTTP/1.1
Host: localhost:9999
User-Agent: curl/7.47.0
Accept: */*
Content-Length: 142
Expect: 100-continue
Content-Type: multipart/form-data; boundary=------------------------e4a299f10827f929

--------------------------e4a299f10827f929
Content-Disposition: form-data; name="param1"

1
--------------------------e4a299f10827f929--

[zimmerle]

Changed the default behavior on modsecurity-recommended.conf to not flag the unmatched boundary in that specific use case scenario. By that the behavior is exactly the same as v2.

https://github.com/SpiderLabs/ModSecurity/blob/9ada0a28c8100f905014c128b0e6d11dd75ec7e5/modsecurity.conf-recommended#L53-L54

[airween]

Hi @victorhora

looks like you've found a bug :).

The details - hope this will be clear:

• your request body looks like a well formed, except some small things:
  • I think the EOL mark is the \r\n, not just the \n - ModSecurity looks up the \r at the end of line, but may be this isn't relevant
  • the two form-data header is equal; I think that's no problem, and at the server side the last one will used, but the right way should be like this: name="filedata[]"; filename="f1.txt", and filename="f2.txt" - anyway, this would be a new feature in multipart check
• there is a "A" char at the begin of the boundary at line 10 of request - I don't know, it's just a typo or that's the "invalid boundary" - I suppose that is the last one, that makes sense with expected 403
• that's very important, that the "A" char at the beginning of line isn't an error, it's a valid, also well formed part, and the part will be looks something like this (imagine that this is the content of a text file):

This is a very small test file..
A----------------------------756b6d74fa1a8ee2
Content-Disposition: form-data; name="filedata"; filename="small_text_file.txt"
Content-Type: text/plain

This is another very small test file..

• the expected value in this case after the multipart check is 2 (@eq 2) or non-zero (!@eq 0), if you want to catch it with action 'deny' (but not 1).
• you expect the value 1 (@eq 1), and if it equal to 1, then the result code will be 403 (as expected). The result will be 1 only if there isn't a matched leading or a matched trailing boundary at least.

And the bug, what you've found: if you remove the "A" char from the beginning of boundary, the result also will be 2. I reviewed the multipart.cc, and looks like the condition of 'm_flag_unmatched_boundary = 2' assignment isn't correct, because if all boundary matches, then it also evaluated, so the totally well formed request will result with value 2, instead of 0.

I've sent a PR to you: https://github.com/victorhora/ModSecurity/pull/1. Please check it, and if you think that's right, approve that.

Hope that this will be clean :).

[airween]

Hi @defanator

I'll also ask here as it seems to be related one - is it normal that simple request like the following causes 403 triggered by 200004 rule:

# curl -iv -F param1=1 http://localhost/modsec-full/
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 80 (#0)
> POST /modsec-full/ HTTP/1.1
> Host: localhost
> User-Agent: curl/7.47.0
> Accept: */*
> Content-Length: 142
> Expect: 100-continue
> Content-Type: multipart/form-data; boundary=------------------------c0e877fcfd24e4f5
> 
< HTTP/1.1 100 Continue
HTTP/1.1 100 Continue

?

Corresponding request caught by listening on separate port:

# nc -l 9999
POST /modsec-full/ HTTP/1.1
Host: localhost:9999
User-Agent: curl/7.47.0
Accept: */*
Content-Length: 142
Expect: 100-continue
Content-Type: multipart/form-data; boundary=------------------------e4a299f10827f929

--------------------------e4a299f10827f929
Content-Disposition: form-data; name="param1"

1
--------------------------e4a299f10827f929--

if the rule uses "!@eq 0", then the answer is yes :). That's what I fixed few minutes ago: https://github.com/victorhora/ModSecurity/pull/1

Please check it if you can.