Parsing error when submitting file with an apostrophe (') in filename

[studersi]

Describe the bug

When submitting a file with an apostrophe (') in the filename, ModSecurity fails to parse the request body causing the ModSecurity Recommended Rules to deny the request.

Logs and dumps

2020/07/07 15:03:49 [warn] 26375#26375: *1 [client 127.0.0.1] ModSecurity: Warning. Matched "Operator `Eq' with parameter `0' against variable `MULTIPART_STRICT_ERROR' (Value: `1' ) [file "conf/modsecurity.conf"] [line "30"] [id "200003"] [rev ""] [msg "Multipart request body failed strict validation: PE 0, BQ 0, BW 0, DB 0, DA 0, HF 0, LF 0, SM 0, IQ 1, IP 0, IH 0, FL "] [data ""] [severity "0"] [ver ""] [maturity "0"] [accuracy "0"] [hostname "127.0.0.1"] [uri "/"] [unique_id "159413422979.139957"] [ref "v173,1"], client: 127.0.0.1, server: localhost, request: "POST / HTTP/1.1", host: "localhost"                                                                                                            

---h0TpBzpy---A--
[07/Jul/2020:15:03:49 +0000] 159413422979.139957 127.0.0.1 51588 127.0.0.1 80
---h0TpBzpy---B--
POST / HTTP/1.1  
Host: localhost                                                                                                                                                                                                                               User-Agent: curl/7.64.0                                                                                                                                                                                                                       Accept: */*                                                                                                                                                                                                                                  
Content-Length: 215
Content-Type: multipart/form-data; boundary=------------------------31e93f28b36621f5
            
---h0TpBzpy---C--
--------------------------31e93f28b36621f5
Content-Disposition: form-data; name="filename"; filename="it's_a_test.txt"
Content-Type: text/plain
                                            
TEST_FILE_CONTENT
                    
--------------------------31e93f28b36621f5--

                 
---h0TpBzpy---D--
                 
---h0TpBzpy---F--
HTTP/1.1 400                                                                                                                                                                                                                                  
Server: nginx                                                                                                                                                                                                                                 
Date: Tue, 07 Jul 2020 15:03:49 GMT                                                                                                                                                                                                           
Content-Length: 166                                                                                                                                                                                                                           
Content-Type: text/html                                                                                                                                                                                                                       
Connection: close                                                                                                                                                                                                                             
                           
---h0TpBzpy---G--

---h0TpBzpy---H--
ModSecurity: Access denied with code 400 (phase 2). Matched "Operator `Eq' with parameter `0' against variable `MULTIPART_STRICT_ERROR' (Value: `1' ) [file "conf/modsecurity.conf"] [line "30"] [id "200003"] [rev ""] [msg "Multipart reque$
t body failed strict validation: PE 0, BQ 0, BW 0, DB 0, DA 0, HF 0, LF 0, SM 0, IQ 1, IP 0, IH 0, FL "] [data ""] [severity "0"] [ver ""] [maturity "0"] [accuracy "0"] [hostname "127.0.0.1"] [uri "/"] [unique_id "159413422979.139957"] [$
ef "v173,1"]

---h0TpBzpy---I--

---h0TpBzpy---J--

---h0TpBzpy---Z--

To Reproduce

• Set up environment: https://www.netnea.com/cms/nginx-modsecurity-tutorials/
• Create test files (one with, one without apostrophe):

echo "TEST_FILE_CONTENT" > its_a_test.txt
cp its_a_test.txt "it's_a_test.txt"

• Perform working request (without apostrophe):

curl -F "filename=@its_a_test.txt" localhost

<html>
<head><title>405 Not Allowed</title></head>
<body bgcolor="white">
<center><h1>405 Not Allowed</h1></center>
<hr><center>nginx</center>
</body>
</html>

• Perform failing equest (with apostrophe):

curl -F "filename=@it's_a_test.txt" localhost

<html>
<head><title>400 Bad Request</title></head>
<body bgcolor="white">
<center><h1>400 Bad Request</h1></center>
<hr><center>nginx</center>
</body>
</html>

Expected behavior

See working request above. ModSec should not intervene.

Server (please complete the following information):

• ModSecurity version (and connector): modsecurity-v3.0.0 with modsecurity-nginx-v1.0.0
• WebServer: nginx-1.13.9
• OS (and distro): Debian GNU/Linux 10

Rule Set (please complete the following information):
Using minimal configuration:

$ cat /opt/nginx-1.13.9/conf/nginx.conf
daemon            off;
worker_processes  2;
user              www-data;

load_module modules/ngx_http_modsecurity_module.so;

events {
    use           epoll;
    worker_connections  128;
}

error_log         logs/error.log info;

http {
    modsecurity on;
    server_tokens off;
    include       mime.types;
    charset       utf-8;

    modsecurity_rules_file conf/modsecurity.conf;

    access_log    logs/access.log  combined;

    server {
        server_name   localhost;
        listen        0.0.0.0:80;

        error_page    500 502 503 504  /50x.html;

        location      / {
            root      html;
        }

    }

}

$ cat /opt/nginx-1.13.9/conf/modsecurity.conf 
SecRuleEngine On
SecRequestBodyAccess On
SecRequestBodyLimit 13107200

SecRequestBodyNoFilesLimit 64000

SecResponseBodyAccess On
SecResponseBodyLimit 10000000

SecTmpDir /tmp/
SecDataDir /tmp/
SecUploadDir /tmp/

SecAuditEngine On
SecAuditLogRelevantStatus "^(?:5|4(?!04))"
SecAuditLogParts ABCDEFGHIJZ

SecAuditLogType Serial
SecAuditLog logs/modsec_audit.log
SecAuditLogStorageDir logs/audit

SecDebugLog logs/modsec_debug.log
SecDebugLogLevel 0

SecRule REQUEST_HEADERS:Content-Type "(?:application(?:/soap\+|/)|text/)xml" \
  "id:200000,phase:1,t:none,t:lowercase,pass,nolog,ctl:requestBodyProcessor=XML"

SecRule REQUEST_HEADERS:Content-Type "application/json" \
  "id:200001,phase:1,t:none,t:lowercase,pass,nolog,ctl:requestBodyProcessor=JSON"

SecRule REQBODY_ERROR "!@eq 0" \
  "id:200002,phase:2,t:none,deny,status:400,log,\
  msg:'Failed to parse request body.',logdata:'%{reqbody_error_msg}',severity:2"

SecRule MULTIPART_STRICT_ERROR "!@eq 0" \
"id:200003,phase:2,t:none,deny,status:400,log, \
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}'"

SecRule TX:/^MSC_/ "!@streq 0" \
  "id:200005,phase:2,t:none,deny,status:500,\
  msg:'ModSecurity internal error flagged: %{MATCHED_VAR_NAME}'"

[martinhsv]

Hi @studersi ,

Since you're using a fairly old ModSecurity (v3.0.0), I would suggest upgrading to a more recent version like v3.0.4. I'm fairly sure that the issue you describe does not exist there -- or in the previous version (v3.0.3) for that matter.

[studersi]

Thank you for your reply!

I just confirmed it with the following versions:

• nginx-1.19.1
• modsecurity-v3.0.4
• modsecurity-nginx-v1.0.1

The behaviour is the same:

$ curl -F "filename=@its_a_test.txt" localhost
<html>
<head><title>405 Not Allowed</title></head>
<body>
<center><h1>405 Not Allowed</h1></center>
<hr><center>nginx</center>
</body>
</html>

$ curl -F "filename=@it's_a_test.txt" localhost
<html>
<head><title>400 Bad Request</title></head>
<body>
<center><h1>400 Bad Request</h1></center>
<hr><center>nginx</center>
</body>
</html>

[martinhsv]

Ah, ok, my mistake.

You're hitting the multipart strict check (rule 200003) -- specifically triggered by the invalid quoting condition ('IQ 1').

The condition you're hitting shouldn't hamper parsing of the remainder of the request body (at least not in 3.0.4), so you could probably just turn off the rule detection of this condition.

One way to do this is to just remove/comment-out rule 200003. A generally safer approach is to remove 200003, but replace it with rules that check for each of the individual conditions except for the one(s) that are causing you well-understood false positives.

A member of the community has posted a sample of this approach near the bottom of this issue: #2267

[studersi]

Indeed but this appears to be more of a workaround for an underlying problem. I do not see why an apostrophe should trigger unmatched quotation error.

Correct me if I am wrong but it appears to me that according to Section 4.1 of RFC 6266 the apostrophe should be a perfectly normal character in the filename-parm value as defined in Section 3.6 RFC 2616 and Section 2.2 RFC 2616. So it should not be flagged as a violation during parsing.

[martinhsv]

Hi @studersi ,

' ... more of a workaround ... '

It sure is.

As a general note, the individual conditions tested within MULTIPART_STRICT_ERROR are not all violations of the standard. By design, they test things that may indeed be perfectly RFC-compliant. But these same things are (or at least were historically) uncommon/unexpected, and hence might be indicators of suspicious activity.

It's certainly appropriate to reassess the MULTIPART_STRICT_ERROR conditions to see whether adjustments ought to be made. Input from the community on what false positives are causing a nuisance is useful to such reassessment, so thanks for raising your case.

[martinhsv]

I have looked into this a bit more, so by way of update ...

First, I'll agree that it would be nice to loosen this restriction somewhat if possible. Even in English, use cases for a single quote (particularly when used as an apostrophe) are not trivially rare -- particularly when using certain proper names (e.g. filename="o'toole.txt") And this can be even more common in other languages where apostrophe usage is more frequent -- either in the native language, or as a consequence of transliteration from non-Roman alphabets.

I have identified when and why this more restrictive form of 'invalid quoting' checking was introduced. It was back in 2013, and it was done quite intentionally to avoid a possible bypass related to how php performs multipart parsing (#460) .

I have examined the php code (only briefly thus far), and I suspect that we can loosen this detection in cases where the entire value is enclosed in double-quotes.

But further work would be required to confirm that we would not be inadvertently re-introducing a security bypass.

[marcstern]

If refusing single quotes is a security feature, I think it's a mistake. this should be implemented by a rule, so it can be managed correctly.
The parser should accept it and a rule can forbid it.

[martinhsv]

@marcstern ,

That's (mostly/sort of) exactly what happens now. The presence of a single quote within the value does not halt parsing. Any block of the transaction will occur as a result of a rule that checks either MULTIPART_STRICT_ERROR ( as rule 200003 in modsecurity.conf-recommended does) or a more specific check for the value of MULTIPART_INVALID_QUOTING). In other words this already can be managed by rule adjustment.

Having said that, I did consider the possibility of removing the check for this use case from the parser.

There are some points in favour of that possibility:

• identifying the presence of suspicious specific characters within particular values is typically the role of a broader rule set, not the job of the engine's parser
• the current way to manage the rules means you cannot turn off the IQ condition for [name="a'bc.txt"] but still retain the detection of [name='abc.txt']

On the other hand there are some arguments against:

• because the potential bypass vulnerability is integral to the parsing of the multipart parsing itself, one can argue that it is the proper concern of the parser itself to signal
• perhaps more importantly, if the parser itself does not signal the condition somehow, I don't see a very clean way for a rule writer to identify the situation. Suppose an ordinary multipart-part (not a file upload) has [name="a'bc.txt"]. That value will be added to collections where it will be indistinguishable (from a rule perspective) from names that were added NOT from multipart requests; and then one loses the ability to distinguish that single-quote might be dangerous in multipart names but perhaps not of concern in other name values.

[marcstern]

one loses the ability to distinguish that single-quote might be dangerous in multipart names but perhaps not of concern in other name values

We can check that with a chained rule

[martinhsv]

By which I guess you mean, the first rule in the chain checks that the overall Content-Type was indeed multipart (or more likely checking REQBODY_PROCESSOR) and then the second rule in the chain checks for single-quote chars. That's fair. But then a separate rule is needed to also check filename content, for example. I'm still not completely sold that this approach would overall yield a cleaner solution. Worth thinking about though.

[marcstern]

a separate rule is needed to also check filename content

That's the goal of the FILES_NAMES collection

[martinhsv]

I didn't mean that a second chained rule pair comparable to the first one was required. Merely that an additional rule would be needed.

[marcstern]

And it's normal to use an additional rule to perform a separate functional check ;-)

[andrewroazen]

I have a situation where a form submitting to PHP has INPUT type="file" elements with attributes such as name="obj_size['a1b3ff']" that trigger IQ 1 even though the filenames are fine. I remove the single quotes and the problem disappears. This seems like a very bad parser at work if it can't discern a name attribute value (which the rule isn't supposed to detect or object to) from a passed filename. I hate to chime in on this, but is there a real-world exploit the rule actually prevented or can it quietly be retired since filenames having ' in them isn't a problem per se?

[martinhsv]

Hi @andrewroazen ,

Please see the earlier comment in this thread #2352 (comment) .

Yes, the check was made more stringent due to idiosyncrasies -- and hence potential evasions -- due to PHP's parsing.

The reason this ticket is still open is because the current behaviour is indeed inconvenient and probably unnecessarily restrictive.

In the meantime, a workaround available to you is to exclude IQ from your rule tests.

[andrewroazen]

Where's the documentation as to how this is done, I commented out some lines and restarted httpd but the errors still occur on filenames with ' in them suggesting I did it wrong (or there's some compilation step I overlooked).

[martinhsv]

There is no recompilation involved.

The detection in question happens because of rules, so the way to turn 'off' this detection is to modify your rules.

Have you had a look at discussion in #2267 ? (This was referenced earlier in this issue -- in the July 7 comment). That goes over some options.

[martinhsv]

I have recently both:

• done some more code inspection of php source code and
• experimented with both an older version of php (5.3) and an up-to-date version

... and I'm fairly confident that there is no risk with loosening this restriction, at least to the extent of my comment on Dec. 2.

Specifically, if the entire value is within double-quotes, then the presence of single-quote characters within the value should not trigger the IQ condition.

I'll plan to put together a pull request sometime within the next couple of weeks.

[willc-work]

Just FYI I had a look at the proposed solution in #2267 but this supposes you have direct access to the .conf file. As I am adding/removing the rules through annotations in my ingress the syntax didnt work. Specifically it did not like single quotes within double quotes.

So for example, the following fails with a parsing error:

SecRule MULTIPART_LF_LINE "!@eq 0" \
"id:'200011',phase:2,t:none,log,deny,status:400, \
msg:'Multipart request body failed custom  validation: DB %{MULTIPART_LF_LINE}.'"

and

SecRule MULTIPART_LF_LINE "!@eq 0" \
"id:200011,phase:2,t:none,log,deny,status:400, \
msg:`Multipart request body failed custom  validation: DB %{MULTIPART_LF_LINE}.`"

is accepted and works to re-enable a single rule after removing the block (200003). This removes single quotes from around the id: value and swaps single quotes to backticks for the msg: content.

[martinhsv]

Hello @willc-work ,

(Note that I will be closing this issue shortly since a software change to address it is already available in v2, and is expected to shortly be likewise in v3. But given what you've written, I'll assume that you cannot independently upgrade your code.)

First, I'm not really sure what you mean by your 'ingress'. If you are using some other software to manage ModSecurity configuration, you might want to consult documentation for that other software. It's quite possible that there is some useful notes there -- perhaps using backslash-escaping?

But, If you really cannot use single quotes for rule updates in your situation one option could be to just not use them.

For the specification of "id:", I'm not aware of any situations where the single quotes are required. You can simply specify (as you did in your second example): id:200011.

Avoiding single quotes entirely may mean abandoning use of the 'msg' action.

This would be quite disadvantageous if using something that is the equivalent of 200003 because it is desirable to know which of the variables within MULTIPART_STRICT_ERROR was flagged.

However if you are using the style of one rule per individual variable as in the example here:
#2267 (comment)

... then one could argue there is limited value in including the msg at all, since you can tell by the Rule id number reported which specific condition was hit.

Alternatively, if you really do think you need a more human-readable message, you still use the msg action, but with a message that avoids creating the need for single quotes -- for example by avoiding whitespace characters:

msg:Multipart_request_body_failed___Invalid_Quoting

In fact, you may even be able to leave spaces intact. Try experimenting.

[martinhsv]

Addressed via:
#2660
#2661